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

[YSQL] Explicit locking fails with "Not implemented" error for a specific corner case #12004

Closed
pkj415 opened this issue Apr 6, 2022 · 0 comments
Closed

[YSQL] Explicit locking fails with "Not implemented" error for a specific corner case #12004

pkj415 opened this issue Apr 6, 2022 · 0 comments
Assignees
@pkj415
Labels
area/ysql Yugabyte SQL (YSQL) kind/bug This issue is a bug priority/medium Medium priority issue

Comments

@pkj415
Copy link
Contributor

pkj415 commented Apr 6, 2022 •
edited by yugabyte-ci
Loading

Jira Link: DB-523

Description

The below snippet explains the issue -

create table test2 (k int primary key, v int);
insert into test values (1, 1);

yugabyte=# begin transaction isolation level read committed;
BEGIN
yugabyte=# select * from test where k=1 for update;
ERROR:  Not implemented: Read request with row mark types must be part of a transaction: OPERATION_NOT_SUPPORTED
yugabyte=#

NOTE: "begin transaction isolation level read committed;" uses repeatable read isolation since yb_enable_read_committed_isolation is false by default.

The cause might be "isolation_level_ = IsolationLevel::NON_TRANSACTIONAL;" in PgTxnManager::ResetTxnAndSession() in the refactor commit c5f5125
@pkj415 pkj415 added area/ysql Yugabyte SQL (YSQL) status/awaiting-triage Issue awaiting triage labels Apr 6, 2022
@pkj415 pkj415 self-assigned this Apr 6, 2022
@kmuthukk kmuthukk mentioned this issue May 9, 2022
Closed
pkj415 added a commit to pkj415/yugabyte-db that referenced this issue May 18, 2022
@pkj415
[yugabyte#12004] YSQL: Explicit locking statement doesn't create a tr…
841cc97 
…ansaction when using READ COMMITTED in YSQL that maps to REPEATABLE READ

Summary:
If yb_enable_read_committed_isolation=false, a "READ COMMITTED" transaction in
YSQL maps to SNAPSHOT ISOLATION in docdb. Currently, a user running with
yb_enable_read_committed_isolation=false is not able to issue a "select ... for
update" as the first statement, if a txn was started with "begin transaction
isolation level read committed;".

This is because no transaction seems to be created if three conditions occur -

(i) "yb_enable_read_committed_isolation=false",
(ii) a READ COMMITTED txn is started in YSQL, and
(iii) the first statement is a "SELECT ... FOR UPDATE" or any other explicit
locking statement.

```
yugabyte=# begin transaction isolation level read committed;
BEGIN
yugabyte=# select * from test where k=1 for update;
ERROR:  Not implemented: Read request with row mark types must be part of a transaction: OPERATION_NOT_SUPPORTED
yugabyte=#
```

There are 2 bugs in code that cause this (only if present together) - one is a
regression which is the cause, another is a bug in read committed implementation
which, if corrected, masks the regression. More details -

(1) The regression:

As part of D15991, RunHelper::Apply() in pg_session.cc doesn't set read_only to
false for a "SELECT ... FOR UPDATE" type of statement if the pg_isolation_level
is READ COMMITTED. Before this diff, read_only was set to false for explicit
locking statements always irrespective of the isolation level. Not setting
read_only to false, causes no transaction to be started if that is the first
operation in the YSQL transaction block.

(2) Day-0 bug, which if fixed separately, it would mask bug (1):

In xact.c, we set pg_isolation_level to XactIsoLevel. And, as part of D15383, we
have extra logic to set it to XACT_REPEATABLE_READ if XactIsoLevel is
XACT_READ_COMMITTED and read committed mode is on. This is the place where READ
COMMITTED isolation maps to REPEATABLE READ if
yb_enable_read_committed_isolation=false (i.e., the old behaviour when read
committed isolation was not implemented).

But, we forgot to add the same logic before calling
YBCPgSetTransactionIsolationLevel() in assign_XactIsoLevel(). This diff adds
that. Not adding that logic causes "READ COMMITTED" isolation in ysqlsh to use
READ COMMITTED in doc db even if yb_enable_read_committed_isolation=false.

Bug (1) is a regression (D15991) and bug (2) was part of the READ COMMITTED
isolation implementation (D15383). Bug (2) along with bug (1) results in the bad
behaviour above.

NOTE: in a later diff, I will rename pg_isolation_level_ in pg_txn_manager.cc to
mapped_pg_isolation_level_ to reflect the true meaning.

Test Plan: ./yb_build.sh --java-test org.yb.pgsql.TestPgTransactions#testMiscellaneous

Reviewers: rsami, dmitry, mtakahara

Subscribers: yql

Differential Revision: https://phabricator.dev.yugabyte.com/D16866
pkj415 added a commit that referenced this issue May 19, 2022
@pkj415
[#12004] YSQL: Explicit locking statement doesn't create a transactio…
331a48b 
…n when using READ COMMITTED in YSQL that maps to REPEATABLE READ

Summary:
If yb_enable_read_committed_isolation=false, a "READ COMMITTED" transaction in
YSQL maps to SNAPSHOT ISOLATION in docdb. Currently, a user running with
yb_enable_read_committed_isolation=false is not able to issue a "select ... for
update" as the first statement, if a txn was started with "begin transaction
isolation level read committed;".

This is because no transaction seems to be created if three conditions occur -

(i) "yb_enable_read_committed_isolation=false",
(ii) a READ COMMITTED txn is started in YSQL, and
(iii) the first statement is a "SELECT ... FOR UPDATE" or any other explicit
locking statement.

```
yugabyte=# begin transaction isolation level read committed;
BEGIN
yugabyte=# select * from test where k=1 for update;
ERROR:  Not implemented: Read request with row mark types must be part of a transaction: OPERATION_NOT_SUPPORTED
yugabyte=#
```

There are 2 bugs in code that cause this (only if present together) - one is a
regression which is the cause, another is a bug in read committed implementation
which, if corrected, masks the regression. More details -

(1) The regression:

As part of D15991, RunHelper::Apply() in pg_session.cc doesn't set read_only to
false for a "SELECT ... FOR UPDATE" type of statement if the pg_isolation_level
is READ COMMITTED. Before this diff, read_only was set to false for explicit
locking statements always irrespective of the isolation level. Not setting
read_only to false, causes no transaction to be started if that is the first
operation in the YSQL transaction block.

(2) Day-0 bug, which if fixed separately, it would mask bug (1):

In xact.c, we set pg_isolation_level to XactIsoLevel. And, as part of D15383, we
have extra logic to set it to XACT_REPEATABLE_READ if XactIsoLevel is
XACT_READ_COMMITTED and read committed mode is on. This is the place where READ
COMMITTED isolation maps to REPEATABLE READ if
yb_enable_read_committed_isolation=false (i.e., the old behaviour when read
committed isolation was not implemented).

But, we forgot to add the same logic before calling
YBCPgSetTransactionIsolationLevel() in assign_XactIsoLevel(). This diff adds
that. Not adding that logic causes "READ COMMITTED" isolation in ysqlsh to use
READ COMMITTED in doc db even if yb_enable_read_committed_isolation=false.

Bug (1) is a regression (D15991) and bug (2) was part of the READ COMMITTED
isolation implementation (D15383). Bug (2) along with bug (1) results in the bad
behaviour above.

Test Plan: ./yb_build.sh --java-test org.yb.pgsql.TestPgTransactions#testMiscellaneous

Reviewers: rsami, mtakahara, dmitry

Reviewed By: dmitry

Subscribers: yql

Differential Revision: https://phabricator.dev.yugabyte.com/D16866
pkj415 added a commit that referenced this issue May 24, 2022
@pkj415
[BACKPORT 2.12] [#12004] YSQL: Explicit locking statement doesn't cre…
2a8e6f0 
…ate a transaction when using READ COMMITTED in YSQL that maps to REPEATABLE READ

Summary:
This backport to 2.12 fixes two bugs in code as mentioned in the original commit
summary below. The first bug which is a regression due to D15991 isn't valid for
2.12 since D15991 isn't backported to 2.12. The second fix is still valid. Also,
the changes that fix the first bug are still cherry-picked since they don't
change any behaviour in 2.12 and will moreover ensure safety by throwing a merge
conflict when an attempt to cherry-pick D15991 to 2.12 is made.

If yb_enable_read_committed_isolation=false, a "READ COMMITTED" transaction in
YSQL maps to SNAPSHOT ISOLATION in docdb. Currently, a user running with
yb_enable_read_committed_isolation=false is not able to issue a "select ... for
update" as the first statement, if a txn was started with "begin transaction
isolation level read committed;".

This is because no transaction seems to be created if three conditions occur -

(i) "yb_enable_read_committed_isolation=false",
(ii) a READ COMMITTED txn is started in YSQL, and
(iii) the first statement is a "SELECT ... FOR UPDATE" or any other explicit
locking statement.

```
yugabyte=# begin transaction isolation level read committed;
BEGIN
yugabyte=# select * from test where k=1 for update;
ERROR:  Not implemented: Read request with row mark types must be part of a transaction: OPERATION_NOT_SUPPORTED
yugabyte=#
```

There are 2 bugs in code that cause this (only if present together) - one is a
regression which is the cause, another is a bug in read committed implementation
which, if corrected, masks the regression. More details -

(1) The regression:

As part of D15991, RunHelper::Apply() in pg_session.cc doesn't set read_only to
false for a "SELECT ... FOR UPDATE" type of statement if the pg_isolation_level
is READ COMMITTED. Before this diff, read_only was set to false for explicit
locking statements always irrespective of the isolation level. Not setting
read_only to false, causes no transaction to be started if that is the first
operation in the YSQL transaction block.

(2) Day-0 bug, which if fixed separately, it would mask bug (1):

In xact.c, we set pg_isolation_level to XactIsoLevel. And, as part of D15383, we
have extra logic to set it to XACT_REPEATABLE_READ if XactIsoLevel is
XACT_READ_COMMITTED and read committed mode is on. This is the place where READ
COMMITTED isolation maps to REPEATABLE READ if
yb_enable_read_committed_isolation=false (i.e., the old behaviour when read
committed isolation was not implemented).

But, we forgot to add the same logic before calling
YBCPgSetTransactionIsolationLevel() in assign_XactIsoLevel(). This diff adds
that. Not adding that logic causes "READ COMMITTED" isolation in ysqlsh to use
READ COMMITTED in doc db even if yb_enable_read_committed_isolation=false.

Bug (1) is a regression (D15991) and bug (2) was part of the READ COMMITTED
isolation implementation (D15383). Bug (2) along with bug (1) results in the bad
behaviour above.

Original commit: 331a48b / D16866

Test Plan:
Jenkins: urgent
./yb_build.sh --java-test org.yb.pgsql.TestPgTransactions#testMiscellaneous

Reviewers: dmitry, mtakahara

Reviewed By: mtakahara

Subscribers: yql

Differential Revision: https://phabricator.dev.yugabyte.com/D17146
@pkj415 pkj415 closed this as completed May 24, 2022
@yugabyte-ci yugabyte-ci added kind/bug This issue is a bug priority/medium Medium priority issue labels May 24, 2022
pkj415 added a commit that referenced this issue May 27, 2022
@pkj415
[BACKPORT 2.14][#12004] YSQL: Explicit locking statement doesn't crea…
acc2e7b 
…te a transaction when using READ COMMITTED in YSQL that maps to REPEATABLE READ

Summary:
If yb_enable_read_committed_isolation=false, a "READ COMMITTED" transaction in
YSQL maps to SNAPSHOT ISOLATION in docdb. Currently, a user running with
yb_enable_read_committed_isolation=false is not able to issue a "select ... for
update" as the first statement, if a txn was started with "begin transaction
isolation level read committed;".

This is because no transaction seems to be created if three conditions occur -

(i) "yb_enable_read_committed_isolation=false",
(ii) a READ COMMITTED txn is started in YSQL, and
(iii) the first statement is a "SELECT ... FOR UPDATE" or any other explicit
locking statement.

```
yugabyte=# begin transaction isolation level read committed;
BEGIN
yugabyte=# select * from test where k=1 for update;
ERROR:  Not implemented: Read request with row mark types must be part of a transaction: OPERATION_NOT_SUPPORTED
yugabyte=#
```

There are 2 bugs in code that cause this (only if present together) - one is a
regression which is the cause, another is a bug in read committed implementation
which, if corrected, masks the regression. More details -

(1) The regression:

As part of D15991, RunHelper::Apply() in pg_session.cc doesn't set read_only to
false for a "SELECT ... FOR UPDATE" type of statement if the pg_isolation_level
is READ COMMITTED. Before this diff, read_only was set to false for explicit
locking statements always irrespective of the isolation level. Not setting
read_only to false, causes no transaction to be started if that is the first
operation in the YSQL transaction block.

(2) Day-0 bug, which if fixed separately, it would mask bug (1):

In xact.c, we set pg_isolation_level to XactIsoLevel. And, as part of D15383, we
have extra logic to set it to XACT_REPEATABLE_READ if XactIsoLevel is
XACT_READ_COMMITTED and read committed mode is on. This is the place where READ
COMMITTED isolation maps to REPEATABLE READ if
yb_enable_read_committed_isolation=false (i.e., the old behaviour when read
committed isolation was not implemented).

But, we forgot to add the same logic before calling
YBCPgSetTransactionIsolationLevel() in assign_XactIsoLevel(). This diff adds
that. Not adding that logic causes "READ COMMITTED" isolation in ysqlsh to use
READ COMMITTED in doc db even if yb_enable_read_committed_isolation=false.

Bug (1) is a regression (D15991) and bug (2) was part of the READ COMMITTED
isolation implementation (D15383). Bug (2) along with bug (1) results in the bad
behaviour above.

Original commit: 331a48b / D16866

Test Plan: ./yb_build.sh --java-test org.yb.pgsql.TestPgTransactions#testMiscellaneous

Reviewers: dmitry, mtakahara

Reviewed By: mtakahara

Subscribers: yql

Differential Revision: https://phabricator.dev.yugabyte.com/D17210
pkj415 added a commit to pkj415/yugabyte-db that referenced this issue May 31, 2022
@pkj415
[yugabyte#12004] YSQL: Explicit locking statement doesn't create a tr…
4857004 
…ansaction when using READ COMMITTED in YSQL that maps to REPEATABLE READ

Summary:
If yb_enable_read_committed_isolation=false, a "READ COMMITTED" transaction in
YSQL maps to SNAPSHOT ISOLATION in docdb. Currently, a user running with
yb_enable_read_committed_isolation=false is not able to issue a "select ... for
update" as the first statement, if a txn was started with "begin transaction
isolation level read committed;".

This is because no transaction seems to be created if three conditions occur -

(i) "yb_enable_read_committed_isolation=false",
(ii) a READ COMMITTED txn is started in YSQL, and
(iii) the first statement is a "SELECT ... FOR UPDATE" or any other explicit
locking statement.

```
yugabyte=# begin transaction isolation level read committed;
BEGIN
yugabyte=# select * from test where k=1 for update;
ERROR:  Not implemented: Read request with row mark types must be part of a transaction: OPERATION_NOT_SUPPORTED
yugabyte=#
```

There are 2 bugs in code that cause this (only if present together) - one is a
regression which is the cause, another is a bug in read committed implementation
which, if corrected, masks the regression. More details -

(1) The regression:

As part of D15991, RunHelper::Apply() in pg_session.cc doesn't set read_only to
false for a "SELECT ... FOR UPDATE" type of statement if the pg_isolation_level
is READ COMMITTED. Before this diff, read_only was set to false for explicit
locking statements always irrespective of the isolation level. Not setting
read_only to false, causes no transaction to be started if that is the first
operation in the YSQL transaction block.

(2) Day-0 bug, which if fixed separately, it would mask bug (1):

In xact.c, we set pg_isolation_level to XactIsoLevel. And, as part of D15383, we
have extra logic to set it to XACT_REPEATABLE_READ if XactIsoLevel is
XACT_READ_COMMITTED and read committed mode is on. This is the place where READ
COMMITTED isolation maps to REPEATABLE READ if
yb_enable_read_committed_isolation=false (i.e., the old behaviour when read
committed isolation was not implemented).

But, we forgot to add the same logic before calling
YBCPgSetTransactionIsolationLevel() in assign_XactIsoLevel(). This diff adds
that. Not adding that logic causes "READ COMMITTED" isolation in ysqlsh to use
READ COMMITTED in doc db even if yb_enable_read_committed_isolation=false.

Bug (1) is a regression (D15991) and bug (2) was part of the READ COMMITTED
isolation implementation (D15383). Bug (2) along with bug (1) results in the bad
behaviour above.

NOTE: in a later diff, I will rename pg_isolation_level_ in pg_txn_manager.cc to
mapped_pg_isolation_level_ to reflect the true meaning.

Test Plan: ./yb_build.sh --java-test org.yb.pgsql.TestPgTransactions#testMiscellaneous

Reviewers: rsami, dmitry, mtakahara

Subscribers: yql

Differential Revision: https://phabricator.dev.yugabyte.com/D16866
@yugabyte-ci yugabyte-ci removed the status/awaiting-triage Issue awaiting triage label Jul 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@pkj415 pkj415

Labels
area/ysql Yugabyte SQL (YSQL) kind/bug This issue is a bug priority/medium Medium priority issue
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@pkj415 @yugabyte-ci