Skip to content

Commit

Permalink
[BACKPORT 2.20][#19975] YSQL: Fix assertion failure in per-database c…
Browse files Browse the repository at this point in the history
…atalog version

Summary:
To reproduce the bug:

1. Create a cluster and convert it into per-database catalog version mode
```
yugabyte=# select * from pg_yb_catalog_version;
 db_oid | current_version | last_breaking_version
--------+-----------------+-----------------------
      1 |               1 |                     1
  13242 |               1 |                     1
  13243 |               1 |                     1
  13245 |               1 |                     1
  13246 |               1 |                     1
(5 rows)
```
The above query output indicates that the cluster is running in per-database
catalog version mode.

2. Run the following DDL global-impact DDL statement
```
yugabyte=# ALTER ROLE yugabyte SUPERUSER;
server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.

```

In the PG log:

```
F1122 01:04:40.396021 15991 pg_txn_manager.cc:346] Check failed: !IsDdlMode() READ COMMITTED semantics don't apply to DDL transactions
```

This happens when we call `yb_increment_all_db_catalog_versions` to increment
catalog versions for all databases. This is done inside function
`YBDecrementDdlNestingLevel` which first does
```
    --ddl_transaction_state.nesting_level;
```
and then if `ddl_transaction_state.nesting_level == 0` it calls
`yb_increment_all_db_catalog_versions` if the current DDL statement has global
impact. In `GetTransactionSnapshot` we have
```
       if (IsYBReadCommitted() && YBGetDdlNestingLevel() == 0)
```
The function `yb_increment_all_db_catalog_versions` is still called as part of
the DDL statement, but `ddl_transaction_state.nesting_level` is already 0 so
`YBGetDdlNestingLevel()` returns 0. As a result the above `if` condition is true
and the DDL statement is treated as a non-DDL statement, eventually leading to
the assertion failure.

The DDL mode is already transferred to pggate when we call
`YBCPgEnterSeparateDdlTxnMode`. So I added `YBCPgIsDdlMode()` and uses it to
replace `YBGetDdlNestingLevel() == 0`.

I reverted some work arounds in `pg_catalog_version-test.cc` which used snapshot
isolation because otherwise those unit tests would fail under read committed
isolation.

Original commit: 417e2de / D30417

Test Plan: ./yb_build.sh debug --cxx-test pg_catalog_version-test

Reviewers: tvesely, pjain

Reviewed By: tvesely

Subscribers: yql

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D30840
  • Loading branch information
myang2021 committed Dec 7, 2023
1 parent 5409a5e commit 16ecf34
Show file tree
Hide file tree
Showing 5 changed files with 11 additions and 1 deletion.
2 changes: 1 addition & 1 deletion src/postgres/src/backend/utils/time/snapmgr.c
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ GetTransactionSnapshot(void)
*
* READ COMMITTED semantics don't apply to DDLs.
*/
if (IsYBReadCommitted() && YBGetDdlNestingLevel() == 0)
if (IsYBReadCommitted() && !YBCPgIsDdlMode())
{
HandleYBStatus(YBCPgFlushBufferedOperations());
/* If this is a retry for a kReadRestart error, avoid resetting the read point */
Expand Down
4 changes: 4 additions & 0 deletions src/yb/yql/pggate/pggate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2034,6 +2034,10 @@ Status PgApiImpl::GetActiveTransactions(YBCPgSessionTxnInfo* infos, size_t num_i
}));
}

bool PgApiImpl::IsDdlMode() const {
return pg_txn_manager_->IsDdlMode();
}

void PgApiImpl::ResetCatalogReadTime() {
pg_session_->ResetCatalogReadPoint();
}
Expand Down
1 change: 1 addition & 0 deletions src/yb/yql/pggate/pggate.h
Original file line number Diff line number Diff line change
Expand Up @@ -625,6 +625,7 @@ class PgApiImpl {
TxnPriorityRequirement GetTransactionPriorityType() const;
Result<Uuid> GetActiveTransaction() const;
Status GetActiveTransactions(YBCPgSessionTxnInfo* infos, size_t num_infos);
bool IsDdlMode() const;

//------------------------------------------------------------------------------------------------
// Expressions.
Expand Down
4 changes: 4 additions & 0 deletions src/yb/yql/pggate/ybc_pggate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1429,6 +1429,10 @@ YBCStatus YBCPgActiveTransactions(YBCPgSessionTxnInfo *infos, size_t num_infos)
return ToYBCStatus(pgapi->GetActiveTransactions(infos, num_infos));
}

bool YBCPgIsDdlMode() {
return pgapi->IsDdlMode();
}

//------------------------------------------------------------------------------------------------
// System validation.
//------------------------------------------------------------------------------------------------
Expand Down
1 change: 1 addition & 0 deletions src/yb/yql/pggate/ybc_pggate.h
Original file line number Diff line number Diff line change
Expand Up @@ -582,6 +582,7 @@ double YBCGetTransactionPriority();
TxnPriorityRequirement YBCGetTransactionPriorityType();
YBCStatus YBCPgGetSelfActiveTransaction(YBCPgUuid *txn_id, bool *is_null);
YBCStatus YBCPgActiveTransactions(YBCPgSessionTxnInfo *infos, size_t num_infos);
bool YBCPgIsDdlMode();

// System validation -------------------------------------------------------------------------------
// Validate placement information
Expand Down

0 comments on commit 16ecf34

Please sign in to comment.