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] Allow client to set isolation level after "begin;" when yb_enable_read_committed_isolation=true #12494

Closed
pkj415 opened this issue May 13, 2022 · 1 comment
Closed

[YSQL] Allow client to set isolation level after "begin;" when yb_enable_read_committed_isolation=true #12494

pkj415 opened this issue May 13, 2022 · 1 comment
Assignees
@pkj415
Labels
area/ysql Yugabyte SQL (YSQL) kind/enhancement This is an enhancement of an existing feature pg-compatibility Label for issues that result in differences b/w YSQL and Pg semantics priority/low Low priority

Comments

@pkj415
Copy link
Contributor

pkj415 commented May 13, 2022 •
edited by jira bot
Loading

Jira Link: [DB-328](https://yugabyte.atlassian.net/browse/DB-328)

Description

As seen in #12251, a "SET TRANSACTION ISOLATION LEVEL ..." fails after a "BEGIN;". This is because, with yb_enable_read_committed_isolation=true, a sub-transaction is created right before executing the "SET TRANSACTION ISOLATION LEVEL ..." statement (which is done to support semantics of READ COMMITTED isolation -- which is the default acting isolation after "BEGIN;"). And we can't set the isolation level to something else after a sub-transaction has been created.
@pkj415 pkj415 added area/ysql Yugabyte SQL (YSQL) status/awaiting-triage Issue awaiting triage labels May 13, 2022
@pkj415 pkj415 self-assigned this May 13, 2022
@yugabyte-ci yugabyte-ci added kind/bug This issue is a bug priority/medium Medium priority issue labels May 13, 2022
@pkj415 pkj415 removed kind/bug This issue is a bug priority/medium Medium priority issue status/awaiting-triage Issue awaiting triage labels May 13, 2022
This was referenced May 13, 2022
Closed
Open
@yugabyte-ci yugabyte-ci added kind/bug This issue is a bug priority/medium Medium priority issue labels May 13, 2022
@pkj415 pkj415 added the pg-compatibility Label for issues that result in differences b/w YSQL and Pg semantics label May 24, 2022
@FranckPachot FranckPachot mentioned this issue Jun 17, 2022
Closed
@pkj415 pkj415 mentioned this issue Aug 9, 2022
Open
@yugabyte-ci yugabyte-ci added kind/enhancement This is an enhancement of an existing feature and removed kind/bug This issue is a bug labels Sep 5, 2022
@polarweasel
Copy link
Contributor

Before you close this one @pkj415 please make sure to remove the note in the docs on the Read Committed architecture page.

pkj415 added a commit that referenced this issue Oct 18, 2022
@pkj415
[#14202] YSQL: Avoid rpc from YSQL to local tserver when switching su…
43900f9 
…b-txns

Summary:
Currently, YSQL sends the PgSetActiveSubTransactionRequestPB rpc to the local
tserver process' Pg client (pg_client_session.cc) when it wants to switch the
active sub transaction id. YSQL will switch the active sub transaction id for
any of following - user savepoint operations, an internal savepoint created for
exception handling or an internal savepoint created before execution of a
statement in READ COMMITTED isolation level as explained in 9f2cc7f.

The PgSetActiveSubTransactionRequestPB rpc was added in c5f5125 as part
of larger PgClient changes. After the change, YSQL sends all rpcs for DMLs to
the PgClientSession on the local tserver process. Distributed transaction
creations and management is handled in the PgClientSession and is not visible
to the YSQL backends. Before this change, YSQL backends managed the lifecycle
of distributed transactions and sub-txn operations were local i.e., any rpcs.

However, a separate rpc is not required for setting the active sub transaction id.
The active sub transaction id can be piggybacked in each PgPerformRequestPB
through the PgPerformOptionsPB sub-field and the Pg client session on the
tserver can change the active sub transaction id before handling the
PgPerformRequestPB.

Removal of this rpc is required as a precursor to solving #12494. As part of #12494,
we can see that postgres allows changing the transaction isolation level after a "BEGIN"
statement. In YSQL, if the "BEGIN" statement starts in READ COMMITTED isolation,
every future statement will acquire an internal savepoint before executing the
statement (refer 9f2cc7f). Without this diff, the internal savepoint creation
results in a PgSetActiveSubTransactionRequestPB rpc which inturn starts a
distributed transaction. This leads to the setting the isolation level in stone,
and not allowing any future modification of the transaction isolation (via the
"SET TRANSACTION ISOLATION LEVEL" statement) even if no other queries have been
executed. It results in the "Attempt to change isolation level of running
transaction..." error message in pg_client_session.cc

NOTE: The RollbackToSubTransaction rpc for rolling back to a sub transaction id stays
and will remain synchronous.

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

Reviewers: mtakahara, rsami

Reviewed By: mtakahara, rsami

Subscribers: zyu, yql, bogdan

Differential Revision: https://phabricator.dev.yugabyte.com/D19902
pkj415 added a commit to pkj415/yugabyte-db that referenced this issue Mar 16, 2023
@pkj415
[yugabyte#12494] YSQL: Allow changing transaction characteristics in …
61d57f9 
…read committed isolation

Summary:
Currently in YSQL, a user isn't allowed to change the transaction isolation level
after starting a READ COMMITTED transaction. This is because of the following
reason:

(1) In PostgreSQL, the transaction isolation level can be changed only if no
sub-transactions have been created. If a sub-transaction is created the user
faces this error - "ERROR:  SET TRANSACTION ISOLATION LEVEL must not be called
in a subtransaction".

(2) In YSQL's read committed isolation, an internal saevpoint is created before
execution of each statement. This is an implementation detail and is required to
be able to undo work and restart a statement in case of serialization and
read restart errors. And each savepoint creates an internal sub-transaction. Hence,
when starting a transaction in READ COMMITTED isolation and trying to change
the isolation level using "SET TRANSACTION ISOLATION LEVEL ...", an internal
sub-transaction is started before changing the isolation level. This leads
to the above error. The same applies for "SET TRANSACTION DEFERRABLE" and
"SET TRANSACTION READ WRITE".

This diff fixes the issues by allowing change of transaction characteristics i.e.,
isolation, defferable, read-write if all sub-transactions are only internal
read committed sub-transactions and no statement/query was executed yet.

NOTE: The correct way to do this would have been to not start an internal
sub-transaction in the first place if a "SET TRANSACTION ..." statement is
encountered. However, that isn't possible without major code changes since
the internal sub-transaction is taken before parsing the statement.

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

Reviewers: mtakahara

Subscribers: yql

Differential Revision: https://phabricator.dev.yugabyte.com/D20688
@yugabyte-ci yugabyte-ci added priority/low Low priority kind/bug This issue is a bug and removed priority/medium Medium priority issue kind/enhancement This is an enhancement of an existing feature labels Aug 31, 2023
@yugabyte-ci yugabyte-ci assigned tvesely and unassigned pkj415 Aug 31, 2023
@yugabyte-ci yugabyte-ci added priority/medium Medium priority issue and removed priority/low Low priority labels Sep 2, 2023
@yugabyte-ci yugabyte-ci added kind/enhancement This is an enhancement of an existing feature and removed kind/bug This issue is a bug labels Sep 3, 2023
@pkj415 pkj415 mentioned this issue Sep 5, 2023
Open
@pkj415 pkj415 assigned pkj415 and unassigned tvesely Dec 11, 2023
@yugabyte-ci yugabyte-ci added priority/low Low priority and removed priority/medium Medium priority issue labels Jan 17, 2024
arpang added a commit that referenced this issue Feb 19, 2024
@arpang
[#21098] YSQL: Use default isolation level for trigger regression tests
3769617 
Summary:
These tests have `SET SESSION CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL SERIALIZABLE` set at the start for historical reasons that are no longer valid. Remove it so that these tests execute in the default isolation level.

With this change,
```
BEGIN;
SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
```

throws `ERROR: SET TRANSACTION ISOLATION LEVEL must not be called in a subtransaction in YB` (GH #12494).

To workaround it, convert it to
`BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;`

Test Plan: ./yb_build.sh --java-test org.yb.pgsql.TestPgRegressTrigger

Reviewers: pjain, mihnea

Reviewed By: pjain

Subscribers: yql

Differential Revision: https://phorge.dev.yugabyte.com/D32516
pkj415 added a commit that referenced this issue Jul 9, 2024
@pkj415
[#12494] YSQL: Allow changing transaction characteristics in read com…
3494424 
…mitted isolation

Summary:
Currently in YSQL, a user isn't allowed to change the transaction isolation level
after starting a READ COMMITTED transaction. This is because of the following
reason:

(1) In PostgreSQL, the transaction isolation level can be changed only if no
sub-transactions have been created. If a sub-transaction is created the user
faces this error - `ERROR:  SET TRANSACTION ISOLATION LEVEL must not be called
in a subtransaction`.

(2) In YSQL's read committed isolation, an internal savepoint is created before
execution of each statement. This is an implementation detail and is required to
be able to undo work and restart a statement in case of serialization and
read restart errors. And each savepoint creates an internal sub-transaction. Hence,
when starting a transaction in READ COMMITTED isolation and trying to change
the isolation level using `SET TRANSACTION ISOLATION LEVEL ...` an internal
sub-transaction is started before changing the isolation level. This leads
to the above error. The same applies for `SET TRANSACTION DEFERRABLE` and
`SET TRANSACTION READ WRITE`.

This diff fixes the issues by allowing change of transaction characteristics i.e.,
isolation, deferrable, read-write if all sub-transactions are only internal
read committed sub-transactions and no statement/query was executed yet.

NOTE: This is a workaround. The correct way to do this would have been to
not start an internal sub-transaction in the first place if a
`SET TRANSACTION ...` statement is encountered. However, that isn't possible
without major code changes since the internal sub-transaction is taken before
parsing the statement. Moreover, once we improve the READ COMMITTED
isolation implementation as part of GitHub issue ##11572, internal
sub-transactions will not be needed and this problem will be solved without
this workaround.
Jira: [DB-328]

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

Reviewers: mtakahara

Reviewed By: mtakahara

Subscribers: amitanand, yql

Differential Revision: https://phorge.dev.yugabyte.com/D20688
jasonyb pushed a commit that referenced this issue Jul 11, 2024
@yugabyte-ci @jaki
[BACKPORT pg15-cherrypicks] all: Bulk port from master - 50
6b1ce69 
Summary:
 facf639 [PLAT-14510] Fix typo in drop table success notification message
 9363f0b [Docs] Upgrade Go, Hugo, Docsy and Node dependencies (#22646)
 Excluded: 3494424 [#12494] YSQL: Allow changing transaction characteristics in read committed isolation
 9877fe3 [#22441] docdb: Change clone op to (almost) never fail to apply
 Excluded: dbecdc1 [PLAT-14577] Force a refetch of xCluster config when opening table operation modals
 3a9b630 [#23159] xCluster: Fix mac test failures due to XClusterClient refactoring
 54fd008 Update Current roadmap in README.md
 56ee972 [PLAT-12736] Enable systemd with v2 api
 9de2593 [docs] Fix tablet splitting flag default values and add max_create_tablets_per_ts
 Excluded: 2a56a86 [PLAT-14072] enable DB scoped switchover
 b0ee793 [#22929] YSQL: Fix Server's System Clock in XLogData in Logical Replication
 a484826 [PLAT-13893] added auto create provider via YNP in case not exist
 a5ea488 [#23166] build: don't run python checks on all third-party-extensions
 76bdd1c [PLAT-14609] Fix log regex for YBC support bundle component to match log files with timestamp-pid prefix
 425919e fix for an XSS vulnerability related to SVG’s onload attribute (#23167)

Test Plan: Jenkins: rebase: pg15-cherrypicks

Reviewers: jason, tfoucher

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D36502
pkj415 added a commit that referenced this issue Jul 16, 2024
@pkj415
[BACKPORT 2024.1][#12494] YSQL: Allow changing transaction characteri…
25fb8a5 
…stics in read committed isolation

Summary:
Currently in YSQL, a user isn't allowed to change the transaction isolation level
after starting a READ COMMITTED transaction. This is because of the following
reason:

(1) In PostgreSQL, the transaction isolation level can be changed only if no
sub-transactions have been created. If a sub-transaction is created the user
faces this error - `ERROR:  SET TRANSACTION ISOLATION LEVEL must not be called
in a subtransaction`.

(2) In YSQL's read committed isolation, an internal savepoint is created before
execution of each statement. This is an implementation detail and is required to
be able to undo work and restart a statement in case of serialization and
read restart errors. And each savepoint creates an internal sub-transaction. Hence,
when starting a transaction in READ COMMITTED isolation and trying to change
the isolation level using `SET TRANSACTION ISOLATION LEVEL ...` an internal
sub-transaction is started before changing the isolation level. This leads
to the above error. The same applies for `SET TRANSACTION DEFERRABLE` and
`SET TRANSACTION READ WRITE`.

This diff fixes the issues by allowing change of transaction characteristics i.e.,
isolation, deferrable, read-write if all sub-transactions are only internal
read committed sub-transactions and no statement/query was executed yet.

NOTE: This is a workaround. The correct way to do this would have been to
not start an internal sub-transaction in the first place if a
`SET TRANSACTION ...` statement is encountered. However, that isn't possible
without major code changes since the internal sub-transaction is taken before
parsing the statement. Moreover, once we improve the READ COMMITTED
isolation implementation as part of GitHub issue ##11572, internal
sub-transactions will not be needed and this problem will be solved without
this workaround.
Jira: [DB-328]

Original commit: 3494424 / D20688

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

Reviewers: mtakahara, amitanand

Reviewed By: mtakahara

Subscribers: yql

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D36581
pkj415 added a commit that referenced this issue Jul 16, 2024
@pkj415
[BACKPORT pg15-cherrypicks] merge: master branch commit '3494424f710b…
2f84486 
…9ce9d7ccfe61a57a61801084ce8d' into pg15

Summary:
Merge YB master commit '3494424f710b9ce9d7ccfe61a57a61801084ce8d' titled
```
[#12494] YSQL: Allow changing transaction characteristics in read committed isolation
```

Conflicts resolved:
- xact.c:
 - location: TopTransactionStateData

YB added a new field called `ybIsInternalRcSubTransaction` in the
struct TransactionStateData and the default for that field needs to be set
in TopTransactionStateData. In setting the default values for
TopTransactionStateData, PG 15 uses a different syntax. Also, PG15
removed explicitly setting the default for various fields starting from
"transaction id", "sub transaction id", ... all the way to "link to the
parent state block". This is probably because the default initialization
values of these fields are the same as those that were intended.

This diff also removes struct member names that were present as code comments
when assigning default values for TopTransactionStateData.

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

Reviewers: jason

Reviewed By: jason

Subscribers: yql

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D36634
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/enhancement This is an enhancement of an existing feature pg-compatibility Label for issues that result in differences b/w YSQL and Pg semantics priority/low Low priority
Projects
Wait-Queue Based Locking
Status: Done
Milestone
No milestone
Development

No branches or pull requests
4 participants
@polarweasel @pkj415 @yugabyte-ci @tvesely