On 2024-Oct-09, Antonin Houska wrote: > diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml > index 9110938fab..f1008f5013 100644 > --- a/doc/src/sgml/ref/vacuum.sgml > +++ b/doc/src/sgml/ref/vacuum.sgml
> @@ -61,8 +62,12 @@ VACUUM [ ( <replaceable > class="parameter">option</replaceable> [, ...] ) ] [ <re > <para> > Without a <replaceable class="parameter">table_and_columns</replaceable> > list, <command>VACUUM</command> processes every table and materialized > view > - in the current database that the current user has permission to vacuum. > - With a list, <command>VACUUM</command> processes only those table(s). > + in the current database that the current user has permission to vacuum. If > + the <literal>CONCURRENTLY</literal> is specified (see below), tables which > + have not been clustered yet are silently skipped. With a > + list, <command>VACUUM</command> processes only those table(s). If > + the <literal>CONCURRENTLY</literal> is specified, the list may only > contain > + tables which have already been clustered. > </para> The idea that VACUUM CONCURRENTLY can only process tables that have been clustered sounds very strange to me. I don't think such a restriction would really fly. However, I think this may just be a documentation mistake; can you please clarify? I am tempted to suggest that VACUUM CONCURRENTLY should receive a table list; without a list, it should raise an error. This is not supposed to be a routine maintenance command that you can run on all your tables, after all. Heck, maybe don't even accept a partitioned table -- the user can process one partition at a time, if they need that. I don't believe in the need for the LOCK_CLUSTER_CONCURRENT define; IMO the code should just use ShareUpdateExclusiveLock where needed. In 0001, the new API of make_new_heap() is somewhat bizarre regarding the output lockmode_new_p parameter. I didn't find any place in the patch series where we use that to return a different lock level that the caller gave; the only case were we do something that looks funny is when a toast table is involved. But I don't think I fully understand what is going on in that case. I'm likely missing something here, but isn't it simpler to just state that make_new_heap will obtain a lock on the new heap, and that the immediately following table_open needn't acquire a lock (or, in the case of RefreshMatViewByOid, no LockRelationOid is necessary)? Anyway, I propose some cosmetic cleanups for 0001 in attachment, including changing make_new_heap to assume a non-null value of lockmode_new_p. I didn't go as far as making it no longer a pointer, but if it can be done, then I suggest we should do that. I didn't try to apply the next patches in the series after this one. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
>From eec9e6dfc4aa5a4f52a82065e3d4973cdbbff09f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email protected]> 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
