On Tue, Feb 23, 2021 at 3:59 AM Andres Freund <and...@anarazel.de> wrote:
>
> Hi,
>
> The 2pc decoding added in
>
> commit a271a1b50e9bec07e2ef3a05e38e7285113e4ce6
> Author: Amit Kapila <akap...@postgresql.org>
> Date: 2021-01-04 08:34:50 +0530
>
> Allow decoding at prepare time in ReorderBuffer.
>
> has a deadlock danger when used in a way that takes advantage of
> separate decoding of the 2PC PREPARE.
>
>
> I assume the goal of decoding the 2PC PREPARE is so one can wait for the
> PREPARE to have logically replicated, before doing the COMMIT PREPARED.
>
>
> However, currently it's pretty easy to get into a state where logical
> decoding cannot progress until the 2PC transaction has
> committed/aborted. Which essentially would lead to undetected deadlocks.
>
> The problem is that catalog tables accessed during logical decoding need
> to get locked (otherwise e.g. a table rewrite could happen
> concurrently). But if the prepared transaction itself holds a lock on a
> catalog table, logical decoding will block on that lock - which won't be
> released until replication progresses. A deadlock.
>
> A trivial example:
>
> SELECT pg_create_logical_replication_slot('test', 'test_decoding');
> CREATE TABLE foo(id serial primary key);
> BEGIN;
> LOCK pg_class;
> INSERT INTO foo DEFAULT VALUES;
> PREPARE TRANSACTION 'foo';
>
> -- hangs waiting for pg_class to be unlocked
> SELECT pg_logical_slot_get_changes('test', NULL, NULL, 'two-phase-commit',
> '1');
>
>
> Now, more realistic versions of this scenario would probably lock a
> 'user catalog table' containing replication metadata instead of
> pg_class, but ...
>
>
> At first this seems to be a significant issue. But on the other hand, if
> you were to shut the cluster down in this situation (or disconnect all
> sessions), you have broken cluster on your hand - without logical
> decoding being involved. As it turns out, we need to read pg_class to
> log in... And I can't remember this being reported to be a problem?
>
>
> Perhaps all that we need to do is to disallow 2PC prepare if [user]
> catalog tables have been locked exclusively? Similar to how we're
> disallowing preparing tables with temp table access.
>
Even I felt we should not allow prepare a transaction that has locked
system tables, as it does not allow creating a new session after
restart and also causes the deadlock while logical decoding of
prepared transaction.
I have made a patch to make the prepare transaction fail in this
scenario. Attached the patch for the same.
Thoughts?
Regards,
Vignesh
From 93014a12a6a7c576cdd11028eaa0e7fa2afa7205 Mon Sep 17 00:00:00 2001
From: vignesh <vignes...@gmail.com>
Date: Mon, 15 Mar 2021 17:21:04 +0530
Subject: [PATCH v1] 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 6395a9b240..d5d54a9e3b 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -2448,6 +2448,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 a 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 34cfaf542c..8eeffea270 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..7d9d616484 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 a 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 a 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 a 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 a 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..53a4f7fa9b 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 a 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 a 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 a 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 a 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
