Skip to content

Commit

Permalink
[#18638] YSQL: Manage ybDataSent and ybDataSentForCurrQuery properly …
Browse files Browse the repository at this point in the history
…in xact.c

Summary:
### Objective

Previously, we correctly initialized the `ybDataSentForCurrQuery` flag on the subtransaction with the parent's value. This is necessary to handle transparent restarts correctly since we do not wish to restart transparently after we already sent some data to the client in the current query.

Now, we also do this for the `ybDataSent` flag.

Additionally, in this revision, we also propagate the value of ybDataSentForCurrQuery or ybDataSent to the parent transaction from a child transaction.

### Impact

Propagating ybDataSent from a parent to a child transaction is required to be able to retry deadlock errors in RC isolation. This is because we will retry deadlock errors only for the first statement of a transaction unlike kConflict and kReadRestart which can be retried on any statement. For other statements in a transaction, we can't be sure if rolling back the statement would result in releasing the blocking locks before retrying, because prior statements could have acquired the blocking locks. A test case for the same will be added as part of #18616.

It wasn't possible to construct any test case that would fail if we didn't propagate ybDataSent and ybDataSentForCurrQuery from a child to a parent. Also, a test case wasn't possible for the existing propagation of ybDataSentForCurrQuery from a parent to child.

Subtransactions in Pg are used mostly for

- SAVEPOINTs
- EXCEPTION WHEN clauses in functions and procedures
- READ COMMITTED uses internal subtransactions for transparent retries

#### Propagation of `ybDataSent` to subtransactions

ybDataSent is currently used in RR and SR isolations. If not using RC, retries are already blocked if we are in a sub-transaction (see IsSubTransaction() check in yb_is_restart_possible). And for RC, we don't use the ybDataSent field. So, no test case is possible that can fail if we don't propagate ybDataSent from parent to child.

Once we start using ybDataSent in RC for blocking deadlock errors retries for non-first statements in a transaction block, a test case can be created which will fail without this propagation.

```lang=sql
BEGIN;
SAVEPOINT a;
SELECT ...; <--- read restart error never triggers a transparent restart
```

#### Retention of `ybDataSent` state after releasing subtransactions

We do not need to propagate the value of this flag to the parent transactions since

- for SAVEPOINTs: ybDataSent is always true after a RELEASE because the server sends output of the command to the client.
- for releasing subtransactions inside functions/procedures: The server does not send any data from within a function or a procedure. The intermediate data used as return value is stored in a temporary table instead.

```lang=sql,name=SAVEPOINTs
BEGIN;
SAVEPOINT a;
SELECT * FROM long_table;      <---- Marks ybDataSent on the subtransaction
RELEASE SAVEPOINT a;            <---- Marks ybDataSent on the top level transaction anyway (even if we do not propagate the flag from the child)
SELECT * FROM short_table;    <---- A read restart error does not trigger a transparent read restart
```

Caveat: We can still encounter issues in batched execution mode containing a RELEASE SAVEPOINT statement since the responses are expected only at the end of the batch. However, this requries a SELECT statement to be part of a batch, but that isn't possible with the pgJDBC driver.

In RC, child transactions are never released. If rolled back, there is no need to propagate the value of ybDataSent.

```lang=sql,name=FUNCTIONs
CREATE FUNCTION foo() ...
BEGIN                                         <-------- start a subtransaction
RETURN QUERY SELECT * FROM long_table;           <-------- ybDataSent never marked since the data is stored in temporary table before sending it to the client
EXCEPTION WHEN ...
END;
```

#### Retention of `ybDataSentForCurrQuery` state after releasing subtransaction

We do not need to propagate the value of this flag to the parent transactions since

- RELEASE SAVEPOINT is a statement/query independent of other queries and `ybDataSentForCurrQuery` is only relevant for individual queries.
- The server never writes to the client from within functions or procedures.
Jira: DB-7563

Test Plan: No test plan since we couldn't identify any situation that fails without this diff.

Reviewers: smishra, pjain

Reviewed By: pjain

Subscribers: yql

Differential Revision: https://phorge.dev.yugabyte.com/D32368
  • Loading branch information
pao214 committed Mar 22, 2024
1 parent 8487e59 commit 1c8a47e
Showing 1 changed file with 6 additions and 0 deletions.
6 changes: 6 additions & 0 deletions src/postgres/src/backend/access/transam/xact.c
Original file line number Diff line number Diff line change
Expand Up @@ -5323,6 +5323,7 @@ PushTransaction(void)
s->prevXactReadOnly = XactReadOnly;
s->parallelModeLevel = 0;
s->ybDataSentForCurrQuery = p->ybDataSentForCurrQuery;
s->ybDataSent = p->ybDataSent;

CurrentTransactionState = s;
YBUpdateActiveSubTransaction(CurrentTransactionState);
Expand Down Expand Up @@ -5354,6 +5355,11 @@ PopTransaction(void)
if (s->parent == NULL)
elog(FATAL, "PopTransaction with no parent");

/* Propagate the data sent information to the parent. */
s->parent->ybDataSent = s->parent->ybDataSent || s->ybDataSent;
s->parent->ybDataSentForCurrQuery = s->parent->ybDataSentForCurrQuery ||
s->ybDataSentForCurrQuery;

CurrentTransactionState = s->parent;
YBUpdateActiveSubTransaction(CurrentTransactionState);

Expand Down

0 comments on commit 1c8a47e

Please sign in to comment.