On Wed, Mar 31, 2021 at 2:35 PM Ajin Cherian <itsa...@gmail.com> wrote:
>
> The patch applies fine on HEAD and "make check" passes fine. No major
> comments on the patch, just a minor comment:
>
> If you could change the error from, " cannot PREPARE a transaction that has a
> lock on user catalog/system table(s)"
> to "cannot PREPARE a transaction that has an exclusive lock on user
> catalog/system table(s)" that would be a more
> accurate instruction to the user.
>
Thanks for reviewing the patch.
Please find the updated patch which includes the fix for the same.
Regards,
Vignesh
From 44c637668a3fc92c5ed9abc23d2c56df9116c6fb Mon Sep 17 00:00:00 2001
From: vignesh <vignes...@gmail.com>
Date: Mon, 15 Mar 2021 17:21:04 +0530
Subject: [PATCH v2] Fail prepared transaction if transaction has locked system
tables/user catalog tables.
Don't allow PREPARE TRANSACTION if we've locked a system table or an user
catalog table in this transaction. Allowing the prepared transactions when it
locked system tables will result in hang while logical decoding of prepared
transactions as logical decoding of prepared transactions locks the system
table.
---
doc/src/sgml/ref/prepare_transaction.sgml | 8 ++
src/backend/access/transam/xact.c | 14 +++
src/backend/commands/lockcmds.c | 17 +++
src/include/access/xact.h | 7 ++
src/test/regress/expected/prepared_xacts.out | 102 ++++++++++++++++
.../regress/expected/prepared_xacts_1.out | 112 ++++++++++++++++++
src/test/regress/sql/prepared_xacts.sql | 75 ++++++++++++
7 files changed, 335 insertions(+)
diff --git a/doc/src/sgml/ref/prepare_transaction.sgml b/doc/src/sgml/ref/prepare_transaction.sgml
index f4f6118ac3..ee8e4072f5 100644
--- a/doc/src/sgml/ref/prepare_transaction.sgml
+++ b/doc/src/sgml/ref/prepare_transaction.sgml
@@ -106,6 +106,14 @@ PREPARE TRANSACTION <replaceable class="parameter">transaction_id</replaceable>
tied to the current session to be useful in a transaction to be prepared.
</para>
+ <para>
+ It is not currently allowed to <command>PREPARE</command> a transaction that
+ has locked system table(s) or user defined catalog table(s). These features
+ are too tightly tied to logical decoding of
+ <command>PREPARE TRANSACTION</command> and creation of new login session to
+ be useful in a transaction to be prepared.
+ </para>
+
<para>
If the transaction modified any run-time parameters with <command>SET</command>
(without the <literal>LOCAL</literal> option),
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index c83aa16f2c..00010c2bd2 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -2422,6 +2422,20 @@ PrepareTransaction(void)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot PREPARE a transaction that has operated on temporary objects")));
+ /*
+ * Don't allow PREPARE TRANSACTION if we've locked a system table or an user
+ * catalog table in this transaction. Allowing the prepared transactions
+ * when it locked system tables will result in deadlock while logical
+ * decoding of prepared transactions as logical decoding of prepared
+ * transactions locks the system table. Restarting the server when there is
+ * a prepared transaction that has a lock on system table results in a
+ * deadlock as we need to access pg_class during login.
+ */
+ if (MyXactFlags & XACT_FLAGS_ACQUIREDEXCLUSIVELOCK_SYSREL)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot PREPARE a transaction that has an exclusive lock on user catalog/system table(s)")));
+
/*
* Likewise, don't allow PREPARE after pg_export_snapshot. This could be
* supported if we added cleanup logic to twophase.c, but for now it
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index 34f2270ced..b7848d3c6f 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -16,6 +16,7 @@
#include "access/table.h"
#include "access/xact.h"
+#include "catalog/catalog.h"
#include "catalog/namespace.h"
#include "catalog/pg_inherits.h"
#include "commands/lockcmds.h"
@@ -60,7 +61,23 @@ LockTableCommand(LockStmt *lockstmt)
if (get_rel_relkind(reloid) == RELKIND_VIEW)
LockViewRecurse(reloid, lockstmt->mode, lockstmt->nowait, NIL);
else if (recurse)
+ {
+ Relation rel;
LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait);
+ rel = table_open(reloid, NoLock);
+
+ /*
+ * Make note that we've locked a system table or an user catalog
+ * table. This flag will be checked later during prepare transaction
+ * to fail the prepare transaction.
+ */
+ if (lockstmt->mode >= ExclusiveLock &&
+ (IsCatalogRelationOid(reloid) ||
+ RelationIsUsedAsCatalogTable(rel)))
+ MyXactFlags |= XACT_FLAGS_ACQUIREDEXCLUSIVELOCK_SYSREL;
+
+ table_close(rel, NoLock);
+ }
}
}
diff --git a/src/include/access/xact.h b/src/include/access/xact.h
index f49a57b35e..f00a79706c 100644
--- a/src/include/access/xact.h
+++ b/src/include/access/xact.h
@@ -107,6 +107,13 @@ extern int MyXactFlags;
*/
#define XACT_FLAGS_ACQUIREDACCESSEXCLUSIVELOCK (1U << 1)
+/*
+ * XACT_FLAGS_ACQUIREDEXCLUSIVELOCK_SYSREL - records whether the top level
+ * xact locked the system tables or user catalog tables in Exclusive lock or
+ * higher.
+ */
+#define XACT_FLAGS_ACQUIREDEXCLUSIVELOCK_SYSREL (1U << 2)
+
/*
* start- and end-of-transaction callbacks for dynamically loaded modules
*/
diff --git a/src/test/regress/expected/prepared_xacts.out b/src/test/regress/expected/prepared_xacts.out
index eb77c18788..e8eab35ea7 100644
--- a/src/test/regress/expected/prepared_xacts.out
+++ b/src/test/regress/expected/prepared_xacts.out
@@ -247,8 +247,110 @@ SELECT gid FROM pg_prepared_xacts;
-----
(0 rows)
+-- Test PREPARED TRANSACTION on a transaction which locked non system tables
+CREATE TABLE pxtest5 (c1 INT);
+begin;
+lock table pxtest5 in SHARE ROW EXCLUSIVE MODE;
+PREPARE TRANSACTION 'prep_txn_lock_table_1';
+-- The above transaction should be prepared.
+SELECT gid FROM pg_prepared_xacts;
+ gid
+-----------------------
+ prep_txn_lock_table_1
+(1 row)
+
+ROLLBACK PREPARED 'prep_txn_lock_table_1';
+begin;
+lock table pxtest5 in ACCESS EXCLUSIVE MODE;
+PREPARE TRANSACTION 'prep_txn_lock_table_2';
+-- The above transaction should be prepared.
+SELECT gid FROM pg_prepared_xacts;
+ gid
+-----------------------
+ prep_txn_lock_table_2
+(1 row)
+
+ROLLBACK PREPARED 'prep_txn_lock_table_2';
+begin;
+lock table pxtest5 in EXCLUSIVE MODE;
+PREPARE TRANSACTION 'prep_txn_lock_table_3';
+-- The above transaction should be prepared.
+SELECT gid FROM pg_prepared_xacts;
+ gid
+-----------------------
+ prep_txn_lock_table_3
+(1 row)
+
+ROLLBACK PREPARED 'prep_txn_lock_table_3';
+-- Test PREPARED TRANSACTION on a transaction which locked system tables
+begin;
+lock table pg_class in SHARE ROW EXCLUSIVE MODE;
+PREPARE TRANSACTION 'prep_txn_lock_sys_table_1';
+-- The above transaction should be prepared.
+SELECT gid FROM pg_prepared_xacts;
+ gid
+---------------------------
+ prep_txn_lock_sys_table_1
+(1 row)
+
+ROLLBACK PREPARED 'prep_txn_lock_sys_table_1';
+begin;
+lock table pg_class in ACCESS EXCLUSIVE MODE;
+-- The above transaction should fail with error.
+PREPARE TRANSACTION 'prep_txn_lock_sys_table_2';
+ERROR: cannot PREPARE a transaction that has an exclusive lock on user catalog/system table(s)
+SELECT gid FROM pg_prepared_xacts;
+ gid
+-----
+(0 rows)
+
+begin;
+lock table pg_class in EXCLUSIVE MODE;
+-- The above transaction should fail with error.
+PREPARE TRANSACTION 'prep_txn_lock_sys_table_3';
+ERROR: cannot PREPARE a transaction that has an exclusive lock on user catalog/system table(s)
+SELECT gid FROM pg_prepared_xacts;
+ gid
+-----
+(0 rows)
+
+-- Test PREPARED TRANSACTION on a transaction which locked user catalog tables
+CREATE TABLE pxtest6(c1 int) WITH (user_catalog_table = 'on');
+begin;
+lock table pxtest6 in SHARE ROW EXCLUSIVE MODE;
+PREPARE TRANSACTION 'prep_txn_lock_user_catalog_table_1';
+-- The above transaction should be prepared.
+SELECT gid FROM pg_prepared_xacts;
+ gid
+------------------------------------
+ prep_txn_lock_user_catalog_table_1
+(1 row)
+
+ROLLBACK PREPARED 'prep_txn_lock_user_catalog_table_1';
+begin;
+lock table pxtest6 in ACCESS EXCLUSIVE MODE;
+-- The above transaction should fail with error.
+PREPARE TRANSACTION 'prep_txn_lock_user_catalog_table_2';
+ERROR: cannot PREPARE a transaction that has an exclusive lock on user catalog/system table(s)
+SELECT gid FROM pg_prepared_xacts;
+ gid
+-----
+(0 rows)
+
+begin;
+lock table pxtest6 in EXCLUSIVE MODE;
+-- The above transaction should fail with error.
+PREPARE TRANSACTION 'prep_txn_lock_user_catalog_table_3';
+ERROR: cannot PREPARE a transaction that has an exclusive lock on user catalog/system table(s)
+SELECT gid FROM pg_prepared_xacts;
+ gid
+-----
+(0 rows)
+
-- Clean up
DROP TABLE pxtest2;
DROP TABLE pxtest3; -- will still be there if prepared xacts are disabled
ERROR: table "pxtest3" does not exist
DROP TABLE pxtest4;
+DROP TABLE pxtest5;
+DROP TABLE pxtest6;
diff --git a/src/test/regress/expected/prepared_xacts_1.out b/src/test/regress/expected/prepared_xacts_1.out
index 0857d259e0..641977fede 100644
--- a/src/test/regress/expected/prepared_xacts_1.out
+++ b/src/test/regress/expected/prepared_xacts_1.out
@@ -241,9 +241,121 @@ SELECT gid FROM pg_prepared_xacts;
-----
(0 rows)
+-- Test PREPARED TRANSACTION on a transaction which locked non system tables
+CREATE TABLE pxtest5 (c1 INT);
+begin;
+lock table pxtest5 in SHARE ROW EXCLUSIVE MODE;
+PREPARE TRANSACTION 'prep_txn_lock_table_1';
+ERROR: prepared transactions are disabled
+HINT: Set max_prepared_transactions to a nonzero value.
+-- The above transaction should be prepared.
+SELECT gid FROM pg_prepared_xacts;
+ gid
+-----
+(0 rows)
+
+ROLLBACK PREPARED 'prep_txn_lock_table_1';
+ERROR: prepared transaction with identifier "prep_txn_lock_table_1" does not exist
+begin;
+lock table pxtest5 in ACCESS EXCLUSIVE MODE;
+PREPARE TRANSACTION 'prep_txn_lock_table_2';
+ERROR: prepared transactions are disabled
+HINT: Set max_prepared_transactions to a nonzero value.
+-- The above transaction should be prepared.
+SELECT gid FROM pg_prepared_xacts;
+ gid
+-----
+(0 rows)
+
+ROLLBACK PREPARED 'prep_txn_lock_table_2';
+ERROR: prepared transaction with identifier "prep_txn_lock_table_2" does not exist
+begin;
+lock table pxtest5 in EXCLUSIVE MODE;
+PREPARE TRANSACTION 'prep_txn_lock_table_3';
+ERROR: prepared transactions are disabled
+HINT: Set max_prepared_transactions to a nonzero value.
+-- The above transaction should be prepared.
+SELECT gid FROM pg_prepared_xacts;
+ gid
+-----
+(0 rows)
+
+ROLLBACK PREPARED 'prep_txn_lock_table_3';
+ERROR: prepared transaction with identifier "prep_txn_lock_table_3" does not exist
+-- Test PREPARED TRANSACTION on a transaction which locked system tables
+begin;
+lock table pg_class in SHARE ROW EXCLUSIVE MODE;
+PREPARE TRANSACTION 'prep_txn_lock_sys_table_1';
+ERROR: prepared transactions are disabled
+HINT: Set max_prepared_transactions to a nonzero value.
+-- The above transaction should be prepared.
+SELECT gid FROM pg_prepared_xacts;
+ gid
+-----
+(0 rows)
+
+ROLLBACK PREPARED 'prep_txn_lock_sys_table_1';
+ERROR: prepared transaction with identifier "prep_txn_lock_sys_table_1" does not exist
+begin;
+lock table pg_class in ACCESS EXCLUSIVE MODE;
+-- The above transaction should fail with error.
+PREPARE TRANSACTION 'prep_txn_lock_sys_table_2';
+ERROR: cannot PREPARE a transaction that has an exclusive lock on user catalog/system table(s)
+SELECT gid FROM pg_prepared_xacts;
+ gid
+-----
+(0 rows)
+
+begin;
+lock table pg_class in EXCLUSIVE MODE;
+-- The above transaction should fail with error.
+PREPARE TRANSACTION 'prep_txn_lock_sys_table_3';
+ERROR: cannot PREPARE a transaction that has an exclusive lock on user catalog/system table(s)
+SELECT gid FROM pg_prepared_xacts;
+ gid
+-----
+(0 rows)
+
+-- Test PREPARED TRANSACTION on a transaction which locked user catalog tables
+CREATE TABLE pxtest6(c1 int) WITH (user_catalog_table = 'on');
+begin;
+lock table pxtest6 in SHARE ROW EXCLUSIVE MODE;
+PREPARE TRANSACTION 'prep_txn_lock_user_catalog_table_1';
+ERROR: prepared transactions are disabled
+HINT: Set max_prepared_transactions to a nonzero value.
+-- The above transaction should be prepared.
+SELECT gid FROM pg_prepared_xacts;
+ gid
+-----
+(0 rows)
+
+ROLLBACK PREPARED 'prep_txn_lock_user_catalog_table_1';
+ERROR: prepared transaction with identifier "prep_txn_lock_user_catalog_table_1" does not exist
+begin;
+lock table pxtest6 in ACCESS EXCLUSIVE MODE;
+-- The above transaction should fail with error.
+PREPARE TRANSACTION 'prep_txn_lock_user_catalog_table_2';
+ERROR: cannot PREPARE a transaction that has an exclusive lock on user catalog/system table(s)
+SELECT gid FROM pg_prepared_xacts;
+ gid
+-----
+(0 rows)
+
+begin;
+lock table pxtest6 in EXCLUSIVE MODE;
+-- The above transaction should fail with error.
+PREPARE TRANSACTION 'prep_txn_lock_user_catalog_table_3';
+ERROR: cannot PREPARE a transaction that has an exclusive lock on user catalog/system table(s)
+SELECT gid FROM pg_prepared_xacts;
+ gid
+-----
+(0 rows)
+
-- Clean up
DROP TABLE pxtest2;
ERROR: table "pxtest2" does not exist
DROP TABLE pxtest3; -- will still be there if prepared xacts are disabled
DROP TABLE pxtest4;
ERROR: table "pxtest4" does not exist
+DROP TABLE pxtest5;
+DROP TABLE pxtest6;
diff --git a/src/test/regress/sql/prepared_xacts.sql b/src/test/regress/sql/prepared_xacts.sql
index d8249a27dc..65306b46df 100644
--- a/src/test/regress/sql/prepared_xacts.sql
+++ b/src/test/regress/sql/prepared_xacts.sql
@@ -152,7 +152,82 @@ SELECT * FROM pxtest3;
-- There should be no prepared transactions
SELECT gid FROM pg_prepared_xacts;
+-- Test PREPARED TRANSACTION on a transaction which locked non system tables
+CREATE TABLE pxtest5 (c1 INT);
+begin;
+lock table pxtest5 in SHARE ROW EXCLUSIVE MODE;
+PREPARE TRANSACTION 'prep_txn_lock_table_1';
+
+-- The above transaction should be prepared.
+SELECT gid FROM pg_prepared_xacts;
+ROLLBACK PREPARED 'prep_txn_lock_table_1';
+
+begin;
+lock table pxtest5 in ACCESS EXCLUSIVE MODE;
+PREPARE TRANSACTION 'prep_txn_lock_table_2';
+
+-- The above transaction should be prepared.
+SELECT gid FROM pg_prepared_xacts;
+ROLLBACK PREPARED 'prep_txn_lock_table_2';
+
+begin;
+lock table pxtest5 in EXCLUSIVE MODE;
+PREPARE TRANSACTION 'prep_txn_lock_table_3';
+
+-- The above transaction should be prepared.
+SELECT gid FROM pg_prepared_xacts;
+ROLLBACK PREPARED 'prep_txn_lock_table_3';
+
+-- Test PREPARED TRANSACTION on a transaction which locked system tables
+begin;
+lock table pg_class in SHARE ROW EXCLUSIVE MODE;
+PREPARE TRANSACTION 'prep_txn_lock_sys_table_1';
+
+-- The above transaction should be prepared.
+SELECT gid FROM pg_prepared_xacts;
+ROLLBACK PREPARED 'prep_txn_lock_sys_table_1';
+
+begin;
+lock table pg_class in ACCESS EXCLUSIVE MODE;
+-- The above transaction should fail with error.
+PREPARE TRANSACTION 'prep_txn_lock_sys_table_2';
+
+SELECT gid FROM pg_prepared_xacts;
+
+begin;
+lock table pg_class in EXCLUSIVE MODE;
+-- The above transaction should fail with error.
+PREPARE TRANSACTION 'prep_txn_lock_sys_table_3';
+
+SELECT gid FROM pg_prepared_xacts;
+
+-- Test PREPARED TRANSACTION on a transaction which locked user catalog tables
+CREATE TABLE pxtest6(c1 int) WITH (user_catalog_table = 'on');
+begin;
+lock table pxtest6 in SHARE ROW EXCLUSIVE MODE;
+PREPARE TRANSACTION 'prep_txn_lock_user_catalog_table_1';
+
+-- The above transaction should be prepared.
+SELECT gid FROM pg_prepared_xacts;
+ROLLBACK PREPARED 'prep_txn_lock_user_catalog_table_1';
+
+begin;
+lock table pxtest6 in ACCESS EXCLUSIVE MODE;
+-- The above transaction should fail with error.
+PREPARE TRANSACTION 'prep_txn_lock_user_catalog_table_2';
+
+SELECT gid FROM pg_prepared_xacts;
+
+begin;
+lock table pxtest6 in EXCLUSIVE MODE;
+-- The above transaction should fail with error.
+PREPARE TRANSACTION 'prep_txn_lock_user_catalog_table_3';
+
+SELECT gid FROM pg_prepared_xacts;
+
-- Clean up
DROP TABLE pxtest2;
DROP TABLE pxtest3; -- will still be there if prepared xacts are disabled
DROP TABLE pxtest4;
+DROP TABLE pxtest5;
+DROP TABLE pxtest6;
--
2.25.1
