Skip to content
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.

Sign up for GitHub

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

Jump to bottom

sql: use the same version of table descriptor for the lifetime of a transaction #2948

Closed
petermattis opened this issue Oct 29, 2015 · 5 comments
Closed

sql: use the same version of table descriptor for the lifetime of a transaction #2948

petermattis opened this issue Oct 29, 2015 · 5 comments
Assignees
@vivekmenezes
Milestone
1.1

Comments

@petermattis
Copy link
Collaborator

The table descriptor lease mechanism as currently implemented guarantees that the same version of the table is used for the lifetime of a single SQL operation, but different operations within a single transaction can see different versions of the table. A transaction which requires the table version to be unchanging across individual operations seems like it would run afoul of a concurrent schema modification in any case. For example, a SELECT * will retrieve any unknown number of columns if a concurrent add/drop column operation occurs. But even if it is safe for the version to be changing within a transaction, it violates the principle of lease surprise for the user.

The table descriptor lease RFC talks about keeping track of which version of a table is used within a transaction and not letting the version change across operations. This is mildly complicated: we have to store the versions in the session transaction object and not release local leases for versions on uncommitted transactions. Marking this as a 1.0 issue because it doesn't seem urgent to address yet we will likely want to address it at some point.
@petermattis petermattis added this to the 1.0 milestone Oct 29, 2015
@petermattis petermattis added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) and removed SQL labels Feb 13, 2016
@tamird
Copy link
Contributor

tamird commented Jun 30, 2016

Cross-linking #7508.

@tamird tamird mentioned this issue Jun 30, 2016
Closed
@spencerkimball
Copy link
Member

@vivekmenezes @tamird @knz Yikes, is this not the case during a single txn? I'm assigning this to @vivekmenezes to re-assign and prioritize as necessary.

@vivekmenezes
Copy link
Contributor

As described in the report, we do use a lease for a stmt, but across statements a close to expiring lease can be released and another lease acquired with the expectation that it is a new lease with the same version. See sql.removeLeaseIfExpiring . This can easily be fixed to check that the versions are the same but I suspect the tests will prolong the fix. But nonetheless worth putting into 1.0

@knz knz mentioned this issue Apr 6, 2017
Closed
22 tasks
@vivekmenezes vivekmenezes removed C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) high priority labels Apr 25, 2017
@vivekmenezes
Copy link
Contributor

this is turning into a bigger change than I expected. I'm moving this to 1.1

@vivekmenezes vivekmenezes modified the milestones: 1.1, 1.0 Apr 27, 2017
vivekmenezes added a commit to vivekmenezes/cockroach that referenced this issue May 23, 2017
@vivekmenezes
sql: use txn timestamp when comparing time in table leases
79916f2 
Prior to this change we used the clocks current time when
deciding if a lease expired. We would also store the current
time as the modification time for the table descriptor.
It was much harder to see if we could hit weird race conditions
and it was almost impossible to reasona around correctness.

This change moves all the table lease logic to use the txn
timestamp when writing timestamps and when comparing timestamps
for expiration. It is a lot easier to reason about the code
after this change.

This change also returns a retryable error when a transaction
encounters two different versions of the same table descriptor.

fixes cockroachdb#2948
vivekmenezes added a commit to vivekmenezes/cockroach that referenced this issue May 23, 2017
@vivekmenezes
sql: use txn timestamp when comparing time in table leases
0934b5b 
Prior to this change we used the clocks current time when
deciding if a lease expired. We would also store the current
time as the modification time for the table descriptor.
It was much harder to see if we could hit weird race conditions
and it was almost impossible to reasona around correctness.

This change moves all the table lease logic to use the txn
timestamp when writing timestamps and when comparing timestamps
for expiration. It is a lot easier to reason about the code
after this change.

This change also returns a retryable error when a transaction
encounters two different versions of the same table descriptor.

fixes cockroachdb#2948
vivekmenezes added a commit to vivekmenezes/cockroach that referenced this issue May 23, 2017
@vivekmenezes
sql: use txn timestamp when comparing time in table leases
3080a94 
Prior to this change we used the clocks current time when
deciding if a lease expired. We would also store the current
time as the modification time for the table descriptor.
It was much harder to see if we could hit weird race conditions
and it was almost impossible to reasona around correctness.

This change moves all the table lease logic to use the txn
timestamp when writing timestamps and when comparing timestamps
for expiration. It is a lot easier to reason about the code
after this change.

This change also returns a retryable error when a transaction
encounters two different versions of the same table descriptor.

fixes cockroachdb#2948
vivekmenezes added a commit to vivekmenezes/cockroach that referenced this issue May 24, 2017
@vivekmenezes
sql: use txn timestamp when comparing time in table leases
65d4318 
Prior to this change we used the clocks current time when
deciding if a lease expired. We would also store the current
time as the modification time for the table descriptor.
It was much harder to see if we could hit weird race conditions
and it was almost impossible to reasona around correctness.

This change moves all the table lease logic to use the txn
timestamp when writing timestamps and when comparing timestamps
for expiration. It is a lot easier to reason about the code
after this change.

This change also returns a retryable error when a transaction
encounters two different versions of the same table descriptor.

fixes cockroachdb#2948
@vivekmenezes vivekmenezes mentioned this issue Jun 12, 2017
Merged
vivekmenezes added a commit to vivekmenezes/cockroach that referenced this issue Jun 14, 2017
@vivekmenezes
sql: changed LeaseManager API to return/use table descriptors
a95331a 
The Acquire/AcquireByName and Release() methods no longer
return/use a LeaseState and instead return/use a
sqlbase.TableDescriptor with an expiration time.

the LeaseManager is moving towards a model where it returns
a table descriptor with an expiration time for the use of the
table descriptor. For now this table descriptor is associated
with a lease, but in the future it might be an older version of
the table descriptor, in which case it will not have a lease.

With this change, leases on the same table descriptor version
are renewed under the covers without needing the API to issue
an explicit release on a table descriptor version. The refcount
on the old lease is simply transferred to the new one. The new
lease is guaranteed to have a higher expiration time and thus a
prior returned expiration time returned by the API is guaranteed
to be contained within the new lease expiration time.

A Release() on the table descriptor will reduce the refcount on the
current lease. In the future, this will reduce the refcount on a table
version which might not be associated with a lease.

Removed the need to use removeLeaseIfExpiring since once a transaction
has picked a timestamp and a valid table descriptor for the timestamp
the table descriptor need not be reacquired through the API.

related to cockroachdb#2948
vivekmenezes added a commit to vivekmenezes/cockroach that referenced this issue Jun 14, 2017
@vivekmenezes
sql: changed LeaseManager API to return/use table descriptors
1f70f31 
The Acquire/AcquireByName and Release() methods no longer
return/use a LeaseState and instead return/use a
sqlbase.TableDescriptor with an expiration time.

the LeaseManager is moving towards a model where it returns
a table descriptor with an expiration time for the use of the
table descriptor. For now this table descriptor is associated
with a lease, but in the future it might be an older version of
the table descriptor, in which case it will not have a lease.

With this change, leases on the same table descriptor version
are renewed under the covers without needing the API to issue
an explicit release on a table descriptor version. The refcount
on the old lease is simply transferred to the new one. The new
lease is guaranteed to have a higher expiration time and thus a
prior returned expiration time returned by the API is guaranteed
to be contained within the new lease expiration time.

A Release() on the table descriptor will reduce the refcount on the
current lease. In the future, this will reduce the refcount on a table
version which might not be associated with a lease.

Removed the need to use removeLeaseIfExpiring since once a transaction
has picked a timestamp and a valid table descriptor for the timestamp
the table descriptor need not be reacquired through the API.

related to cockroachdb#2948
vivekmenezes added a commit to vivekmenezes/cockroach that referenced this issue Jun 16, 2017
@vivekmenezes
sql: changed LeaseManager API to return/use table descriptors
cdc87dd 
The Acquire/AcquireByName and Release() methods no longer
return/use a LeaseState and instead return/use a
sqlbase.TableDescriptor with an expiration time.

the LeaseManager is moving towards a model where it returns
a table descriptor with an expiration time for the use of the
table descriptor. For now this table descriptor is associated
with a lease, but in the future it might be an older version of
the table descriptor, in which case it will not have a lease.

With this change, leases on the same table descriptor version
are renewed under the covers without needing the API to issue
an explicit release on a table descriptor version. The refcount
on the old lease is simply transferred to the new one. The new
lease is guaranteed to have a higher expiration time and thus a
prior returned expiration time returned by the API is guaranteed
to be contained within the new lease expiration time.

A Release() on the table descriptor will reduce the refcount on the
current lease. In the future, this will reduce the refcount on a table
version which might not be associated with a lease.

Removed the need to use removeLeaseIfExpiring since once a transaction
has picked a timestamp and a valid table descriptor for the timestamp
the table descriptor need not be reacquired through the API.

related to cockroachdb#2948
@vivekmenezes
Copy link
Contributor

Closing this issue because we now use the same version of a table descriptor during a transaction and hold the version in a TableCollection object for the life of a transaction. This is tested by TestTxnObeysTableModificationTime

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vivekmenezes vivekmenezes

Labels
None yet
Projects
None yet
Milestone
1.1
Development

No branches or pull requests
4 participants
@tamird @spencerkimball @petermattis @vivekmenezes