diff --git a/src/postgres/contrib/pg_stat_statements/pg_stat_statements.c b/src/postgres/contrib/pg_stat_statements/pg_stat_statements.c index 8fd3773dd194..8a877b397db6 100644 --- a/src/postgres/contrib/pg_stat_statements/pg_stat_statements.c +++ b/src/postgres/contrib/pg_stat_statements/pg_stat_statements.c @@ -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. */ @@ -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. diff --git a/src/postgres/src/backend/access/transam/xact.c b/src/postgres/src/backend/access/transam/xact.c index f52bc4fef24f..dc686e5c058f 100644 --- a/src/postgres/src/backend/access/transam/xact.c +++ b/src/postgres/src/backend/access/transam/xact.c @@ -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(); diff --git a/src/yb/yql/pggate/pggate_flags.cc b/src/yb/yql/pggate/pggate_flags.cc index 9c9f2c1b6366..018b82ead970 100644 --- a/src/yb/yql/pggate/pggate_flags.cc +++ b/src/yb/yql/pggate/pggate_flags.cc @@ -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."); diff --git a/src/yb/yql/pggate/pggate_flags.h b/src/yb/yql/pggate/pggate_flags.h index cfc65c607b43..9ea14fc1453a 100644 --- a/src/yb/yql/pggate/pggate_flags.h +++ b/src/yb/yql/pggate/pggate_flags.h @@ -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 diff --git a/src/yb/yql/pgwrapper/pg_libpq-test.cc b/src/yb/yql/pgwrapper/pg_libpq-test.cc index 864919600ebc..97b4d47728ea 100644 --- a/src/yb/yql/pgwrapper/pg_libpq-test.cc +++ b/src/yb/yql/pgwrapper/pg_libpq-test.cc @@ -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 {