From eec9e6dfc4aa5a4f52a82065e3d4973cdbbff09f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= Date: Fri, 6 Dec 2024 14:39:02 +0100 Subject: [PATCH] Minor code review --- src/backend/commands/cluster.c | 72 ++++++++++++-------------------- src/backend/commands/matview.c | 7 +++- src/backend/commands/tablecmds.c | 5 ++- src/backend/commands/vacuum.c | 5 +-- 4 files changed, 36 insertions(+), 53 deletions(-) diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index e32abf15e69..4a62aff46bd 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -191,13 +191,11 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel) stmt->indexname, stmt->relation->relname))); } + /* For non-partitioned tables, do what we came here to do. */ if (rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE) { - /* - * Do the job. (The function will close the relation, lock is kept - * till commit.) - */ cluster_rel(rel, indexOid, ¶ms); + /* cluster_rel closes the relation, but keeps lock */ return; } @@ -284,11 +282,9 @@ cluster_multiple_rels(List *rtcs, ClusterParams *params) rel = table_open(rtc->tableOid, AccessExclusiveLock); - /* - * Do the job. (The function will close the relation, lock is kept - * till commit.) - */ + /* Process this table */ cluster_rel(rel, rtc->indexOid, params); + /* cluster_rel closes the relation, but keeps lock */ PopActiveSnapshot(); CommitTransactionCommand(); @@ -301,8 +297,7 @@ cluster_multiple_rels(List *rtcs, ClusterParams *params) * This clusters the table by creating a new, clustered table and * swapping the relfilenumbers of the new table and the old table, so * the OID of the original table is preserved. Thus we do not lose - * GRANT, inheritance nor references to this table (this was a bug - * in releases through 7.3). + * GRANT, inheritance nor references to this table. * * Indexes are rebuilt too, via REINDEX. Since we are effectively bulk-loading * the new table, it's better to create the indexes afterwards than to fill @@ -311,8 +306,6 @@ cluster_multiple_rels(List *rtcs, ClusterParams *params) * If indexOid is InvalidOid, the table will be rewritten in physical order * instead of index order. This is the new implementation of VACUUM FULL, * and error messages should refer to the operation as VACUUM not CLUSTER. - * - * We expect that OldHeap is already locked in AccessExclusiveLock mode. */ void cluster_rel(Relation OldHeap, Oid indexOid, ClusterParams *params) @@ -325,6 +318,8 @@ cluster_rel(Relation OldHeap, Oid indexOid, ClusterParams *params) bool recheck = ((params->options & CLUOPT_RECHECK) != 0); Relation index = NULL; + Assert(CheckRelationLockedByMe(OldHeap, AccessExclusiveLock, false)); + /* Check for user-requested abort. */ CHECK_FOR_INTERRUPTS(); @@ -472,11 +467,7 @@ cluster_rel(Relation OldHeap, Oid indexOid, ClusterParams *params) /* rebuild_relation does all the dirty work */ rebuild_relation(OldHeap, index, verbose); - - /* - * NB: rebuild_relation does table_close() on OldHeap, and also on index, - * if the pointer is valid. - */ + /* rebuild_relation closes OldHeap, and index if valid */ out: /* Roll back any GUC changes executed by index functions */ @@ -635,7 +626,6 @@ static void rebuild_relation(Relation OldHeap, Relation index, bool verbose) { Oid tableOid = RelationGetRelid(OldHeap); - Oid indexOid = index ? RelationGetRelid(index) : InvalidOid; Oid accessMethod = OldHeap->rd_rel->relam; Oid tableSpace = OldHeap->rd_rel->reltablespace; Oid OIDNewHeap; @@ -647,9 +637,9 @@ rebuild_relation(Relation OldHeap, Relation index, bool verbose) MultiXactId cutoffMulti; LOCKMODE lockmode_new; - if (OidIsValid(indexOid)) + if (index) /* Mark the correct index as clustered */ - mark_index_clustered(OldHeap, indexOid, true); + mark_index_clustered(OldHeap, RelationGetRelid(index), true); /* Remember info about rel before closing OldHeap */ relpersistence = OldHeap->rd_rel->relpersistence; @@ -666,10 +656,8 @@ rebuild_relation(Relation OldHeap, Relation index, bool verbose) accessMethod, relpersistence, NoLock, &lockmode_new); - Assert(lockmode_new == AccessExclusiveLock || lockmode_new == NoLock); - /* Lock iff not done above. */ - NewHeap = table_open(OIDNewHeap, lockmode_new == NoLock ? - AccessExclusiveLock : NoLock); + /* NewHeap already locked by make_new_heap */ + NewHeap = table_open(OIDNewHeap, NoLock); /* Copy the heap data into the new table in the desired order */ copy_table_data(NewHeap, OldHeap, index, verbose, @@ -683,9 +671,8 @@ rebuild_relation(Relation OldHeap, Relation index, bool verbose) /* * Close the new relation so it can be dropped as soon as the storage is - * swapped. The relation is not visible to others, so we could unlock it - * completely, but it's simpler to pass NoLock than to track all the locks - * acquired so far. + * swapped. The relation is not visible to others, so no need to unlock it + * explicitly. */ table_close(NewHeap, NoLock); @@ -710,9 +697,8 @@ rebuild_relation(Relation OldHeap, Relation index, bool verbose) * After this, the caller should load the new heap with transferred/modified * data, then call finish_heap_swap to complete the operation. * - * If a specific lock mode is needed for the new relation, pass it via the - * in/out parameter lockmode_new_p. On exit, the output value tells whether - * the lock was actually acquired. + * Locking: lockmode_old is acquired on OldHeap, if not NoLock; lockmode_new_p + * is acquired on NewHeap. */ Oid make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, Oid NewAccessMethod, @@ -730,13 +716,8 @@ make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, Oid NewAccessMethod, Oid namespaceid; LOCKMODE lockmode_new; - if (lockmode_new_p) - { - lockmode_new = *lockmode_new_p; - *lockmode_new_p = NoLock; - } - else - lockmode_new = lockmode_old; + lockmode_new = *lockmode_new_p; + *lockmode_new_p = NoLock; OldHeap = table_open(OIDOldHeap, lockmode_old); OldHeapDesc = RelationGetDescr(OldHeap); @@ -833,8 +814,7 @@ make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, Oid NewAccessMethod, reloptions = (Datum) 0; NewHeapCreateToastTable(OIDNewHeap, reloptions, lockmode_new, toastid); - if (lockmode_new_p) - *lockmode_new_p = lockmode_new; + *lockmode_new_p = lockmode_new; ReleaseSysCache(tuple); } @@ -857,9 +837,6 @@ copy_table_data(Relation NewHeap, Relation OldHeap, Relation OldIndex, bool verb bool *pSwapToastByContent, TransactionId *pFreezeXid, MultiXactId *pCutoffMulti) { - Oid OIDOldHeap = RelationGetRelid(OldHeap); - Oid OIDOldIndex = OldIndex ? RelationGetRelid(OldIndex) : InvalidOid; - Oid OIDNewHeap = RelationGetRelid(NewHeap); Relation relRelation; HeapTuple reltup; Form_pg_class relform; @@ -978,7 +955,8 @@ copy_table_data(Relation NewHeap, Relation OldHeap, Relation OldIndex, bool verb * provided, else plain seqscan. */ if (OldIndex != NULL && OldIndex->rd_rel->relam == BTREE_AM_OID) - use_sort = plan_cluster_use_sort(OIDOldHeap, OIDOldIndex); + use_sort = plan_cluster_use_sort(RelationGetRelid(OldHeap), + RelationGetRelid(OldIndex)); else use_sort = false; @@ -1036,16 +1014,18 @@ copy_table_data(Relation NewHeap, Relation OldHeap, Relation OldIndex, bool verb /* Update pg_class to reflect the correct values of pages and tuples. */ relRelation = table_open(RelationRelationId, RowExclusiveLock); - reltup = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(OIDNewHeap)); + reltup = SearchSysCacheCopy1(RELOID, + ObjectIdGetDatum(RelationGetRelid(NewHeap))); if (!HeapTupleIsValid(reltup)) - elog(ERROR, "cache lookup failed for relation %u", OIDNewHeap); + elog(ERROR, "cache lookup failed for relation %u", + RelationGetRelid(NewHeap)); relform = (Form_pg_class) GETSTRUCT(reltup); relform->relpages = num_pages; relform->reltuples = num_tuples; /* Don't update the stats for pg_class. See swap_relation_files. */ - if (OIDOldHeap != RelationRelationId) + if (RelationGetRelid(OldHeap) != RelationRelationId) CatalogTupleUpdate(relRelation, &reltup->t_self, reltup); else CacheInvalidateRelcacheByTuple(reltup); diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c index 8eaf951cc16..6b2bcb168b6 100644 --- a/src/backend/commands/matview.c +++ b/src/backend/commands/matview.c @@ -173,6 +173,7 @@ RefreshMatViewByOid(Oid matviewOid, bool is_create, bool skipData, Oid tableSpace; Oid relowner; Oid OIDNewHeap; + LOCKMODE newrel_lock; uint64 processed = 0; char relpersistence; Oid save_userid; @@ -316,10 +317,12 @@ RefreshMatViewByOid(Oid matviewOid, bool is_create, bool skipData, * it against access by any other process until commit (by which time it * will be gone). */ + newrel_lock = AccessExclusiveLock; OIDNewHeap = make_new_heap(matviewOid, tableSpace, matviewRel->rd_rel->relam, - relpersistence, ExclusiveLock, NULL); - LockRelationOid(OIDNewHeap, AccessExclusiveLock); + relpersistence, ExclusiveLock, + &newrel_lock); + /* lock on new rel needn't be explicitly released */ /* Generate the data, if wanted. */ if (!skipData) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index f2aa05a2e7c..f982af8a1d4 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -5799,6 +5799,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode, Oid NewAccessMethod; Oid NewTableSpace; char persistence; + LOCKMODE newrel_lock; OldHeap = table_open(tab->relid, NoLock); @@ -5888,8 +5889,10 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode, * persistence. That wouldn't work for pg_class, but that can't be * unlogged anyway. */ + newrel_lock = lockmode; OIDNewHeap = make_new_heap(tab->relid, NewTableSpace, NewAccessMethod, - persistence, lockmode, NULL); + persistence, lockmode, &newrel_lock); + /* lock on NewHeap needn't be explicitly released */ /* * Copy the heap data into the new table with the desired diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 3c2ec7101d7..a0158b1fcde 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -2223,11 +2223,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params, /* VACUUM FULL is now a variant of CLUSTER; see cluster.c */ cluster_rel(rel, InvalidOid, &cluster_params); + /* cluster_rel closes the relation, but keeps lock */ - /* - * cluster_rel() should have closed the relation, lock is kept - * till commit. - */ rel = NULL; } else -- 2.39.5