commit 5d0c2d5b847bd48aa4c187077717ca99a90086bd Author: Stas Kelvich Date: Tue May 14 12:47:59 2019 +0300 Read-only access to temp tables for 2PC transactions diff --git a/src/backend/access/common/relation.c b/src/backend/access/common/relation.c index 41a0051f88..c70449bf4c 100644 --- a/src/backend/access/common/relation.c +++ b/src/backend/access/common/relation.c @@ -23,6 +23,7 @@ #include "access/relation.h" #include "access/xact.h" #include "catalog/namespace.h" +#include "commands/tablecmds.h" #include "miscadmin.h" #include "pgstat.h" #include "storage/lmgr.h" @@ -69,9 +70,19 @@ relation_open(Oid relationId, LOCKMODE lockmode) IsBootstrapProcessingMode() || CheckRelationLockedByMe(r, AccessShareLock, true)); - /* Make note that we've accessed a temporary relation */ - if (RelationUsesLocalBuffers(r)) + /* + * Make note that we've accessed a temporary relation with at least + * RowShareLock lockmode. See also comments above MyXactFlags check + * in PrepareTransaction(). + */ + if (RelationUsesLocalBuffers(r) && + (lockmode > AccessShareLock || + (lockmode == NoLock && + CheckRelationLockedByMe(r, RowShareLock, true)))) + { + register_temprel_modify(relationId); MyXactFlags |= XACT_FLAGS_ACCESSEDTEMPNAMESPACE; + } pgstat_initstats(r); @@ -119,9 +130,19 @@ try_relation_open(Oid relationId, LOCKMODE lockmode) Assert(lockmode != NoLock || CheckRelationLockedByMe(r, AccessShareLock, true)); - /* Make note that we've accessed a temporary relation */ - if (RelationUsesLocalBuffers(r)) + /* + * Make note that we've accessed a temporary relation with at least + * RowShareLock lockmode. See also comments above MyXactFlags check + * in PrepareTransaction(). + */ + if (RelationUsesLocalBuffers(r) && + (lockmode > AccessShareLock || + (lockmode == NoLock && + CheckRelationLockedByMe(r, AccessShareLock, true)))) + { + register_temprel_modify(relationId); MyXactFlags |= XACT_FLAGS_ACCESSEDTEMPNAMESPACE; + } pgstat_initstats(r); diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 20feeec327..39a4c1106d 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -2352,12 +2352,14 @@ PrepareTransaction(void) /* NOTIFY will be handled below */ /* - * Don't allow PREPARE TRANSACTION if we've accessed a temporary table in + * Don't allow PREPARE TRANSACTION if we've modified a temporary table in * this transaction. Having the prepared xact hold locks on another * backend's temp table seems a bad idea --- for instance it would prevent * the backend from exiting. There are other problems too, such as how to * clean up the source backend's local buffers and ON COMMIT state if the - * prepared xact includes a DROP of a temp table. + * prepared xact includes a DROP of a temp table. Special case of reading + * from a temporary table is allowed by dropping AccessShare locks during + * PREPARE. * * Other objects types, like functions, operators or extensions, share the * same restriction as they should not be created, locked or dropped as @@ -2368,8 +2370,9 @@ PrepareTransaction(void) * might still access a temp relation. * * XXX In principle this could be relaxed to allow some useful special - * cases, such as a temp table created and dropped all within the - * transaction. That seems to require much more bookkeeping though. + * cases besides reading from a temp table, such as a temp table created + * and dropped all within the transaction. That seems to require much more + * bookkeeping though. */ if ((MyXactFlags & XACT_FLAGS_ACCESSEDTEMPNAMESPACE)) ereport(ERROR, diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index baeb13e6a0..1e3ae90d95 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -122,6 +122,12 @@ typedef struct OnCommitItem static List *on_commits = NIL; +/* + * Set of temp relation oid's for which we aquired locks stronger then + * AccessShare. During commit this set will be intersected with on_commits + * to determine which relations needs truncation. + */ +static HTAB *modified_temprels = NULL; /* * State information for ALTER TABLE @@ -14398,6 +14404,34 @@ register_on_commit_action(Oid relid, OnCommitAction action) MemoryContextSwitchTo(oldcxt); } +/* + * Track modifying access to temporary relations. + * Later we use that set to determine which relations should be + * truncated. + */ +void +register_temprel_modify(Oid relid) +{ + /* + * Init modified_temprels. + * max_locks_per_xact is used as a guess to the amount of relations + * that normally is accessed in a single transaction. + */ + if (modified_temprels == NULL) + { + HASHCTL ctl; + MemSet(&ctl, 0, sizeof(ctl)); + ctl.keysize = sizeof(Oid); + ctl.entrysize = sizeof(Oid); + ctl.hcxt = TopTransactionContext; + modified_temprels = hash_create("Temp rels modified in this tx", + max_locks_per_xact, + &ctl, HASH_ELEM | HASH_BLOBS); + } + + (void) hash_search(modified_temprels, &relid, HASH_ENTER, NULL); +} + /* * Unregister any ON COMMIT action when a relation is deleted. * @@ -14448,14 +14482,19 @@ PreCommit_on_commit_actions(void) /* Do nothing (there shouldn't be such entries, actually) */ break; case ONCOMMIT_DELETE_ROWS: - /* - * If this transaction hasn't accessed any temporary - * relations, we can skip truncating ON COMMIT DELETE ROWS - * tables, as they must still be empty. + * We will truncate only relations that we opened with lock + * stronger then AccessShare (so possibly were modified). */ - if ((MyXactFlags & XACT_FLAGS_ACCESSEDTEMPNAMESPACE)) - oids_to_truncate = lappend_oid(oids_to_truncate, oc->relid); + if (modified_temprels != NULL) + { + bool modified; + (void) hash_search(modified_temprels, &oc->relid, + HASH_FIND, &modified); + + if (modified) + oids_to_truncate = lappend_oid(oids_to_truncate, oc->relid); + } break; case ONCOMMIT_DROP: oids_to_drop = lappend_oid(oids_to_drop, oc->relid); @@ -14559,6 +14598,12 @@ AtEOXact_on_commit_actions(bool isCommit) cur_item = lnext(prev_item); } } + + /* + * Reset modified_temprels. Hash table itself will be released with + * TopTransactionContext. + */ + modified_temprels = NULL; } /* diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c index c8958766f1..9eee017c3d 100644 --- a/src/backend/storage/lmgr/lock.c +++ b/src/backend/storage/lmgr/lock.c @@ -3212,6 +3212,13 @@ AtPrepare_Locks(void) locallock->lock = locallock->proclock->tag.myLock; } + /* + * Do not save any AccessShare locks as they will not be transfered to + * a dummy PGPRPROC by PostPrepare_Locks(). + */ + if (locallock->tag.mode == AccessShareLock) + continue; + /* * Arrange to not release any strong lock count held by this lock * entry. We must retain the count until the prepared transaction is @@ -3382,6 +3389,33 @@ PostPrepare_Locks(TransactionId xid) if (proclock->releaseMask != proclock->holdMask) elog(PANIC, "we seem to have dropped a bit somewhere"); + /* + * To be able to safely read from temp tables prepared transaction + * should not hold AccessShare locks. So here we are releasing any + * AccessShare locks without transfering them further to a dummy + * PGPROC. See also comments above MyXactFlags check in + * PrepareTransaction(). + */ + if (proclock->holdMask & LOCKBIT_ON(AccessShareLock)) + { + LockMethod lockMethodTable; + bool wakeupNeeded; + + lockMethodTable = LockMethods[lock->tag.locktag_lockmethodid]; + wakeupNeeded = UnGrantLock(lock, AccessShareLock, + proclock, lockMethodTable); + + CleanUpLock(lock, proclock, lockMethodTable, + LockTagHashCode(&lock->tag), wakeupNeeded); + + /* + * If we were holding lock only in AccessShare mode than + * there is nothing to pass to a new PGPROC. + */ + if (proclock->holdMask == 0) + continue; + } + /* * We cannot simply modify proclock->tag.myProc to reassign * ownership of the lock, because that's part of the hash key and diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h index 96927b900d..f3c75e70a6 100644 --- a/src/include/commands/tablecmds.h +++ b/src/include/commands/tablecmds.h @@ -80,6 +80,7 @@ extern void register_on_commit_action(Oid relid, OnCommitAction action); extern void remove_on_commit_action(Oid relid); extern void PreCommit_on_commit_actions(void); +extern void register_temprel_modify(Oid relid); extern void AtEOXact_on_commit_actions(bool isCommit); extern void AtEOSubXact_on_commit_actions(bool isCommit, SubTransactionId mySubid, diff --git a/src/test/regress/expected/prepared_xacts.out b/src/test/regress/expected/prepared_xacts.out index eb77c18788..14ea813342 100644 --- a/src/test/regress/expected/prepared_xacts.out +++ b/src/test/regress/expected/prepared_xacts.out @@ -241,6 +241,36 @@ SELECT * FROM pxtest3; ERROR: relation "pxtest3" does not exist LINE 1: SELECT * FROM pxtest3; ^ +-- Read-only access to temporary table is allowed from a prepared +-- transaction +create temp table twophase_tab_preparable (a int); +begin; +select a from twophase_tab_preparable; + a +--- +(0 rows) + +prepare transaction 'twophase_tab_preparable'; +-- prepared transactions should not hold AccessShare locks +create table twophase_tab_testdrop1 (a int); +create table twophase_tab_testdrop2 (a int); +begin; +select a from twophase_tab_testdrop1; + a +--- +(0 rows) + +select a from twophase_tab_testdrop2; + a +--- +(0 rows) + +prepare transaction 'twophase_tab_testdrop'; +drop table twophase_tab_testdrop1; +\c - +drop table twophase_tab_testdrop2; +commit prepared 'twophase_tab_preparable'; +commit prepared 'twophase_tab_testdrop'; -- There should be no prepared transactions SELECT gid FROM pg_prepared_xacts; gid diff --git a/src/test/regress/expected/prepared_xacts_1.out b/src/test/regress/expected/prepared_xacts_1.out index 0857d259e0..b7f0f56818 100644 --- a/src/test/regress/expected/prepared_xacts_1.out +++ b/src/test/regress/expected/prepared_xacts_1.out @@ -235,6 +235,42 @@ SELECT * FROM pxtest3; ----- (0 rows) +-- Read-only access to temporary table is allowed from a prepared +-- transaction +create temp table twophase_tab_preparable (a int); +begin; +select a from twophase_tab_preparable; + a +--- +(0 rows) + +prepare transaction 'twophase_tab_preparable'; +ERROR: prepared transactions are disabled +HINT: Set max_prepared_transactions to a nonzero value. +-- prepared transactions should not hold AccessShare locks +create table twophase_tab_testdrop1 (a int); +create table twophase_tab_testdrop2 (a int); +begin; +select a from twophase_tab_testdrop1; + a +--- +(0 rows) + +select a from twophase_tab_testdrop2; + a +--- +(0 rows) + +prepare transaction 'twophase_tab_testdrop'; +ERROR: prepared transactions are disabled +HINT: Set max_prepared_transactions to a nonzero value. +drop table twophase_tab_testdrop1; +\c - +drop table twophase_tab_testdrop2; +commit prepared 'twophase_tab_preparable'; +ERROR: prepared transaction with identifier "twophase_tab_preparable" does not exist +commit prepared 'twophase_tab_testdrop'; +ERROR: prepared transaction with identifier "twophase_tab_testdrop" does not exist -- There should be no prepared transactions SELECT gid FROM pg_prepared_xacts; gid diff --git a/src/test/regress/expected/temp.out b/src/test/regress/expected/temp.out index ad7d558191..1f4a342f75 100644 --- a/src/test/regress/expected/temp.out +++ b/src/test/regress/expected/temp.out @@ -336,17 +336,9 @@ begin; create sequence pg_temp.twophase_seq; prepare transaction 'twophase_sequence'; ERROR: cannot PREPARE a transaction that has operated on temporary objects --- Temporary tables cannot be used with two-phase commit. +-- Temporary tables cannot be modified by prepared transaction. create temp table twophase_tab (a int); begin; -select a from twophase_tab; - a ---- -(0 rows) - -prepare transaction 'twophase_tab'; -ERROR: cannot PREPARE a transaction that has operated on temporary objects -begin; insert into twophase_tab values (1); prepare transaction 'twophase_tab'; ERROR: cannot PREPARE a transaction that has operated on temporary objects diff --git a/src/test/regress/sql/prepared_xacts.sql b/src/test/regress/sql/prepared_xacts.sql index d8249a27dc..59c5435239 100644 --- a/src/test/regress/sql/prepared_xacts.sql +++ b/src/test/regress/sql/prepared_xacts.sql @@ -149,6 +149,26 @@ SELECT gid FROM pg_prepared_xacts; COMMIT PREPARED 'regress-two'; SELECT * FROM pxtest3; +-- Read-only access to temporary table is allowed from a prepared +-- transaction +create temp table twophase_tab_preparable (a int); +begin; +select a from twophase_tab_preparable; +prepare transaction 'twophase_tab_preparable'; + +-- prepared transactions should not hold AccessShare locks +create table twophase_tab_testdrop1 (a int); +create table twophase_tab_testdrop2 (a int); +begin; +select a from twophase_tab_testdrop1; +select a from twophase_tab_testdrop2; +prepare transaction 'twophase_tab_testdrop'; +drop table twophase_tab_testdrop1; +\c - +drop table twophase_tab_testdrop2; +commit prepared 'twophase_tab_preparable'; +commit prepared 'twophase_tab_testdrop'; + -- There should be no prepared transactions SELECT gid FROM pg_prepared_xacts; diff --git a/src/test/regress/sql/temp.sql b/src/test/regress/sql/temp.sql index e634ddb9ca..4643278d84 100644 --- a/src/test/regress/sql/temp.sql +++ b/src/test/regress/sql/temp.sql @@ -257,12 +257,9 @@ begin; create sequence pg_temp.twophase_seq; prepare transaction 'twophase_sequence'; --- Temporary tables cannot be used with two-phase commit. +-- Temporary tables cannot be modified by prepared transaction. create temp table twophase_tab (a int); begin; -select a from twophase_tab; -prepare transaction 'twophase_tab'; -begin; insert into twophase_tab values (1); prepare transaction 'twophase_tab'; begin;