Skip to content

Commit

Permalink
[#20820] YSQL: Fix !options.ddl_mode() Check failure in debug build
Browse files Browse the repository at this point in the history
Summary:
This bug can be reproduced by enabling a specific case in a unit test
`VerifyWaitStateEntered/kBackfillIndex_WaitForAFreeSlot`. The relevant code is

```
RSTATUS_DCHECK(!options.ddl_mode(), NotSupported, "Restarting a DDL transaction not supported");
```

This happens when the PG backend is trying to restart a DDL statement and at
tserver side this isn't expected. In debug build it results in a CHECK failure.

In this test, PG tries to restart a `CREATE INDEX idx_0 on t (value)` statement.
Due to a concurrent write statement and some additional wait-state related logic
in this unit test, the CREATE INDEX hits a `Restart read required` error. Then
due to the following code, the DDL state is reset:

```
    PG_CATCH();
    {
        if (is_ddl)
        {
            /*
             * It is possible that nesting_level has wrong value due to error.
             * Ddl transaction state should be reset.
             */
            YBResetDdlState();
        }
        PG_RE_THROW();
    }
    PG_END_TRY();
```

In particular, `YBResetDdlState()` invokes

```
    HandleYBStatus(YBCPgClearSeparateDdlTxnMode());
```

As a result, PG no longer knows that the current statement is a DDL statement,
and in function `yb_is_restart_possible`
```
    if (IsYBReadCommitted())
    {
        if (YBGetDdlNestingLevel() != 0) {
            elog(LOG, "READ COMMITTED retry semantics don't support DDLs");
            *rc_ignoring_ddl_statement = true;
            return false;
        }
    }
```

Now that `YBGetDdlNestingLevel() == 0` after `YBResetDdlState()`, we fail to
hit `READ COMMITTED retry semantics don't support DDLs`, hence there will be
the retry of the DDL statement via

```
            if (YBCIsRestartReadError(edata->yb_txn_errcode))
            {
                YBCRestartTransaction();
            }
```

Then when we retry, we re-detect that `CREATE INDEX` is a DDL statement. However
at this time `YBCRestartTransaction()` is already invoked, which has set
`need_restart_` to true.

I changed  RSTATUS_DCHECK to return STATUS when `options.ddl_mode()` is true.
In this way we avoid tserver CHECK failure in debug build.
Jira: DB-9809

Test Plan:
Make the following test change:

```

diff --git a/src/yb/integration-tests/wait_states-itest.cc b/src/yb/integration-tests/wait_states-itest.cc
index 08a4f7d6ea..ada826780e 100644
--- a/src/yb/integration-tests/wait_states-itest.cc
+++ b/src/yb/integration-tests/wait_states-itest.cc
@@ -670,8 +670,7 @@ INSTANTIATE_TEST_SUITE_P(
       ash::WaitStateCode::kRetryableRequests_SaveToDisk,
       ash::WaitStateCode::kMVCC_WaitForSafeTime,
       ash::WaitStateCode::kLockedBatchEntry_Lock,
-      // TODO(#20820) : Enable once #20820 is fixed
-      // ash::WaitStateCode::kBackfillIndex_WaitForAFreeSlot,
+      ash::WaitStateCode::kBackfillIndex_WaitForAFreeSlot,
       ash::WaitStateCode::kCreatingNewTablet,
       ash::WaitStateCode::kSaveRaftGroupMetadataToDisk,
       ash::WaitStateCode::kDumpRunningRpc_WaitOnReactor,
```
then run the test

./yb_build.sh debug --cxx-test integration-tests_wait_states-itest --gtest_filter WaitStateITest/AshTestVerifyOccurrence.VerifyWaitStateEntered/kBackfillIndex_WaitForAFreeSlot

Note that the test still fails but the `!options.ddl_mode()` Check failure no longer appears.

Reviewers: pjain, amitanand

Reviewed By: pjain

Subscribers: bogdan, ybase, yql

Differential Revision: https://phorge.dev.yugabyte.com/D32444
  • Loading branch information
myang2021 committed Feb 21, 2024
1 parent a313efa commit 0ff0bfc
Showing 1 changed file with 3 additions and 1 deletion.
4 changes: 3 additions & 1 deletion src/yb/tserver/pg_client_session.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1192,7 +1192,9 @@ PgClientSession::SetupSession(
const auto read_time_serial_no = options.read_time_serial_no();
UsedReadTimePtr used_read_time;
if (options.restart_transaction()) {
RSTATUS_DCHECK(!options.ddl_mode(), NotSupported, "Restarting a DDL transaction not supported");
if (options.ddl_mode()) {
return STATUS(NotSupported, "Restarting a DDL transaction not supported");
}
Transaction(kind) = VERIFY_RESULT(RestartTransaction(session, transaction));
transaction = Transaction(kind).get();
} else {
Expand Down

0 comments on commit 0ff0bfc

Please sign in to comment.