Skip to content

Make it possible to start multiple transactions using the same initial transaction snapshot - CORE-6018 #193

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

asfernandes
Copy link
Member

@asfernandes asfernandes commented Feb 22, 2019

With this feature it's possible to create parallel (via different attachments) processes reading consistent data from a database.

For example, a backup process may create multiple threads paralleling read data from the database.

Also a web service may dispatch distributed sub services paralleling doing some processing.

That is accomplished creating a transaction with SET TRANSACTION SNAPSHOT [ AT NUMBER <snapshot number> ] or isc_tpb_at_snapshot_number.

The <snapshot number> from the first transaction may be obtained with RDB$GET_CONTEXT('SYSTEM', 'SNAPSHOT_NUMBER') or transaction info call with fb_info_tra_snapshot_number.

Also added CORE-6017 - Add transaction info fb_info_tra_snapshot_number.

@hvlad
Copy link
Member

hvlad commented Feb 22, 2019

Good start.

But it seems too heavy and complex at the part that ensure shared snapshot existence.
Usage of LM lock for every non-RC transaction which could potentially share its snapshot with other transactions is too much cost, as for me.

When new snapshot is created using another snapshot number, all we need is to ensure that another
snapshot is exists when we create new one. To do it, it is enough to:

  • reserve new slot at SnapshotList
  • put there passed snapshot number
  • search SnapshotList for another slot with passed snapshot number
  • if it exists - OK
  • else - deallocate new slot and throw error

Caller application should ensure that base\source transaction is not finished while new transaction
is started. And i see no reason to not do it for app developer.

Last issue with this approach it how engine should know snapshot number of base transaction.
To solve it i offer to pass snapshot number instead of base transaction number into TPB\SET TRANSACTION.
User application anyway must know something about base transaction before start new one - in your
scheme it is base transaction number. In my - snapshot number used by base transaction. It could be
obtained using isc_transaction_info\context_variable - i.e. in the same way as transaction number.

@asfernandes
Copy link
Member Author

Caller application should ensure that base\source transaction is not finished while new transaction is started. And i see no reason to not do it for app developer.

What would be the implication if caller get the commit number, finish the transaction and later pass it to the new transaction? Would the part about SnapshotList scanning detect the problem, i.e., like current code detect that TR1 was already finished?

@hvlad
Copy link
Member

hvlad commented Feb 22, 2019

Caller application should ensure that base\source transaction is not finished while new transaction is started. And i see no reason to not do it for app developer.

What would be the implication if caller get the commit number,

You mean snapshot number here, correct ?

finish the transaction and later pass it to the new transaction? Would the part about SnapshotList scanning detect the problem, i.e., like current code detect that TR1 was already finished?

Sure.

If snapshot number to reuse\share is present in SnapshotList while we allocate and fill new slot - it is safe
for us. Because nobody can remove backversions visible for this snapshot. It is not important who "owns"
snapshot. What is important - is to guarantee that nobody could see snapshot list without this snapshot number before we allocate new slot with it. After snapshot number is put into new slot - TR1 (and its
snapshot ) is not important anymore.

@asfernandes
Copy link
Member Author

asfernandes commented Feb 22, 2019

@hvlad I changed the code accordingly to your comments. Could you check again?

I didn't changed (not even thought about) the SQL command / TPB constant in regard to the change of "base transaction number" to "base snapshot number".

@hvlad
Copy link
Member

hvlad commented Feb 22, 2019

Much better, thanks.
Two notes:

  1. First create new snapshot with passed CN, then look for existing one (at another slot).
    Else base snapshot could disappear between succesful search and allocating new slot.
    Correct order of snapshot "cloning" is described at my first comment.
  2. When looking for existing snapshot, check also if slot is used (attachment_id is not zero) - look at TipCache::allocateSnapshotSlot()

@asfernandes
Copy link
Member Author

@hvlad a question about the original code: allocateSnapshotSlot gets free slots that have attachment_id == 0. And beginSnapshot may parallel call allocateSnapshotSlot and just later set the allocated slot->attachment_id. Can't that mean there is a race condition that more than one execution catches the same previously free slot?

@asfernandes
Copy link
Member Author

Ah, there is the SharedMutexGuard, but then, doesn't it protect about the the case you noted as "1. base snapshot could disappear between succesful search and allocating new slot"?

@hvlad
Copy link
Member

hvlad commented Feb 23, 2019

Oops, i forget about SharedMutexGuard. You are correct, there is no need in careful order.
So, things more easy than i thought

case fb_info_tra_snapshot_cn:
length = INF_convert(static_cast<SINT64>(transaction->tra_snapshot_number), buffer);
break;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems we have misunderstanding here. At fb-devel we agree that:

I also suggest that new isc_info_tra_snapshot_number returns the same data as SNAPSHOT_CN variable.

Yes, no problem. But for RC it will be NULL as there is no request.

but code above returns 0 for every kind if RC transaction, while SNAPSHOT_CN variable returns request snapshot number, if available.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Transaction info API have no request, so the request snapshot cannot be get. I did returned 0 meaning NULL in this context. How would you want to return NULL on transaction info?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Transaction info API have no request, so the request snapshot cannot be get

Yes. But when we use the same naming (fb_info_tra_snapshot_cn, SNAPSHOT_CN) it suggests that both ways is the equal. Thus i propose to change fb_info_tra_snapshot_cn by fb_info_tra_snapshot_number.

As for 0 instead of NULL - no objection of course.

@asfernandes asfernandes force-pushed the work/sharing-snapshot-cn branch from 3044836 to c000aba Compare March 1, 2019 15:25

if (!found)
ERR_post(Arg::Gds(isc_tra_snapshot_does_not_exist));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, it makes sense to add value of missing snapshot number into error message

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As string? isc_arg_number is 32-bit...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same way as isc_concurrent_transaction reports transaction number.
Hmm... is it broken ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it is really broken now with 64-bit transaction numbers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand why Num::Num constructor accepts an intptr_t instead of proper 32 bits.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because all items put into status-vector array ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the constructor parameter does not need to be. But anyway, it does not seems to catch the 64 bits as a warning when changing it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Time to introduce isc_arg_number2 ? :)

@asfernandes asfernandes changed the title Initial version for transaction snapshot sharing. Make it possible to start multiple transactions using the same initial transaction snapshot - CORE-6018 Mar 1, 2019
@asfernandes asfernandes marked this pull request as ready for review March 1, 2019 18:13
@asfernandes asfernandes merged commit cfbcbed into FirebirdSQL:master Mar 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants