On 2024-Mar-08, Michael Paquier wrote:
> I have spent more time reviewing the whole and the tests (I didn't see
> much value in testing the DEFAULT clause twice for the partitioned
> table case and there is a test in d61a6cad6418), tweaked a few
> comments and the documentation, did an indentation and a commit
> message draft.
>
> How does that look to you? The test coverage and the semantics do
> what we want them to do, so that looks rather reasonable here. A
> second or even third pair of eyes would not hurt.
I gave this a look. I found some of the comments a bit confusing or
overly long, so I propose to reword them. I also propose a small doc
change (during writing which I noticed that the docs for tablespace had
been neglected and one comment too many; patch to be committed
separately soon). I ended up also moving code in tablecmds.c so that
all the AT*SetAccessMethod* routines appear together rather than mixed
with the ones for tablespaces, and removing one CCI that seems
unnecessary, at the bottom of ATExecSetAccessMethodNoStorage.
0001 is Michaël's patch, 0002 are my proposed changes.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Hay que recordar que la existencia en el cosmos, y particularmente la
elaboración de civilizaciones dentro de él no son, por desgracia,
nada idílicas" (Ijon Tichy)
>From 8b4408ac6cc46af2257e9c613d95a0eb38238a8e Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Tue, 19 Mar 2024 09:51:58 +0100
Subject: [PATCH] comment updates, remove one CCI
---
doc/src/sgml/catalogs.sgml | 17 ++-
src/backend/commands/tablecmds.c | 223 ++++++++++++++-----------------
2 files changed, 114 insertions(+), 126 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index c86ff19754..476a7b70dd 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1989,9 +1989,12 @@ SCRAM-SHA-256$<replaceable><iteration
count></replaceable>:<replaceable>&l
</para>
<para>
If this is a table or an index, the access method used (heap,
- B-tree, hash, etc.); otherwise zero. Zero occurs for sequences,
- as well as relations without storage, such as views. Partitioned
- tables may use a non-zero value.
+ B-tree, hash, etc.); for partitioned tables, the access method
+ for newly created partitions, when one is not specified in the
+ creation command, or zero to make creation fall back to
+ <varname>default_table_access_method</varname>.
+ Zero for sequences, as well as other relations without
+ storage, such as views.
</para></entry>
</row>
@@ -2012,9 +2015,11 @@ SCRAM-SHA-256$<replaceable><iteration
count></replaceable>:<replaceable>&l
(references <link
linkend="catalog-pg-tablespace"><structname>pg_tablespace</structname></link>.<structfield>oid</structfield>)
</para>
<para>
- The tablespace in which this relation is stored. If zero,
- the database's default tablespace is implied. (Not meaningful
- if the relation has no on-disk file.)
+ The tablespace in which this relation is stored; for partitioned
+ tables, the tablespace in which partitions are created
+ when one is not specified in the creation command.
+ If zero, the database's default tablespace is implied.
+ (Not meaningful if the relation has no on-disk file.)
</para></entry>
</row>
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 54c58d1764..9e36aae425 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -950,27 +950,20 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid
ownerId,
}
/*
- * If the statement hasn't specified an access method, but we're
defining
- * a type of relation that needs one, use the default.
+ * Select access method to use: an explicitly indicated one, or (in the
+ * case of a partitioned table) the parent's, if it has one.
*/
if (stmt->accessMethod != NULL)
accessMethodId = get_table_am_oid(stmt->accessMethod, false);
else if (stmt->partbound)
{
- /*
- * For partitions, if no access method is specified, use the AM
of the
- * parent table.
- */
Assert(list_length(inheritOids) == 1);
accessMethodId = get_rel_relam(linitial_oid(inheritOids));
}
else
accessMethodId = InvalidOid;
- /*
- * Still nothing? Use the default. Partitioned tables default to
- * InvalidOid without an access method specified.
- */
+ /* still nothing? use the default */
if (RELKIND_HAS_TABLE_AM(relkind) && !OidIsValid(accessMethodId))
accessMethodId = get_table_am_oid(default_table_access_method,
false);
@@ -5403,7 +5396,6 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
/* nothing to do here, oid columns don't exist anymore
*/
break;
case AT_SetAccessMethod: /* SET ACCESS METHOD */
- /* handled specially in Phase 3 */
/*
* Only do this for partitioned tables, for which this
is just a
@@ -15207,15 +15199,8 @@ ATExecDropCluster(Relation rel, LOCKMODE lockmode)
/*
* Preparation phase for SET ACCESS METHOD
*
- * Check that access method exists. If it is the same as the table's current
- * access method, it is a no-op. Otherwise, a table rewrite is necessary for
- * relations with storage.
- * If amname is NULL, select default_table_access_method as access method.
- *
- * Partitioned tables may use InvalidOid to assign the default AM to partitions
- * when created. This operation is a no-op if a DEFAULT was given and the
- * partitioned table uses InvalidOid as relam, or if the relam and the access
- * method specified match.
+ * Check that the access method exists and determine whether a change is
+ * actually needed.
*/
static void
ATPrepSetAccessMethod(AlteredTableInfo *tab, Relation rel, const char *amname)
@@ -15223,10 +15208,9 @@ ATPrepSetAccessMethod(AlteredTableInfo *tab, Relation
rel, const char *amname)
Oid amoid;
/*
- * Check that the table access method exists.
- *
- * Use the access method specified, otherwise, when not specified, use
- * InvalidOid for partitioned tables or the configured default AM.
+ * Look up the access method name and check that it differs from the
+ * table's current AM. If DEFAULT was specified for a partitioned table
+ * (amname is NULL), set it to InvalidOid to reset the catalogued AM.
*/
if (amname != NULL)
amoid = get_table_am_oid(amname, false);
@@ -15235,6 +15219,7 @@ ATPrepSetAccessMethod(AlteredTableInfo *tab, Relation
rel, const char *amname)
else
amoid = get_table_am_oid(default_table_access_method, false);
+ /* if it's a match, phase 3 doesn't need to do anything */
if (rel->rd_rel->relam == amoid)
return;
@@ -15244,6 +15229,100 @@ ATPrepSetAccessMethod(AlteredTableInfo *tab, Relation
rel, const char *amname)
tab->chgAccessMethod = true;
}
+/*
+ * Special handling of ALTER TABLE SET ACCESS METHOD for relations with no
+ * storage that have an interest in preserving AM.
+ *
+ * Since these have no storage, setting the access method is a catalog only
+ * operation.
+ */
+static void
+ATExecSetAccessMethodNoStorage(Relation rel, Oid newAccessMethodId)
+{
+ Relation pg_class;
+ Oid oldAccessMethodId;
+ HeapTuple tuple;
+ Form_pg_class rd_rel;
+ Oid reloid = RelationGetRelid(rel);
+
+ /*
+ * Shouldn't be called on relations having storage; these are processed
in
+ * phase 3.
+ */
+ Assert(!RELKIND_HAS_STORAGE(rel->rd_rel->relkind));
+
+ /* Get a modifiable copy of the relation's pg_class row. */
+ pg_class = table_open(RelationRelationId, RowExclusiveLock);
+
+ tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(reloid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for relation %u", reloid);
+ rd_rel = (Form_pg_class) GETSTRUCT(tuple);
+
+ /* Update the pg_class row. */
+ oldAccessMethodId = rd_rel->relam;
+ rd_rel->relam = newAccessMethodId;
+
+ /* Leave if no update required */
+ if (rd_rel->relam == oldAccessMethodId)
+ {
+ heap_freetuple(tuple);
+ table_close(pg_class, RowExclusiveLock);
+ return;
+ }
+
+ CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
+
+ /*
+ * Update the dependency on the new access method. No dependency is
added
+ * if the new access method is InvalidOid (default case). Be very
careful
+ * that this has to compare the previous value stored in pg_class with
the
+ * new one.
+ */
+ if (!OidIsValid(oldAccessMethodId) && OidIsValid(rd_rel->relam))
+ {
+ ObjectAddress relobj,
+ referenced;
+
+ /*
+ * New access method is defined and there was no dependency
+ * previously, so record a new one.
+ */
+ ObjectAddressSet(relobj, RelationRelationId, reloid);
+ ObjectAddressSet(referenced, AccessMethodRelationId,
rd_rel->relam);
+ recordDependencyOn(&relobj, &referenced, DEPENDENCY_NORMAL);
+ }
+ else if (OidIsValid(oldAccessMethodId) &&
+ !OidIsValid(rd_rel->relam))
+ {
+ /*
+ * There was an access method defined, and no new one, so just
remove
+ * the existing dependency.
+ */
+ deleteDependencyRecordsForClass(RelationRelationId, reloid,
+
AccessMethodRelationId,
+
DEPENDENCY_NORMAL);
+ }
+ else
+ {
+ Assert(OidIsValid(oldAccessMethodId) &&
+ OidIsValid(rd_rel->relam));
+
+ /* Both are valid, so update the dependency */
+ changeDependencyFor(RelationRelationId, reloid,
+ AccessMethodRelationId,
+ oldAccessMethodId,
rd_rel->relam);
+ }
+
+ /* make the relam and dependency changes visible */
+ CommandCounterIncrement();
+
+ InvokeObjectPostAlterHook(RelationRelationId, RelationGetRelid(rel), 0);
+
+ heap_freetuple(tuple);
+ table_close(pg_class, RowExclusiveLock);
+}
+
/*
* ALTER TABLE SET TABLESPACE
*/
@@ -15566,102 +15645,6 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace,
LOCKMODE lockmode)
list_free(reltoastidxids);
}
-/*
- * Special handling of ALTER TABLE SET ACCESS METHOD for relations with no
- * storage that have an interest in preserving AM.
- *
- * Since these have no storage, setting the access method is a catalog only
- * operation.
- */
-static void
-ATExecSetAccessMethodNoStorage(Relation rel, Oid newAccessMethodId)
-{
- Relation pg_class;
- Oid oldAccessMethodId;
- HeapTuple tuple;
- Form_pg_class rd_rel;
- Oid reloid = RelationGetRelid(rel);
-
- /*
- * Shouldn't be called on relations having storage; these are processed
in
- * phase 3.
- */
- Assert(!RELKIND_HAS_STORAGE(rel->rd_rel->relkind));
-
- /* Get a modifiable copy of the relation's pg_class row. */
- pg_class = table_open(RelationRelationId, RowExclusiveLock);
-
- tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(reloid));
- if (!HeapTupleIsValid(tuple))
- elog(ERROR, "cache lookup failed for relation %u", reloid);
- rd_rel = (Form_pg_class) GETSTRUCT(tuple);
-
- /* Update the pg_class row. */
- oldAccessMethodId = rd_rel->relam;
- rd_rel->relam = newAccessMethodId;
-
- /* Leave if no update required */
- if (rd_rel->relam == oldAccessMethodId)
- {
- heap_freetuple(tuple);
- table_close(pg_class, RowExclusiveLock);
- return;
- }
-
- CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
-
- /*
- * Update the dependency on the new access method. No dependency is
added
- * if the new access method is InvalidOid (default case). Be very
careful
- * that this has to compare the previous value stored in pg_class with
the
- * new one.
- */
- if (!OidIsValid(oldAccessMethodId) && OidIsValid(rd_rel->relam))
- {
- ObjectAddress relobj,
- referenced;
-
- /*
- * New access method is defined and there was no dependency
- * previously, so record a new one.
- */
- ObjectAddressSet(relobj, RelationRelationId, reloid);
- ObjectAddressSet(referenced, AccessMethodRelationId,
rd_rel->relam);
- recordDependencyOn(&relobj, &referenced, DEPENDENCY_NORMAL);
- }
- else if (OidIsValid(oldAccessMethodId) &&
- !OidIsValid(rd_rel->relam))
- {
- /*
- * There was an access method defined, and no new one, so just
remove
- * the existing dependency.
- */
- deleteDependencyRecordsForClass(RelationRelationId, reloid,
-
AccessMethodRelationId,
-
DEPENDENCY_NORMAL);
- }
- else
- {
- Assert(OidIsValid(oldAccessMethodId) &&
- OidIsValid(rd_rel->relam));
-
- /* Both are valid, so update the dependency */
- changeDependencyFor(RelationRelationId, reloid,
- AccessMethodRelationId,
- oldAccessMethodId,
rd_rel->relam);
- }
-
- CommandCounterIncrement();
-
- InvokeObjectPostAlterHook(RelationRelationId, RelationGetRelid(rel), 0);
-
- heap_freetuple(tuple);
- table_close(pg_class, RowExclusiveLock);
-
- /* Make sure the relam change is visible */
- CommandCounterIncrement();
-}
-
/*
* Special handling of ALTER TABLE SET TABLESPACE for relations with no
* storage that have an interest in preserving tablespace.
--
2.39.2
