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.

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

[YSQL] Support SKIP LOCKED for explicit row-level locks in REPEATABLE READ isolation level #2742

Closed
lhotari opened this issue Oct 28, 2019 · 2 comments
Assignees
Labels
area/ysql Yugabyte SQL (YSQL) community/request Issues created by external users kind/bug This issue is a bug priority/medium Medium priority issue
Milestone

Comments

@lhotari
Copy link

lhotari commented Oct 28, 2019

Jira Link: DB-3138
For implementing work queues with a database, it's important that the database supports "SKIP LOCKED" locking option in a SELECT .. FOR UPDATE statement. (Issue for SELECT .. FOR UPDATE is #1199)

PostgreSQL also supports "NOWAIT" and "SKIP LOCKED" options for "SELECT ... FOR UPDATE" .

The utility of "SKIP LOCKED" is explained in this article: https://www.2ndquadrant.com/en/blog/what-is-select-skip-locked-for-in-postgresql-9-5/

Besides PostgreSQL, "SKIP LOCKED" option is also supported in MySQL, MS SQL Server and Oracle. It will be hard to port applications relying on the "SKIP LOCKED" lock option to YugaByte when YB lacks this support.

@mme-private
Copy link

Is there any progress on SKIP LOCKED?

@teddyknox
Copy link

bump

pkj415 added a commit to pkj415/yugabyte-db that referenced this issue Nov 30, 2021
…ing semantics + support SKIP LOCKED for REPEATABLE READ isolation level

Summary:
Part 1 (for first 2 issues):
----------------------------

Currently in YB, to lock rows explicitly, YSQL sends a marker with read requests to tserver processes.
A lock is taken on the longest possible prefix of the DocKey that will contain the data matching the query.
Example: Consider a table with pk (h1, h2, r1, r2). If a read request sets h1, h2, and r1, a lock is
taken on everything that matches that prefix (this is done via a single intent). Similarly, if the read
request sets just h1, a lock is taken on everything that matches that value for h1. And if a read request
doesn't specify h1, a lock is taken on the whole tablet. This is the case for both REPEATABLE READ
and SERIALIZABLE.

This results in 2 issues for REPEATABLE READ isolation in YSQL:
1. Even data that is filtered out later by YSQL is locked.
2. New tuples also can't be inserted that fall in the locked prefix range. So, we are locking future
   non-existant tuples as well.

For any isolation other than SERIALIZABLE, in Pg, locks are only taken on the rows that are "returned" to the
client or parent plan node by the LockRows plan node. Rows that are filtered out by child plan nodes of LockRows
are not locked. In YSQL, LockRows is just a dummy placeholder and we instead pass down locking information to
leaf nodes that scan docdb and hence lock larger chunks of the tablet.

This mismatch happens because explicit locking was implemented (https://phabricator.dev.yugabyte.com/D7007)
by just returning early from ExecLockRows() in YSQL and hence logic in the LockRows node was not used.

This diff performs explicit locking tuple by tuple in the LockRows node similar to Postgres.
This has two benefits -
1. We match postgres locking semantics in REPEATABLE READ (and also will match in the READ COMMITED isolation
   that we plan to add support for in near future).
2. This makes implementation of other Pg locking features like SKIP LOCKED stragiht-forward (since we now
   lock 1 tuple at a time and not whole chunks - which allows others txns to see what rows are still available
   for locking). Without this, such existing Pg features would require lot of code changes.

Locking only rows returned to user as compared to coarser chunks (i.e., by locking a prefix of the pk) might
seem to be inefficient due to the many locks that we now have to take. But note coarser locks can also hurt
performance by locking what isn't to be locked. So, both methods have their disadvantages based on the specific
workload; but locking only rows returned to user gives us behaviour same as Postgres. This serves as the
motivation to switch to only locking rows that are returned to user rather than coarser locks.

NOTE: For SERIALIZABLE level, explicit locking is same as before since we need to lock whole predicates to
block insertion of new rows matching the predicate (which the current prefix-locking achieves).

Part 2: Support for SKIP LOCKED
--------------------------------

After Part 1, SKIP LOCKED becomes straight-forward (since we now lock 1 tuple at a time and not whole
chunks - which allows others txns to see what rows are still available for locking).

The implementation plumbs the wait policy for locking from postgres to tserver process which uses
it in conflict_resolution.cc. If the policy for a certain intent is to skip, in case of conflict we return
a SkipLocking error.

Following the wait_policy in YBCLockTuple() will make the implementation clear.

Test Plan:
1. New test method TestRowLockInJoin: to ensure we lock only those tuples which are returned to user (in REPEATABLE READ isolation).
2. New test method TestSkipLocked: for SKIP LOCKED functionality.
3. TestRowKeyShareLock: This test existed already and assertions in it were based on the assumption that both SERIALIZABLE and REPEATABLE READ take blanket/predicate locks (e.g.: full table if no prefix of the pk is specified). The behaviour in test is corrected to match Pg behaviour. Also some more cases are added to it to test predicate locking in SERIALIZABLE level.
4. TestInsertSelectRowLock, TestDeleteSelectRowLock: Modified tests since yugabyte#2809 is fixed with this.

Reviewers: mbautin, sergei, neil, jason

Subscribers: yql, bogdan

Differential Revision: https://phabricator.dev.yugabyte.com/D13352
pkj415 added a commit that referenced this issue Dec 3, 2021
…ort SKIP LOCKED for REPEATABLE READ isolation level

Summary:
**Part 1 (for first 2 issues):**

Currently in YB, to lock rows explicitly, YSQL sends a marker with read requests to tserver processes.
A lock is taken on the longest possible prefix of the DocKey that will contain the data matching the query.
Example: Consider a table with pk (h1, h2, r1, r2). If a read request sets h1, h2, and r1, a lock is
taken on everything that matches that prefix (this is done via a single intent). Similarly, if the read
request sets just h1, a lock is taken on everything that matches that value for h1. And if a read request
doesn't specify h1, a lock is taken on the whole tablet. This is the case for both REPEATABLE READ
and SERIALIZABLE.

**This results in 2 issues for REPEATABLE READ isolation in YSQL:**
1. Even data that is filtered out later by YSQL is locked.
2. New tuples also can't be inserted that fall in the locked prefix range. With a "prefix" intent, we are locking
    future non-existent tuples as well.

For any isolation other than SERIALIZABLE, in Pg, locks are only taken on the rows that are "returned" to the
client or parent plan node by the LockRows plan node. Rows that are filtered out by child plan nodes of LockRows
are not locked. In YSQL, LockRows is just a dummy placeholder and we instead pass down locking information to
leaf nodes that scan docdb and hence lock larger chunks of the tablet.

This mismatch happens because explicit locking was implemented (https://phabricator.dev.yugabyte.com/D7007)
by just returning early from ExecLockRows() in YSQL and hence logic in the LockRows node was not used.

This diff performs explicit locking tuple by tuple in the LockRows node similar to Postgres.

**This has two benefits -**

i) We match postgres locking semantics in REPEATABLE READ (and also will match in the READ COMMITED isolation
that we plan to add support for in near future).

ii) For REPEATABLE READ/ READ COMMITTED, this makes implementation of other Pg locking features easier -
    a) SKIP LOCKED becomes straight-forward since we now lock 1 tuple at a time and not whole chunks - which allows others
        txns to see what rows are still available for locking). Without this, such existing Pg features would require lot of code changes.
    b) Special handling of conflicts that use "EvalPlanQual" in Postgres will be readily available for YB use as we implement READ
        COMMITTED.

Locking only rows returned to user as compared to coarser chunks (i.e., by locking a prefix of the pk) might
seem to be inefficient due to the many locks that we now have to take. But note coarser locks can also hurt
performance by locking what isn't to be locked. So, both methods have their disadvantages based on the specific
workload; but locking only rows returned to user gives us behaviour same as Postgres. This serves as the
motivation to switch to only locking rows that are returned to user rather than coarser locks.

To follow the flow of logic, start with //nodeLockRows.c//

This diff is a result of conversation from this slack thread - https://yugabyte.slack.com/archives/CAR5BCH29/p1631036276226800

```
NOTE:

i) For SERIALIZABLE level, explicit locking is same as before since we need to lock whole predicates to
block insertion of new rows matching the predicate (which the current prefix-locking achieves).
ii) We can optimize the case of explicit locking on a SELECT with full pk specified by using just 1 RPC call. The will follow after this diff lands (#10429)

```
**Part 2: Support for SKIP LOCKED**

After Part 1, SKIP LOCKED becomes straight-forward (since we now lock 1 tuple at a time and not whole
chunks - which allows others txns to see what rows are still available for locking).

The implementation plumbs the wait policy for locking from postgres to tserver process which uses
it in conflict_resolution.cc. If the policy for a certain intent is to skip, in case of conflict we return
a SkipLocking error.

Following the wait_policy in YBCLockTuple() will make the implementation clear.

Test Plan:
**New tests:**

1. TestRowLockInJoin: to ensure we lock only those tuples which are returned to user (in REPEATABLE READ isolation).
2. TestSkipLocked: for SKIP LOCKED functionality.

**Modified existing tests:**
1. TestInsertSelectRowLock, TestDeleteSelectRowLock: Modified tests since #2809 is fixed with this.
2. TestRowKeyShareLock: This test existed already and assertions in it were based on the assumption that both SERIALIZABLE and REPEATABLE READ take blanket/predicate locks (e.g.: full table if no prefix of the pk is specified). The behaviour in test is corrected to match Pg behaviour. Also some more cases are added to it to test predicate locking in SERIALIZABLE level.
3. TestPgExplicitLocks, TestPgForeignKeyOptimization and TestPgTransactions.testExplicitLocking

Jenkins: urgent

Reviewers: mbautin, neil, jason, smishra, mihnea, dmitry, sergei

Reviewed By: dmitry, sergei

Subscribers: yql, bogdan

Differential Revision: https://phabricator.dev.yugabyte.com/D13352
@pkj415 pkj415 closed this as completed Dec 3, 2021
pkj415 added a commit to pkj415/yugabyte-db that referenced this issue Feb 5, 2022
…ramework and fix a rare SKIP LOCKED bug.

Summary:
Part 1
------
Add java class org.yb.pgsql.TestPgIsolationRegress to support running tests in
src/test/isolation (similar to src/test/regress).

Enable tests skip-locked and skip-locked-2. The test skip-locked results in a
bug explained and fixed in Part 2 below.

An extra test case yb-modification-followed-by-lock is added.

Part 2
------

1. In skip-locked test, the step s2b sometimes results in a serialization error
   in permutation [s1a s1b s2a s1c s2b s2c]. This happens if the intents of s1
   haven't yet moved to regular db before conflict resolution of s2b starts. The
   following steps in conflict resolution of s2b lead to a serialization error
   in this scenario -
     a) s2b sees a conflicting row lock intent in intents db that corresponds to
        transaction in s1 (in ReadIntentConflicts())
     b) s2b finds s1's transaction to be committed and hence aborts itself (as
        part of CheckLocalCommits())

In step 2, it is correct to abort if the conflicting intents of committed
transaction 1 result in some modification to the regular db. Otherwise, s2b can
ignore the intents of transaction 1 since they are only explicit row lock
intents and locks are released post commit. The fix adds logic to not abort in
case a committed transactions had only conflicting explicit row lock intents
(i.e., those which can only block but not change any data).

2. s2b sometimes worked correctly if the intents of s1 have already been moved
to regular db before conflict resolution of s2b starts because in that case s2b
doesn't see any conflicts.

Test Plan: ./yb_build.sh --java-test org.yb.pgsql.TestPgIsolationRegress#testIsolationRegress

Reviewers: mihnea, sergei

Subscribers: yql

Differential Revision: https://phabricator.dev.yugabyte.com/D14362
pkj415 added a commit that referenced this issue Feb 5, 2022
… and fix rare SKIP LOCKED bugs

Summary:
Part 1
-------
Add java class org.yb.pgsql.TestPgIsolationRegress to support running tests in
src/test/isolation (similar to src/test/regress).

Enable tests skip-locked and skip-locked-2. Without a fix that is explained in
Part 2 below, the skip-locked test would fail rarely due a bug (also explained
in Part 2).

Two extra tests are added -

(1) yb-modification-followed-by-lock to test that a modification doesn't wait on
a lock that was held by a recently committed transaction. This test would have
failed rarely without the fix in part 2 due to the same bug that affects the
skip-locked test.

(2) yb-skip-locked-after-update to assert that a statement with SKIP LOCKED
actually skips locking a tuple that was concurrently modified by another
transaction. The test asserts the skip locked behaviour for three different
states of the concurrent transaction that modified the tuple: pending,
committed but not applied, committed and applied.

Right now, the assertions fail in the latter 2 cases where the transaction has
committed but not applied or has committed and applied. Part 3 explains the
reason for the bug along with the fix.

Part 2
-------

(1) In skip-locked test, the step s2b rarely results in a serialization error
in permutation [s1a s1b s2a s1c s2b s2c]. This happens if the intents of s1
haven't yet moved to regular db before conflict resolution of s2b starts. The
following steps in conflict resolution of s2b lead to a serialization error in
this scenario -
  (a) s2b sees a conflicting row lock intent in intents db that corresponds to
      transaction in s1 (in ReadIntentConflicts())
  (b) s2b finds s1's transaction to be committed and hence aborts itself (as
      part of CheckLocalCommits())

In step b), it is incorrect to abort if the conflicting intents of committed
transaction 1 were only corresponding to explicit lock and not any modification
to the regular db (which is true for statements s1a and s1b). Instead, s2b
should ignore such intents of transaction 1 since they are only explicit row
lock intents and locks are released post commit. The fix adds logic to ignore
such conflicting intents of committed transactions which correspond to only
explicit row lock intents (i.e., those which can only block but not change any
data). This is achieved by usage of a new field "all_lock_only_conflicts" in
conflict_resolution.cc

(2) In cases other than (1) i.e., if the transaction in s1 has already been
applied before conflict resolution for s2b is done, there is no serialization
error. This is because no intents are moved to regular db as part of the "apply"
step of s1 (which in turn is because the intents are lock-only intents and not
a modification). And so, s2b doesn't see any conflicts in both intents db and
regular db.

This fix of ignoring explicit lock only intents of committed transactions solves
the rare issue mentioned in skip-locked test above which occurs if steps
described in (1) happen. Moreover, the fix also ensure that there is no
serialization error in the newly added test yb-modification-followed-by-lock (
the error would have occurred in this test as well without the fix).

Part 3
------
As mentioned earlier, SKIP LOCKED functionality fails in
yb-skip-locked-after-update if the concurrently modified tuple is part of a
transaction that has committed but not applied or has committed and applied. The
reason for this is:

(1) "committed and applied" case: currently, an explicit locking rpc with SKIP
LOCKED will abort if it finds a modified row in regular db with commit time
after read time. We should instead skip locking this row exactly as done in
CheckPriorityInternal() in D13352 i.e., by throwing
TransactionErrorCode::kSkipLocking if the policy is WAIT_SKIP. We check for
conflicts with regular db in StrongConflictChecker::Check() and the fix is to
add the extra handling here.

(2) "committed but not applied" case: even if we don't find conflicting
modifications in regular db that would lead to aborting or skipping the tuple
(i.e., case (1)), we might still find committed conflicting modifications in
intents db. This might happen if a transaction that modified the tuple has
committed but hasn't been applied yet (i.e., intents haven't moved to regular db
yet). Such conflicts are handled in CheckConflictWithCommitted(). Right now we
just abort here or ignore the conflict if all_lock_only_conflicts=true after fix
in Part 1. An extra check similar to (1) is needed here.

These two bugs arise because we missed to throw the
TransactionErrorCode::kSkipLocking error if the policy is WAIT_SKIP at the two
places mentioned above apart from CheckPriorityInternal() in D13352 (the diff
that added SKIP LOCKED functionality).

Note that case (2) is very hard to simulate in test with small transactions that
don't spend much time in moving intents to regular db in the apply stage. To
ensure we test the case reliably, testIsolationRegressWithDelayedTxnApply() has
been added to TestPgIsolationRegress. It sets
TEST_inject_sleep_before_applying_intents_ms to ensure a transaction sleeps in
the apply stage so that case (2) is hit with certainty.

Test Plan: ./yb_build.sh --java-test org.yb.pgsql.TestPgIsolationRegress#testIsolationRegress

Reviewers: mihnea, sergei, jason, dmitry

Reviewed By: dmitry

Subscribers: alex, rsami, jason, neil, yql

Differential Revision: https://phabricator.dev.yugabyte.com/D14362
pkj415 added a commit that referenced this issue Feb 8, 2022
…g java framework and fix rare SKIP LOCKED bugs

Summary:
Part 1
-------
Add java class org.yb.pgsql.TestPgIsolationRegress to support running tests in
src/test/isolation (similar to src/test/regress).

Enable tests skip-locked and skip-locked-2. Without a fix that is explained in
Part 2 below, the skip-locked test would fail rarely due a bug (also explained
in Part 2).

Two extra tests are added -

(1) yb-modification-followed-by-lock to test that a modification doesn't wait on
a lock that was held by a recently committed transaction. This test would have
failed rarely without the fix in part 2 due to the same bug that affects the
skip-locked test.

(2) yb-skip-locked-after-update to assert that a statement with SKIP LOCKED
actually skips locking a tuple that was concurrently modified by another
transaction. The test asserts the skip locked behaviour for three different
states of the concurrent transaction that modified the tuple: pending,
committed but not applied, committed and applied.

Right now, the assertions fail in the latter 2 cases where the transaction has
committed but not applied or has committed and applied. Part 3 explains the
reason for the bug along with the fix.

Part 2
-------

(1) In skip-locked test, the step s2b rarely results in a serialization error
in permutation [s1a s1b s2a s1c s2b s2c]. This happens if the intents of s1
haven't yet moved to regular db before conflict resolution of s2b starts. The
following steps in conflict resolution of s2b lead to a serialization error in
this scenario -
  (a) s2b sees a conflicting row lock intent in intents db that corresponds to
      transaction in s1 (in ReadIntentConflicts())
  (b) s2b finds s1's transaction to be committed and hence aborts itself (as
      part of CheckLocalCommits())

In step b), it is incorrect to abort if the conflicting intents of committed
transaction 1 were only corresponding to explicit lock and not any modification
to the regular db (which is true for statements s1a and s1b). Instead, s2b
should ignore such intents of transaction 1 since they are only explicit row
lock intents and locks are released post commit. The fix adds logic to ignore
such conflicting intents of committed transactions which correspond to only
explicit row lock intents (i.e., those which can only block but not change any
data). This is achieved by usage of a new field "all_lock_only_conflicts" in
conflict_resolution.cc

(2) In cases other than (1) i.e., if the transaction in s1 has already been
applied before conflict resolution for s2b is done, there is no serialization
error. This is because no intents are moved to regular db as part of the "apply"
step of s1 (which in turn is because the intents are lock-only intents and not
a modification). And so, s2b doesn't see any conflicts in both intents db and
regular db.

This fix of ignoring explicit lock only intents of committed transactions solves
the rare issue mentioned in skip-locked test above which occurs if steps
described in (1) happen. Moreover, the fix also ensure that there is no
serialization error in the newly added test yb-modification-followed-by-lock (
the error would have occurred in this test as well without the fix).

Part 3
------
As mentioned earlier, SKIP LOCKED functionality fails in
yb-skip-locked-after-update if the concurrently modified tuple is part of a
transaction that has committed but not applied or has committed and applied. The
reason for this is:

(1) "committed and applied" case: currently, an explicit locking rpc with SKIP
LOCKED will abort if it finds a modified row in regular db with commit time
after read time. We should instead skip locking this row exactly as done in
CheckPriorityInternal() in D13352 i.e., by throwing
TransactionErrorCode::kSkipLocking if the policy is WAIT_SKIP. We check for
conflicts with regular db in StrongConflictChecker::Check() and the fix is to
add the extra handling here.

(2) "committed but not applied" case: even if we don't find conflicting
modifications in regular db that would lead to aborting or skipping the tuple
(i.e., case (1)), we might still find committed conflicting modifications in
intents db. This might happen if a transaction that modified the tuple has
committed but hasn't been applied yet (i.e., intents haven't moved to regular db
yet). Such conflicts are handled in CheckConflictWithCommitted(). Right now we
just abort here or ignore the conflict if all_lock_only_conflicts=true after fix
in Part 1. An extra check similar to (1) is needed here.

These two bugs arise because we missed to throw the
TransactionErrorCode::kSkipLocking error if the policy is WAIT_SKIP at the two
places mentioned above apart from CheckPriorityInternal() in D13352 (the diff
that added SKIP LOCKED functionality).

Note that case (2) is very hard to simulate in test with small transactions that
don't spend much time in moving intents to regular db in the apply stage. To
ensure we test the case reliably, testIsolationRegressWithDelayedTxnApply() has
been added to TestPgIsolationRegress. It sets
TEST_inject_sleep_before_applying_intents_ms to ensure a transaction sleeps in
the apply stage so that case (2) is hit with certainty.

Original commit: 85531fb, D14362

Test Plan:
Jenkins: rebase: 2.12
./yb_build.sh --java-test org.yb.pgsql.TestPgIsolationRegress#testIsolationRegress

Reviewers: mihnea, sergei, dmitry, jason

Reviewed By: jason

Subscribers: yql, neil, jason, rsami, alex

Differential Revision: https://phabricator.dev.yugabyte.com/D15283
jayant07-yb pushed a commit to jayant07-yb/yugabyte-db that referenced this issue Mar 8, 2022
…ramework and fix rare SKIP LOCKED bugs

Summary:
Part 1
-------
Add java class org.yb.pgsql.TestPgIsolationRegress to support running tests in
src/test/isolation (similar to src/test/regress).

Enable tests skip-locked and skip-locked-2. Without a fix that is explained in
Part 2 below, the skip-locked test would fail rarely due a bug (also explained
in Part 2).

Two extra tests are added -

(1) yb-modification-followed-by-lock to test that a modification doesn't wait on
a lock that was held by a recently committed transaction. This test would have
failed rarely without the fix in part 2 due to the same bug that affects the
skip-locked test.

(2) yb-skip-locked-after-update to assert that a statement with SKIP LOCKED
actually skips locking a tuple that was concurrently modified by another
transaction. The test asserts the skip locked behaviour for three different
states of the concurrent transaction that modified the tuple: pending,
committed but not applied, committed and applied.

Right now, the assertions fail in the latter 2 cases where the transaction has
committed but not applied or has committed and applied. Part 3 explains the
reason for the bug along with the fix.

Part 2
-------

(1) In skip-locked test, the step s2b rarely results in a serialization error
in permutation [s1a s1b s2a s1c s2b s2c]. This happens if the intents of s1
haven't yet moved to regular db before conflict resolution of s2b starts. The
following steps in conflict resolution of s2b lead to a serialization error in
this scenario -
  (a) s2b sees a conflicting row lock intent in intents db that corresponds to
      transaction in s1 (in ReadIntentConflicts())
  (b) s2b finds s1's transaction to be committed and hence aborts itself (as
      part of CheckLocalCommits())

In step b), it is incorrect to abort if the conflicting intents of committed
transaction 1 were only corresponding to explicit lock and not any modification
to the regular db (which is true for statements s1a and s1b). Instead, s2b
should ignore such intents of transaction 1 since they are only explicit row
lock intents and locks are released post commit. The fix adds logic to ignore
such conflicting intents of committed transactions which correspond to only
explicit row lock intents (i.e., those which can only block but not change any
data). This is achieved by usage of a new field "all_lock_only_conflicts" in
conflict_resolution.cc

(2) In cases other than (1) i.e., if the transaction in s1 has already been
applied before conflict resolution for s2b is done, there is no serialization
error. This is because no intents are moved to regular db as part of the "apply"
step of s1 (which in turn is because the intents are lock-only intents and not
a modification). And so, s2b doesn't see any conflicts in both intents db and
regular db.

This fix of ignoring explicit lock only intents of committed transactions solves
the rare issue mentioned in skip-locked test above which occurs if steps
described in (1) happen. Moreover, the fix also ensure that there is no
serialization error in the newly added test yb-modification-followed-by-lock (
the error would have occurred in this test as well without the fix).

Part 3
------
As mentioned earlier, SKIP LOCKED functionality fails in
yb-skip-locked-after-update if the concurrently modified tuple is part of a
transaction that has committed but not applied or has committed and applied. The
reason for this is:

(1) "committed and applied" case: currently, an explicit locking rpc with SKIP
LOCKED will abort if it finds a modified row in regular db with commit time
after read time. We should instead skip locking this row exactly as done in
CheckPriorityInternal() in D13352 i.e., by throwing
TransactionErrorCode::kSkipLocking if the policy is WAIT_SKIP. We check for
conflicts with regular db in StrongConflictChecker::Check() and the fix is to
add the extra handling here.

(2) "committed but not applied" case: even if we don't find conflicting
modifications in regular db that would lead to aborting or skipping the tuple
(i.e., case (1)), we might still find committed conflicting modifications in
intents db. This might happen if a transaction that modified the tuple has
committed but hasn't been applied yet (i.e., intents haven't moved to regular db
yet). Such conflicts are handled in CheckConflictWithCommitted(). Right now we
just abort here or ignore the conflict if all_lock_only_conflicts=true after fix
in Part 1. An extra check similar to (1) is needed here.

These two bugs arise because we missed to throw the
TransactionErrorCode::kSkipLocking error if the policy is WAIT_SKIP at the two
places mentioned above apart from CheckPriorityInternal() in D13352 (the diff
that added SKIP LOCKED functionality).

Note that case (2) is very hard to simulate in test with small transactions that
don't spend much time in moving intents to regular db in the apply stage. To
ensure we test the case reliably, testIsolationRegressWithDelayedTxnApply() has
been added to TestPgIsolationRegress. It sets
TEST_inject_sleep_before_applying_intents_ms to ensure a transaction sleeps in
the apply stage so that case (2) is hit with certainty.

Test Plan: ./yb_build.sh --java-test org.yb.pgsql.TestPgIsolationRegress#testIsolationRegress

Reviewers: mihnea, sergei, jason, dmitry

Reviewed By: dmitry

Subscribers: alex, rsami, jason, neil, yql

Differential Revision: https://phabricator.dev.yugabyte.com/D14362
@pkj415 pkj415 changed the title [YSQL] Support SKIP LOCKED locking option in SELECT ... FOR UPDATE statement [YSQL] Support SKIP LOCKED for explicit row-level locks in REPEATABLE READ isolation level Aug 9, 2022
@yugabyte-ci yugabyte-ci added kind/bug This issue is a bug priority/medium Medium priority issue labels Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ysql Yugabyte SQL (YSQL) community/request Issues created by external users kind/bug This issue is a bug priority/medium Medium priority issue
Projects
None yet
Development

No branches or pull requests

9 participants