Skip to content

Commit

Permalink
[BACKPORT 2.16][#19509] YSQL: Fix YB does not release LWLocks on Abor…
Browse files Browse the repository at this point in the history
…tTransaction

Summary:
Original commit: 7e22b28 / D29194

This revision backports the fix for releasing LWLocks on AbortTransaction. It is imported from the replication slot support revision: https://phorge.dev.yugabyte.com/D29194.

The issue was introduced in 3f536a8 / D5518
Jira: DB-8305

Test Plan:
```
./yb_build.sh --cxx-test pgwrapper_pg_libpq-test --gtest_filter PgLibPqTest.TestLWPgBackendErrorAfterLWLockAcquire
```

Reviewers: telgersma

Reviewed By: telgersma

Subscribers: yql

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D29996
  • Loading branch information
dr0pdb committed Nov 10, 2023
1 parent 0cdc87d commit 470bf5b
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 9 deletions.
27 changes: 27 additions & 0 deletions src/postgres/contrib/pg_stat_statements/pg_stat_statements.c
Original file line number Diff line number Diff line change
Expand Up @@ -2313,6 +2313,25 @@ yb_lwlock_crash_after_acquire_pg_stat_statements_reset()

}

/*
* Function that caches environmental variable
* FLAGS_TEST_yb_lwlock_error_after_acquire_pg_stat_statements_reset.
*
* This avoids the process of checking the value of the environmental variable
* time and again.
*/
bool
yb_lwlock_error_after_acquire_pg_stat_statements_reset()
{
static int cached_value = -1;
if (cached_value == -1)
{
cached_value = YBCIsEnvVarTrue(
"FLAGS_TEST_yb_lwlock_error_after_acquire_pg_stat_statements_reset");
}
return cached_value;
}

/*
* Release all entries.
*/
Expand All @@ -2333,6 +2352,14 @@ entry_reset(void)

if (yb_lwlock_crash_after_acquire_pg_stat_statements_reset())
kill(getpid(), 9);

if (yb_lwlock_error_after_acquire_pg_stat_statements_reset())
ereport(ERROR,
(errcode(ERRCODE_INTERNAL_ERROR),
errmsg("Forced error since "
"yb_lwlock_error_after_acquire_pg_stat_statements_reset"
" is set")));

/*
* Write new empty query file, perhaps even creating a new one to recover
* if the file was missing.
Expand Down
16 changes: 7 additions & 9 deletions src/postgres/src/backend/access/transam/xact.c
Original file line number Diff line number Diff line change
Expand Up @@ -2678,15 +2678,13 @@ AbortTransaction(void)
AtAbort_Memory();
AtAbort_ResourceOwner();

if (YBIsPgLockingEnabled()) {
/*
* Release any LW locks we might be holding as quickly as possible.
* (Regular locks, however, must be held till we finish aborting.)
* Releasing LW locks is critical since we might try to grab them again
* while cleaning up!
*/
LWLockReleaseAll();
}
/*
* Release any LW locks we might be holding as quickly as possible.
* (Regular locks, however, must be held till we finish aborting.)
* Releasing LW locks is critical since we might try to grab them again
* while cleaning up!
*/
LWLockReleaseAll();

/* Clear wait information and command progress indicator */
pgstat_report_wait_end();
Expand Down
4 changes: 4 additions & 0 deletions src/yb/yql/pggate/pggate_flags.cc
Original file line number Diff line number Diff line change
Expand Up @@ -139,5 +139,9 @@ DEFINE_bool(yb_enable_read_committed_isolation, false,
DEFINE_test_flag(bool, yb_lwlock_crash_after_acquire_pg_stat_statements_reset, false,
"Issue sigkill for crash test after acquiring a LWLock in pg_stat_statements reset.");

DEFINE_test_flag(bool, yb_lwlock_error_after_acquire_pg_stat_statements_reset, false,
"Issue ereport(ERROR) for error test after acquiring a LWLock"
" in pg_stat_statements reset.");

DEFINE_test_flag(bool, yb_test_fail_matview_refresh_after_creation, false,
"Fail a refresh on a matview after the creation of a new relation.");
1 change: 1 addition & 0 deletions src/yb/yql/pggate/pggate_flags.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ DECLARE_int32(ysql_max_write_restart_attempts);
DECLARE_bool(ysql_sleep_before_retry_on_txn_conflict);
DECLARE_bool(ysql_disable_portal_run_context);
DECLARE_bool(TEST_yb_lwlock_crash_after_acquire_pg_stat_statements_reset);
DECLARE_bool(TEST_yb_lwlock_error_after_acquire_pg_stat_statements_reset);
DECLARE_bool(TEST_yb_test_fail_matview_refresh_after_creation);
DECLARE_bool(ysql_enable_read_request_caching);
#endif // YB_YQL_PGGATE_PGGATE_FLAGS_H
26 changes: 26 additions & 0 deletions src/yb/yql/pgwrapper/pg_libpq-test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2873,6 +2873,32 @@ TEST_F_EX(PgLibPqTest,
}
#endif

// The motive of this test is to prove that when a postgres backend errors out
// while possessing an LWLock, the lock is released.
// TEST_yb_lwlock_error_after_acquire_pg_stat_statements_reset when set true
// will error out a postgres backend after acquiring a LWLock. Specifically in
// this example, when pg_stat_statements_reset() function is called when this
// flag is set, it errors out after acquiring a lock on pgss->lock.
// We verify that future commands on the same connection do not deadlock as the
// lock should have been released after error.
class PgLibPqYSQLBackendError: public PgLibPqTest {
public:
void UpdateMiniClusterOptions(ExternalMiniClusterOptions* options) override {
options->extra_tserver_flags.push_back(
Format("--TEST_yb_lwlock_error_after_acquire_pg_stat_statements_reset=true"));
}
};

TEST_F_EX(PgLibPqTest,
YB_DISABLE_TEST_IN_TSAN(TestLWPgBackendErrorAfterLWLockAcquire),
PgLibPqYSQLBackendError) {
auto conn = ASSERT_RESULT(Connect());
ASSERT_NOK(conn.FetchFormat("SELECT pg_stat_statements_reset()"));

// Verify that future commands on the same connection works.
EXPECT_OK(conn.FetchFormat("SELECT 1"));
}

class PgLibPqRefreshMatviewFailure: public PgLibPqTest {
public:
void UpdateMiniClusterOptions(ExternalMiniClusterOptions* options) override {
Expand Down

0 comments on commit 470bf5b

Please sign in to comment.