From e5550d5fec66aa74caad1f79b79826ec64898688 Mon Sep 17 00:00:00 2001 From: Simon Riggs Date: Sun, 6 Apr 2014 11:13:43 -0400 Subject: [PATCH] Reduce lock levels of some ALTER TABLE cmds VALIDATE CONSTRAINT CLUSTER ON SET WITHOUT CLUSTER ALTER COLUMN SET STATISTICS ALTER COLUMN SET () ALTER COLUMN RESET () All other sub-commands use AccessExclusiveLock Simon Riggs and Noah Misch Reviews by Robert Haas and Andres Freund --- doc/src/sgml/mvcc.sgml | 9 +- doc/src/sgml/ref/alter_table.sgml | 38 ++++- src/backend/catalog/toasting.c | 51 +++++-- src/backend/commands/cluster.c | 14 +- src/backend/commands/createas.c | 4 +- src/backend/commands/tablecmds.c | 174 ++++++++++++++-------- src/backend/tcop/utility.c | 4 +- src/backend/utils/adt/ruleutils.c | 33 +++- src/backend/utils/cache/relcache.c | 74 ++++++++- src/include/catalog/toasting.h | 8 +- src/test/isolation/isolation_schedule | 1 + src/test/regress/expected/alter_table.out | 81 ++++++++-- src/test/regress/sql/alter_table.sql | 29 ++++ 13 files changed, 401 insertions(+), 119 deletions(-) diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml index 2ca423ce8a..12b7814bfd 100644 --- a/doc/src/sgml/mvcc.sgml +++ b/doc/src/sgml/mvcc.sgml @@ -865,7 +865,9 @@ ERROR: could not serialize access due to read/write dependencies among transact Acquired by VACUUM (without ), ANALYZE, CREATE INDEX CONCURRENTLY, and - some forms of ALTER TABLE. + ALTER TABLE VALIDATE and other + ALTER TABLE variants (for full details see + ). @@ -951,10 +953,11 @@ ERROR: could not serialize access due to read/write dependencies among transact - Acquired by the ALTER TABLE, DROP TABLE, + Acquired by the DROP TABLE, TRUNCATE, REINDEX, CLUSTER, and VACUUM FULL - commands. + commands. Many forms of ALTER TABLE also acquire + a lock at this level (see ). This is also the default lock mode for LOCK TABLE statements that do not specify a mode explicitly. diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index f0a8b8650c..0b08f83ba3 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -84,7 +84,10 @@ ALTER TABLE [ IF EXISTS ] name ALTER TABLE changes the definition of an existing table. - There are several subforms: + There are several subforms described below. Note that the lock level required + may differ for each subform. An ACCESS EXCLUSIVE lock is held + unless explicitly noted. When multiple subcommands are listed, the lock + held will be the strictest one required from any subcommand. @@ -181,6 +184,9 @@ ALTER TABLE [ IF EXISTS ] name PostgreSQL query planner, refer to . + + SET STATISTICS acquires a SHARE UPDATE EXCLUSIVE lock. + @@ -213,6 +219,10 @@ ALTER TABLE [ IF EXISTS ] name of statistics by the PostgreSQL query planner, refer to . + + Changing per-attribute options acquires a + SHARE UPDATE EXCLUSIVE lock. + @@ -338,11 +348,17 @@ ALTER TABLE [ IF EXISTS ] name Nothing happens if the constraint is already marked valid. - Validation can be a long process on larger tables and currently requires - an ACCESS EXCLUSIVE lock. The value of separating + Validation can be a long process on larger tables. The value of separating validation from initial creation is that you can defer validation to less busy times, or can be used to give additional time to correct pre-existing - errors while preventing new errors. + errors while preventing new errors. Note also that validation on its own + does not prevent normal write commands against the table while it runs. + + + Validation acquires only a SHARE UPDATE EXCLUSIVE lock + on the table being altered. If the constraint is a foreign key then + a ROW SHARE lock is also required on + the table referenced by the constraint. @@ -408,6 +424,9 @@ ALTER TABLE [ IF EXISTS ] name operations. It does not actually re-cluster the table. + + Changing cluster options acquires a SHARE UPDATE EXCLUSIVE lock. + @@ -420,6 +439,9 @@ ALTER TABLE [ IF EXISTS ] name index specification from the table. This affects future cluster operations that don't specify an index. + + Changing cluster options acquires a SHARE UPDATE EXCLUSIVE lock. + @@ -1078,6 +1100,14 @@ ALTER TABLE distributors ADD CONSTRAINT distfk FOREIGN KEY (address) REFERENCES + + To add a foreign key constraint to a table with the least impact on other work: + +ALTER TABLE distributors ADD CONSTRAINT distfk FOREIGN KEY (address) REFERENCES addresses (address) NOT VALID; +ALTER TABLE distributors VALIDATE CONSTRAINT distfk; + + + To add a (multicolumn) unique constraint to a table: diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c index f044280dba..5275e4bfdb 100644 --- a/src/backend/catalog/toasting.c +++ b/src/backend/catalog/toasting.c @@ -27,6 +27,7 @@ #include "catalog/toasting.h" #include "miscadmin.h" #include "nodes/makefuncs.h" +#include "storage/lock.h" #include "utils/builtins.h" #include "utils/rel.h" #include "utils/syscache.h" @@ -34,13 +35,15 @@ /* Potentially set by contrib/pg_upgrade_support functions */ Oid binary_upgrade_next_toast_pg_type_oid = InvalidOid; +static void CheckAndCreateToastTable(Oid relOid, Datum reloptions, + LOCKMODE lockmode, bool check); static bool create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid, - Datum reloptions); + Datum reloptions, LOCKMODE lockmode, bool check); static bool needs_toast_table(Relation rel); /* - * AlterTableCreateToastTable + * CreateToastTable variants * If the table needs a toast table, and doesn't already have one, * then create a toast table for it. * @@ -52,21 +55,32 @@ static bool needs_toast_table(Relation rel); * to end with CommandCounterIncrement if it makes any changes. */ void -AlterTableCreateToastTable(Oid relOid, Datum reloptions) +AlterTableCreateToastTable(Oid relOid, Datum reloptions, LOCKMODE lockmode) +{ + CheckAndCreateToastTable(relOid, reloptions, lockmode, true); +} + +void +NewHeapCreateToastTable(Oid relOid, Datum reloptions, LOCKMODE lockmode) +{ + CheckAndCreateToastTable(relOid, reloptions, lockmode, false); +} + +void +NewRelationCreateToastTable(Oid relOid, Datum reloptions) +{ + CheckAndCreateToastTable(relOid, reloptions, AccessExclusiveLock, false); +} + +static void +CheckAndCreateToastTable(Oid relOid, Datum reloptions, LOCKMODE lockmode, bool check) { Relation rel; - /* - * Grab an exclusive lock on the target table, since we'll update its - * pg_class tuple. This is redundant for all present uses, since caller - * will have such a lock already. But the lock is needed to ensure that - * concurrent readers of the pg_class tuple won't have visibility issues, - * so let's be safe. - */ - rel = heap_open(relOid, AccessExclusiveLock); + rel = heap_open(relOid, lockmode); /* create_toast_table does all the work */ - (void) create_toast_table(rel, InvalidOid, InvalidOid, reloptions); + (void) create_toast_table(rel, InvalidOid, InvalidOid, reloptions, lockmode, check); heap_close(rel, NoLock); } @@ -91,7 +105,8 @@ BootstrapToastTable(char *relName, Oid toastOid, Oid toastIndexOid) relName))); /* create_toast_table does all the work */ - if (!create_toast_table(rel, toastOid, toastIndexOid, (Datum) 0)) + if (!create_toast_table(rel, toastOid, toastIndexOid, (Datum) 0, + AccessExclusiveLock, false)) elog(ERROR, "\"%s\" does not require a toast table", relName); @@ -107,7 +122,8 @@ BootstrapToastTable(char *relName, Oid toastOid, Oid toastIndexOid) * bootstrap they can be nonzero to specify hand-assigned OIDs */ static bool -create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid, Datum reloptions) +create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid, + Datum reloptions, LOCKMODE lockmode, bool check) { Oid relOid = RelationGetRelid(rel); HeapTuple reltup; @@ -160,6 +176,13 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid, Datum reloptio !OidIsValid(binary_upgrade_next_toast_pg_class_oid))) return false; + /* + * If requested check lockmode is sufficient. This is a cross check + * in case of errors or conflicting decisions in earlier code. + */ + if (check && lockmode != AccessExclusiveLock) + elog(ERROR, "AccessExclusiveLock required to add toast table."); + /* * Create the toast table and its index */ diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 620f071aaf..4ac1e0b864 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -408,10 +408,10 @@ cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose) /* * Verify that the specified heap and index are valid to cluster on * - * Side effect: obtains exclusive lock on the index. The caller should - * already have exclusive lock on the table, so the index lock is likely - * redundant, but it seems best to grab it anyway to ensure the index - * definition can't change under us. + * Side effect: obtains lock on the index. The caller may + * in some cases already have AccessExclusiveLock on the table, but + * not in all cases so we can't rely on the table-level lock for + * protection here. */ void check_index_is_clusterable(Relation OldHeap, Oid indexOid, bool recheck, LOCKMODE lockmode) @@ -696,10 +696,10 @@ make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp, * * If the relation doesn't have a TOAST table already, we can't need one * for the new relation. The other way around is possible though: if some - * wide columns have been dropped, AlterTableCreateToastTable can decide + * wide columns have been dropped, NewHeapCreateToastTable can decide * that no TOAST table is needed for the new table. * - * Note that AlterTableCreateToastTable ends with CommandCounterIncrement, + * Note that NewHeapCreateToastTable ends with CommandCounterIncrement, * so that the TOAST table will be visible for insertion. */ toastid = OldHeap->rd_rel->reltoastrelid; @@ -714,7 +714,7 @@ make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp, if (isNull) reloptions = (Datum) 0; - AlterTableCreateToastTable(OIDNewHeap, reloptions); + NewHeapCreateToastTable(OIDNewHeap, reloptions, lockmode); ReleaseSysCache(tuple); } diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c index 73e6e20985..e434d38702 100644 --- a/src/backend/commands/createas.c +++ b/src/backend/commands/createas.c @@ -359,7 +359,7 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo) /* * If necessary, create a TOAST table for the target table. Note that - * AlterTableCreateToastTable ends with CommandCounterIncrement(), so that + * NewRelationCreateToastTable ends with CommandCounterIncrement(), so that * the TOAST table will be visible for insertion. */ CommandCounterIncrement(); @@ -373,7 +373,7 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo) (void) heap_reloptions(RELKIND_TOASTVALUE, toast_options, true); - AlterTableCreateToastTable(intoRelationId, toast_options); + NewRelationCreateToastTable(intoRelationId, toast_options); /* Create the "view" part of a materialized view. */ if (is_matview) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 7f3f730a87..8f9e5e5607 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -2685,10 +2685,8 @@ AlterTableLookupRelation(AlterTableStmt *stmt, LOCKMODE lockmode) * the whole operation; we don't have to do anything special to clean up. * * The caller must lock the relation, with an appropriate lock level - * for the subcommands requested. Any subcommand that needs to rewrite - * tuples in the table forces the whole command to be executed with - * AccessExclusiveLock (actually, that is currently required always, but - * we hope to relax it at some point). We pass the lock level down + * for the subcommands requested, using AlterTableGetLockLevel(stmt->cmds) + * or higher. We pass the lock level down * so that we can apply it recursively to inherited tables. Note that the * lock level we want as we recurse might well be higher than required for * that specific subcommand. So we pass down the overall lock requirement, @@ -2750,35 +2748,24 @@ AlterTableInternal(Oid relid, List *cmds, bool recurse) * ALTER TABLE RENAME which is treated as a different statement type T_RenameStmt * and does not travel through this section of code and cannot be combined with * any of the subcommands given here. + * + * Note that Hot Standby only knows about AccessExclusiveLocks on the master + * so any changes that might affect SELECTs running on standbys need to use + * AccessExclusiveLocks even if you think a lesser lock would do, unless you + * have a solution for that also. + * + * Also note that pg_dump uses only an AccessShareLock, meaning that anything + * that takes a lock less than AccessExclusiveLock can change object definitions + * while pg_dump is running. Be careful to check that the appropriate data is + * derived by pg_dump using an MVCC snapshot, rather than syscache lookups, + * otherwise we might end up with an inconsistent dump that can't restore. */ LOCKMODE AlterTableGetLockLevel(List *cmds) { /* - * Late in 9.1 dev cycle a number of issues were uncovered with access to - * catalog relations, leading to the decision to re-enforce all DDL at - * AccessExclusiveLock level by default. - * - * The issues are that there is a pervasive assumption in the code that - * the catalogs will not be read unless an AccessExclusiveLock is held. If - * that rule is relaxed, we must protect against a number of potential - * effects - infrequent, but proven possible with test cases where - * multiple DDL operations occur in a stream against frequently accessed - * tables. - * - * 1. Catalog tables were read using SnapshotNow, which has a race bug that - * allows a scan to return no valid rows even when one is present in the - * case of a commit of a concurrent update of the catalog table. - * SnapshotNow also ignores transactions in progress, so takes the latest - * committed version without waiting for the latest changes. - * - * 2. Relcache needs to be internally consistent, so unless we lock the - * definition during reads we have no way to guarantee that. - * - * 3. Catcache access isn't coordinated at all so refreshes can occur at - * any time. + * This only works if we read catalog tables using MVCC snapshots. */ -#ifdef REDUCED_ALTER_TABLE_LOCK_LEVELS ListCell *lcmd; LOCKMODE lockmode = ShareUpdateExclusiveLock; @@ -2790,28 +2777,58 @@ AlterTableGetLockLevel(List *cmds) switch (cmd->subtype) { /* - * Need AccessExclusiveLock for these subcommands because they - * affect or potentially affect both read and write - * operations. - * - * New subcommand types should be added here by default. + * These subcommands rewrite the heap, so require full locks. */ case AT_AddColumn: /* may rewrite heap, in some cases and visible * to SELECT */ - case AT_DropColumn: /* change visible to SELECT */ - case AT_AddColumnToView: /* CREATE VIEW */ + case AT_SetTableSpace: /* must rewrite heap */ case AT_AlterColumnType: /* must rewrite heap */ + case AT_AddOids: /* must rewrite heap */ + cmd_lockmode = AccessExclusiveLock; + break; + + /* + * These subcommands may require addition of toast tables. If we + * add a toast table to a table currently being scanned, we + * might miss data added to the new toast table by concurrent + * insert transactions. + */ + case AT_SetStorage: /* may add toast tables, see ATRewriteCatalogs() */ + cmd_lockmode = AccessExclusiveLock; + break; + + /* + * Removing constraints can affect SELECTs that have been + * optimised assuming the constraint holds true. + */ case AT_DropConstraint: /* as DROP INDEX */ - case AT_AddOids: /* must rewrite heap */ - case AT_DropOids: /* calls AT_DropColumn */ + case AT_DropNotNull: /* may change some SQL plans */ + cmd_lockmode = AccessExclusiveLock; + break; + + /* + * Subcommands that may be visible to concurrent SELECTs + */ + case AT_DropColumn: /* change visible to SELECT */ + case AT_AddColumnToView: /* CREATE VIEW */ + case AT_DropOids: /* calls AT_DropColumn */ case AT_EnableAlwaysRule: /* may change SELECT rules */ case AT_EnableReplicaRule: /* may change SELECT rules */ - case AT_EnableRule: /* may change SELECT rules */ + case AT_EnableRule: /* may change SELECT rules */ case AT_DisableRule: /* may change SELECT rules */ + cmd_lockmode = AccessExclusiveLock; + break; + + /* + * Changing owner may remove implicit SELECT privileges + */ case AT_ChangeOwner: /* change visible to SELECT */ - case AT_SetTableSpace: /* must rewrite heap */ - case AT_DropNotNull: /* may change some SQL plans */ - case AT_SetNotNull: + cmd_lockmode = AccessExclusiveLock; + break; + + /* + * Changing foreign table options may affect optimisation. + */ case AT_GenericOptions: case AT_AlterColumnGenericOptions: cmd_lockmode = AccessExclusiveLock; @@ -2819,6 +2836,7 @@ AlterTableGetLockLevel(List *cmds) /* * These subcommands affect write operations only. + * XXX Theoretically, these could be ShareRowExclusiveLock. */ case AT_ColumnDefault: case AT_ProcessedConstraint: /* becomes AT_AddConstraint */ @@ -2832,10 +2850,12 @@ AlterTableGetLockLevel(List *cmds) case AT_DisableTrig: case AT_DisableTrigAll: case AT_DisableTrigUser: + case AT_AlterConstraint: case AT_AddIndex: /* from ADD CONSTRAINT */ case AT_AddIndexConstraint: case AT_ReplicaIdentity: - cmd_lockmode = ShareRowExclusiveLock; + case AT_SetNotNull: + cmd_lockmode = AccessExclusiveLock; break; case AT_AddConstraint: @@ -2854,8 +2874,10 @@ AlterTableGetLockLevel(List *cmds) * could reduce the lock strength to ShareLock if * we can work out how to allow concurrent catalog * updates. + * XXX Might be set down to ShareRowExclusiveLock + * but requires further analysis. */ - cmd_lockmode = ShareRowExclusiveLock; + cmd_lockmode = AccessExclusiveLock; break; case CONSTR_FOREIGN: @@ -2863,12 +2885,15 @@ AlterTableGetLockLevel(List *cmds) * We add triggers to both tables when we add a * Foreign Key, so the lock level must be at least * as strong as CREATE TRIGGER. + * XXX Might be set down to ShareRowExclusiveLock + * though trigger info is accessed by + * pg_get_triggerdef */ - cmd_lockmode = ShareRowExclusiveLock; + cmd_lockmode = AccessExclusiveLock; break; default: - cmd_lockmode = ShareRowExclusiveLock; + cmd_lockmode = AccessExclusiveLock; } } break; @@ -2879,22 +2904,31 @@ AlterTableGetLockLevel(List *cmds) * behaviour, while queries started after we commit will see * new behaviour. No need to prevent reads or writes to the * subtable while we hook it up though. + * Changing the TupDesc may be a problem, so keep highest lock. */ case AT_AddInherit: case AT_DropInherit: - cmd_lockmode = ShareUpdateExclusiveLock; + cmd_lockmode = AccessExclusiveLock; break; /* * These subcommands affect implicit row type conversion. They - * have affects similar to CREATE/DROP CAST on queries. We + * have affects similar to CREATE/DROP CAST on queries. * don't provide for invalidating parse trees as a result of - * such changes. Do avoid concurrent pg_class updates, - * though. + * such changes, so we keep these at AccessExclusiveLock. */ case AT_AddOf: case AT_DropOf: - cmd_lockmode = ShareUpdateExclusiveLock; + cmd_lockmode = AccessExclusiveLock; + break; + + /* + * Only used by CREATE OR REPLACE VIEW which must conflict + * with an SELECTs currently using the view. + */ + case AT_ReplaceRelOptions: + cmd_lockmode = AccessExclusiveLock; + break; /* * These subcommands affect general strategies for performance @@ -2906,20 +2940,33 @@ AlterTableGetLockLevel(List *cmds) * applies: we don't currently allow concurrent catalog * updates. */ - case AT_SetStatistics: - case AT_ClusterOn: - case AT_DropCluster: - case AT_SetRelOptions: - case AT_ResetRelOptions: - case AT_ReplaceRelOptions: - case AT_SetOptions: - case AT_ResetOptions: - case AT_SetStorage: - case AT_AlterConstraint: - case AT_ValidateConstraint: + case AT_SetStatistics: /* Uses MVCC in getTableAttrs() */ + case AT_ClusterOn: /* Uses MVCC in getIndexes() */ + case AT_DropCluster: /* Uses MVCC in getIndexes() */ + case AT_SetOptions: /* Uses MVCC in getTableAttrs() */ + case AT_ResetOptions: /* Uses MVCC in getTableAttrs() */ cmd_lockmode = ShareUpdateExclusiveLock; break; + case AT_ValidateConstraint: /* Uses MVCC in getConstraints() */ + cmd_lockmode = ShareUpdateExclusiveLock; + break; + + /* + * Rel options are more complex than first appears. Options + * are set here for tables, views and indexes; for historical + * reasons these can all be used with ALTER TABLE, so we + * can't decide between them using the basic grammar. + * + * XXX Look in detail at each option to determine lock level, + * e.g. + * cmd_lockmode = GetRelOptionsLockLevel((List *) cmd->def); + */ + case AT_SetRelOptions: /* Uses MVCC in getIndexes() and getTables() */ + case AT_ResetRelOptions: /* Uses MVCC in getIndexes() and getTables() */ + cmd_lockmode = AccessExclusiveLock; + break; + default: /* oops */ elog(ERROR, "unrecognized alter table type: %d", (int) cmd->subtype); @@ -2932,9 +2979,6 @@ AlterTableGetLockLevel(List *cmds) if (cmd_lockmode > lockmode) lockmode = cmd_lockmode; } -#else - LOCKMODE lockmode = AccessExclusiveLock; -#endif return lockmode; } @@ -3272,7 +3316,7 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode) if (tab->relkind == RELKIND_RELATION || tab->relkind == RELKIND_MATVIEW) - AlterTableCreateToastTable(tab->relid, (Datum) 0); + AlterTableCreateToastTable(tab->relid, (Datum) 0, lockmode); } } @@ -3586,7 +3630,7 @@ ATRewriteTables(List **wqueue, LOCKMODE lockmode) /* Create transient table that will receive the modified data */ OIDNewHeap = make_new_heap(tab->relid, NewTableSpace, false, - AccessExclusiveLock); + lockmode); /* * Copy the heap data into the new table with the desired diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index d1621add7d..1846570a3e 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -908,7 +908,7 @@ ProcessUtilitySlow(Node *parsetree, InvalidOid); /* - * Let AlterTableCreateToastTable decide if this + * Let NewRelationCreateToastTable decide if this * one needs a secondary relation too. */ CommandCounterIncrement(); @@ -927,7 +927,7 @@ ProcessUtilitySlow(Node *parsetree, toast_options, true); - AlterTableCreateToastTable(relOid, toast_options); + NewRelationCreateToastTable(relOid, toast_options); } else if (IsA(stmt, CreateForeignTableStmt)) { diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index b1bac866aa..ea7b8c5942 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -54,6 +54,7 @@ #include "utils/fmgroids.h" #include "utils/lsyscache.h" #include "utils/rel.h" +#include "utils/snapmgr.h" #include "utils/syscache.h" #include "utils/tqual.h" #include "utils/typcache.h" @@ -1284,6 +1285,9 @@ pg_get_constraintdef_string(Oid constraintId) return pg_get_constraintdef_worker(constraintId, true, 0); } +/* + * As of 9.4, we now use an MVCC snapshot for this. + */ static char * pg_get_constraintdef_worker(Oid constraintId, bool fullCommand, int prettyFlags) @@ -1291,10 +1295,34 @@ pg_get_constraintdef_worker(Oid constraintId, bool fullCommand, HeapTuple tup; Form_pg_constraint conForm; StringInfoData buf; + SysScanDesc scandesc; + ScanKeyData scankey[1]; + Snapshot snapshot = RegisterSnapshot(GetTransactionSnapshot()); + Relation relation = heap_open(ConstraintRelationId, AccessShareLock); + + ScanKeyInit(&scankey[0], + ObjectIdAttributeNumber, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(constraintId)); + + scandesc = systable_beginscan(relation, + ConstraintOidIndexId, + true, + snapshot, + 1, + scankey); + + /* + * We later use the tuple with SysCacheGetAttr() as if we + * had obtained it via SearchSysCache, which works fine. + */ + tup = systable_getnext(scandesc); + + UnregisterSnapshot(snapshot); - tup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(constraintId)); if (!HeapTupleIsValid(tup)) /* should not happen */ elog(ERROR, "cache lookup failed for constraint %u", constraintId); + conForm = (Form_pg_constraint) GETSTRUCT(tup); initStringInfo(&buf); @@ -1575,7 +1603,8 @@ pg_get_constraintdef_worker(Oid constraintId, bool fullCommand, appendStringInfoString(&buf, " NOT VALID"); /* Cleanup */ - ReleaseSysCache(tup); + systable_endscan(scandesc); + heap_close(relation, AccessShareLock); return buf.data; } diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index 32313244ad..c8cea028d4 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -162,6 +162,14 @@ static bool eoxact_list_overflowed = false; eoxact_list_overflowed = true; \ } while (0) +/* + * EOXactTupleDescArray stores TupleDescs that (might) need AtEOXact + * cleanup work. The array expands as needed; there is no hashtable because + * we don't need to access individual items except at EOXact. + */ +static TupleDesc *EOXactTupleDescArray; +static int NextEOXactTupleDescNum = 0; +static int EOXactTupleDescArrayLen = 0; /* * macros to manipulate the lookup hashtables @@ -220,11 +228,12 @@ static HTAB *OpClassCache = NULL; /* non-export function prototypes */ -static void RelationDestroyRelation(Relation relation); +static void RelationDestroyRelation(Relation relation, bool remember_tupdesc); static void RelationClearRelation(Relation relation, bool rebuild); static void RelationReloadIndexInfo(Relation relation); static void RelationFlushRelation(Relation relation); +static void RememberToFreeTupleDescAtEOX(TupleDesc td); static void AtEOXact_cleanup(Relation relation, bool isCommit); static void AtEOSubXact_cleanup(Relation relation, bool isCommit, SubTransactionId mySubid, SubTransactionId parentSubid); @@ -1858,7 +1867,7 @@ RelationReloadIndexInfo(Relation relation) * Caller must already have unhooked the entry from the hash table. */ static void -RelationDestroyRelation(Relation relation) +RelationDestroyRelation(Relation relation, bool remember_tupdesc) { Assert(RelationHasReferenceCountZero(relation)); @@ -1878,7 +1887,20 @@ RelationDestroyRelation(Relation relation) /* can't use DecrTupleDescRefCount here */ Assert(relation->rd_att->tdrefcount > 0); if (--relation->rd_att->tdrefcount == 0) - FreeTupleDesc(relation->rd_att); + { + /* + * If we Rebuilt a relcache entry during a transaction then its + * possible we did that because the TupDesc changed as the result + * of an ALTER TABLE that ran at less than AccessExclusiveLock. + * It's possible someone copied that TupDesc, in which case the + * copy would point to free'd memory. So if we rebuild an entry + * we keep the TupDesc around until end of transaction, to be safe. + */ + if (remember_tupdesc) + RememberToFreeTupleDescAtEOX(relation->rd_att); + else + FreeTupleDesc(relation->rd_att); + } list_free(relation->rd_indexlist); bms_free(relation->rd_indexattr); FreeTriggerDesc(relation->trigdesc); @@ -1992,7 +2014,7 @@ RelationClearRelation(Relation relation, bool rebuild) RelationCacheDelete(relation); /* And release storage */ - RelationDestroyRelation(relation); + RelationDestroyRelation(relation, false); } else if (!IsTransactionState()) { @@ -2059,7 +2081,7 @@ RelationClearRelation(Relation relation, bool rebuild) { /* Should only get here if relation was deleted */ RelationCacheDelete(relation); - RelationDestroyRelation(relation); + RelationDestroyRelation(relation, false); elog(ERROR, "relation %u deleted while still in use", save_relid); } @@ -2121,7 +2143,7 @@ RelationClearRelation(Relation relation, bool rebuild) #undef SWAPFIELD /* And now we can throw away the temporary entry */ - RelationDestroyRelation(newrel); + RelationDestroyRelation(newrel, !keep_tupdesc); } } @@ -2359,6 +2381,33 @@ RelationCloseSmgrByOid(Oid relationId) RelationCloseSmgr(relation); } +void +RememberToFreeTupleDescAtEOX(TupleDesc td) +{ + if (EOXactTupleDescArray == NULL) + { + MemoryContext oldcxt; + oldcxt = MemoryContextSwitchTo(CacheMemoryContext); + + EOXactTupleDescArray = (TupleDesc *) palloc(16 * sizeof(TupleDesc)); + EOXactTupleDescArrayLen = 16; + NextEOXactTupleDescNum = 0; + MemoryContextSwitchTo(oldcxt); + } + else if (NextEOXactTupleDescNum >= EOXactTupleDescArrayLen) + { + int32 newlen = EOXactTupleDescArrayLen * 2; + + Assert(EOXactTupleDescArrayLen > 0); + + EOXactTupleDescArray = (TupleDesc *) repalloc(EOXactTupleDescArray, + newlen * sizeof(TupleDesc)); + EOXactTupleDescArrayLen = newlen; + } + + EOXactTupleDescArray[NextEOXactTupleDescNum++] = td; +} + /* * AtEOXact_RelationCache * @@ -2414,9 +2463,20 @@ AtEOXact_RelationCache(bool isCommit) } } - /* Now we're out of the transaction and can clear the list */ + if (EOXactTupleDescArrayLen > 0) + { + Assert(EOXactTupleDescArray != NULL); + for (i = 0; i < NextEOXactTupleDescNum; i++) + FreeTupleDesc(EOXactTupleDescArray[i]); + pfree(EOXactTupleDescArray); + EOXactTupleDescArray = NULL; + } + + /* Now we're out of the transaction and can clear the lists */ eoxact_list_len = 0; eoxact_list_overflowed = false; + NextEOXactTupleDescNum = 0; + EOXactTupleDescArrayLen = 0; } /* diff --git a/src/include/catalog/toasting.h b/src/include/catalog/toasting.h index ca047f0618..0947760118 100644 --- a/src/include/catalog/toasting.h +++ b/src/include/catalog/toasting.h @@ -14,10 +14,16 @@ #ifndef TOASTING_H #define TOASTING_H +#include "storage/lock.h" + /* * toasting.c prototypes */ -extern void AlterTableCreateToastTable(Oid relOid, Datum reloptions); +extern void NewRelationCreateToastTable(Oid relOid, Datum reloptions); +extern void NewHeapCreateToastTable(Oid relOid, Datum reloptions, + LOCKMODE lockmode); +extern void AlterTableCreateToastTable(Oid relOid, Datum reloptions, + LOCKMODE lockmode); extern void BootstrapToastTable(char *relName, Oid toastOid, Oid toastIndexOid); diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index 1e73b4aebd..36acec1ca8 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -23,4 +23,5 @@ test: multixact-no-deadlock test: multixact-no-forget test: propagate-lock-delete test: drop-index-concurrently-1 +test: alter-table-1 test: timeouts diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index e2279e63c1..a182176ba6 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -1840,28 +1840,31 @@ and relnamespace != (select oid from pg_namespace where nspname = 'pg_catalog') and c.relname != 'my_locks' group by c.relname; create table alterlock (f1 int primary key, f2 text); +insert into alterlock values (1, 'foo'); +create table alterlock2 (f3 int primary key, f1 int); +insert into alterlock2 values (1, 1); begin; alter table alterlock alter column f2 set statistics 150; select * from my_locks order by 1; - relname | max_lockmode ------------+--------------------- - alterlock | AccessExclusiveLock + relname | max_lockmode +-----------+-------------------------- + alterlock | ShareUpdateExclusiveLock (1 row) rollback; begin; alter table alterlock cluster on alterlock_pkey; select * from my_locks order by 1; - relname | max_lockmode -----------------+--------------------- - alterlock | AccessExclusiveLock - alterlock_pkey | AccessExclusiveLock + relname | max_lockmode +----------------+-------------------------- + alterlock | ShareUpdateExclusiveLock + alterlock_pkey | ShareUpdateExclusiveLock (2 rows) commit; begin; alter table alterlock set without cluster; select * from my_locks order by 1; - relname | max_lockmode ------------+--------------------- - alterlock | AccessExclusiveLock + relname | max_lockmode +-----------+-------------------------- + alterlock | ShareUpdateExclusiveLock (1 row) commit; @@ -1902,6 +1905,14 @@ select * from my_locks order by 1; commit; begin; alter table alterlock alter column f2 set (n_distinct = 1); +select * from my_locks order by 1; + relname | max_lockmode +-----------+-------------------------- + alterlock | ShareUpdateExclusiveLock +(1 row) + +rollback; +begin; alter table alterlock alter column f2 set storage extended; select * from my_locks order by 1; relname | max_lockmode -----------+--------------------- @@ -1909,7 +1920,7 @@ select * from my_locks order by 1; (1 row) rollback; -begin; alter table alterlock alter column f2 set storage extended; +begin; alter table alterlock alter column f2 set default 'x'; select * from my_locks order by 1; relname | max_lockmode -----------+--------------------- @@ -1917,15 +1928,61 @@ select * from my_locks order by 1; (1 row) rollback; -begin; alter table alterlock alter column f2 set default 'x'; +begin; +create trigger ttdummy + before delete or update on alterlock + for each row + execute procedure + ttdummy (1, 1); select * from my_locks order by 1; relname | max_lockmode -----------+--------------------- alterlock | AccessExclusiveLock (1 row) +rollback; +begin; +select * from my_locks order by 1; + relname | max_lockmode +---------+-------------- +(0 rows) + +alter table alterlock2 add foreign key (f1) references alterlock (f1); +select * from my_locks order by 1; + relname | max_lockmode +-----------------+--------------------- + alterlock | AccessExclusiveLock + alterlock2 | AccessExclusiveLock + alterlock2_pkey | AccessShareLock + alterlock_pkey | AccessShareLock +(4 rows) + +rollback; +begin; +alter table alterlock2 +add constraint alterlock2nv foreign key (f1) references alterlock (f1) NOT VALID; +select * from my_locks order by 1; + relname | max_lockmode +------------+--------------------- + alterlock | AccessExclusiveLock + alterlock2 | AccessExclusiveLock +(2 rows) + +commit; +begin; +alter table alterlock2 validate constraint alterlock2nv; +select * from my_locks order by 1; + relname | max_lockmode +-----------------+-------------------------- + alterlock | RowShareLock + alterlock2 | ShareUpdateExclusiveLock + alterlock2_pkey | AccessShareLock + alterlock_pkey | AccessShareLock +(4 rows) + rollback; -- cleanup +drop table alterlock2; drop table alterlock; drop view my_locks; drop type lockmodes; diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index 2f2c2e3502..3f641f9596 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -1283,6 +1283,9 @@ and c.relname != 'my_locks' group by c.relname; create table alterlock (f1 int primary key, f2 text); +insert into alterlock values (1, 'foo'); +create table alterlock2 (f3 int primary key, f1 int); +insert into alterlock2 values (1, 1); begin; alter table alterlock alter column f2 set statistics 150; select * from my_locks order by 1; @@ -1324,7 +1327,33 @@ begin; alter table alterlock alter column f2 set default 'x'; select * from my_locks order by 1; rollback; +begin; +create trigger ttdummy + before delete or update on alterlock + for each row + execute procedure + ttdummy (1, 1); +select * from my_locks order by 1; +rollback; + +begin; +select * from my_locks order by 1; +alter table alterlock2 add foreign key (f1) references alterlock (f1); +select * from my_locks order by 1; +rollback; + +begin; +alter table alterlock2 +add constraint alterlock2nv foreign key (f1) references alterlock (f1) NOT VALID; +select * from my_locks order by 1; +commit; +begin; +alter table alterlock2 validate constraint alterlock2nv; +select * from my_locks order by 1; +rollback; + -- cleanup +drop table alterlock2; drop table alterlock; drop view my_locks; drop type lockmodes; -- 2.40.0