public inbox for [email protected]  
help / color / mirror / Atom feed
[PATCH v5 2/2] New command ALTER TABLE ALL IN TABLESPACE SET LOGGED/UNLOGGED
4+ messages / 3 participants
[nested] [flat]

* [PATCH v5 2/2] New command ALTER TABLE ALL IN TABLESPACE SET LOGGED/UNLOGGED
@ 2020-11-11 14:21  Kyotaro Horiguchi <[email protected]>
  0 siblings, 0 replies; 4+ messages in thread

From: Kyotaro Horiguchi @ 2020-11-11 14:21 UTC (permalink / raw)

To ease invoking ALTER TABLE SET LOGGED/UNLOGGED, this command changes
relation persistence of all tables in the specified tablespace.
---
 src/backend/commands/tablecmds.c | 140 +++++++++++++++++++++++++++++++
 src/backend/nodes/copyfuncs.c    |  16 ++++
 src/backend/nodes/equalfuncs.c   |  15 ++++
 src/backend/parser/gram.y        |  20 +++++
 src/backend/tcop/utility.c       |  11 +++
 src/include/commands/tablecmds.h |   2 +
 src/include/nodes/nodes.h        |   1 +
 src/include/nodes/parsenodes.h   |   9 ++
 8 files changed, 214 insertions(+)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 37a15d31ee..2f65abb19b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13696,6 +13696,146 @@ AlterTableMoveAll(AlterTableMoveAllStmt *stmt)
 	return new_tablespaceoid;
 }
 
+/*
+ * Alter Table ALL ... SET LOGGED/UNLOGGED
+ *
+ * Allows a user to change persistence of all objects in a given tablespace in
+ * the current database.  Objects can be chosen based on the owner of the
+ * object also, to allow users to change persistene only their objects. The
+ * main permissions handling is done by the lower-level change persistence
+ * function.
+ *
+ * All to-be-modified objects are locked first. If NOWAIT is specified and the
+ * lock can't be acquired then we ereport(ERROR).
+ */
+void
+AlterTableSetLoggedAll(AlterTableSetLoggedAllStmt *stmt)
+{
+	List	   *relations = NIL;
+	ListCell   *l;
+	ScanKeyData key[1];
+	Relation	rel;
+	TableScanDesc scan;
+	HeapTuple	tuple;
+	Oid			tablespaceoid;
+	List	   *role_oids = roleSpecsToIds(NIL);
+
+	/* Ensure we were not asked to change something we can't */
+	if (stmt->objtype != OBJECT_TABLE)
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("only tables can be specified")));
+
+	/* Get the tablespace OID */
+	tablespaceoid = get_tablespace_oid(stmt->tablespacename, false);
+
+	/*
+	 * Now that the checks are done, check if we should set either to
+	 * InvalidOid because it is our database's default tablespace.
+	 */
+	if (tablespaceoid == MyDatabaseTableSpace)
+		tablespaceoid = InvalidOid;
+
+	/*
+	 * Walk the list of objects in the tablespace to pick up them. This will
+	 * only find objects in our database, of course.
+	 */
+	ScanKeyInit(&key[0],
+				Anum_pg_class_reltablespace,
+				BTEqualStrategyNumber, F_OIDEQ,
+				ObjectIdGetDatum(tablespaceoid));
+
+	rel = table_open(RelationRelationId, AccessShareLock);
+	scan = table_beginscan_catalog(rel, 1, key);
+	while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
+	{
+		Form_pg_class relForm = (Form_pg_class) GETSTRUCT(tuple);
+		Oid			relOid = relForm->oid;
+
+		/*
+		 * Do not pick-up objects in pg_catalog as part of this, if an admin
+		 * really wishes to do so, they can issue the individual ALTER
+		 * commands directly.
+		 *
+		 * Also, explicitly avoid any shared tables, temp tables, or TOAST
+		 * (TOAST will be changed with the main table).
+		 */
+		if (IsCatalogNamespace(relForm->relnamespace) ||
+			relForm->relisshared ||
+			isAnyTempNamespace(relForm->relnamespace) ||
+			IsToastNamespace(relForm->relnamespace))
+			continue;
+
+		/* Only pick up the object type requested */
+		if (relForm->relkind != RELKIND_RELATION)
+			continue;
+
+		/* Check if we are only picking-up objects owned by certain roles */
+		if (role_oids != NIL && !list_member_oid(role_oids, relForm->relowner))
+			continue;
+
+		/*
+		 * Handle permissions-checking here since we are locking the tables
+		 * and also to avoid doing a bunch of work only to fail part-way. Note
+		 * that permissions will also be checked by AlterTableInternal().
+		 *
+		 * Caller must be considered an owner on the table of which we're going
+		 * to change persistence.
+		 */
+		if (!pg_class_ownercheck(relOid, GetUserId()))
+			aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(get_rel_relkind(relOid)),
+						   NameStr(relForm->relname));
+
+		if (stmt->nowait &&
+			!ConditionalLockRelationOid(relOid, AccessExclusiveLock))
+			ereport(ERROR,
+					(errcode(ERRCODE_OBJECT_IN_USE),
+					 errmsg("aborting because lock on relation \"%s.%s\" is not available",
+							get_namespace_name(relForm->relnamespace),
+							NameStr(relForm->relname))));
+		else
+			LockRelationOid(relOid, AccessExclusiveLock);
+
+		/*
+		 * Add to our list of objects of which we're going to change
+		 * persistence.
+		 */
+		relations = lappend_oid(relations, relOid);
+	}
+
+	table_endscan(scan);
+	table_close(rel, AccessShareLock);
+
+	if (relations == NIL)
+		ereport(NOTICE,
+				(errcode(ERRCODE_NO_DATA_FOUND),
+				 errmsg("no matching relations in tablespace \"%s\" found",
+						tablespaceoid == InvalidOid ? "(database default)" :
+						get_tablespace_name(tablespaceoid))));
+
+	/*
+	 * Everything is locked, loop through and change persistence of all of the
+	 * relations.
+	 */
+	foreach(l, relations)
+	{
+		List	   *cmds = NIL;
+		AlterTableCmd *cmd = makeNode(AlterTableCmd);
+
+		if (stmt->logged)
+			cmd->subtype = AT_SetLogged;
+		else
+			cmd->subtype = AT_SetUnLogged;
+
+		cmds = lappend(cmds, cmd);
+
+		EventTriggerAlterTableStart((Node *) stmt);
+		/* OID is set by AlterTableInternal */
+		AlterTableInternal(lfirst_oid(l), cmds, false);
+		EventTriggerAlterTableEnd();
+	}
+}
+
 static void
 index_copy_data(Relation rel, RelFileNode newrnode)
 {
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index ba3ccc712c..127da5151d 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -4138,6 +4138,19 @@ _copyAlterTableMoveAllStmt(const AlterTableMoveAllStmt *from)
 	return newnode;
 }
 
+static AlterTableSetLoggedAllStmt *
+_copyAlterTableSetLoggedAllStmt(const AlterTableSetLoggedAllStmt *from)
+{
+	AlterTableSetLoggedAllStmt *newnode = makeNode(AlterTableSetLoggedAllStmt);
+
+	COPY_STRING_FIELD(tablespacename);
+	COPY_SCALAR_FIELD(objtype);
+	COPY_SCALAR_FIELD(logged);
+	COPY_SCALAR_FIELD(nowait);
+
+	return newnode;
+}
+
 static CreateExtensionStmt *
 _copyCreateExtensionStmt(const CreateExtensionStmt *from)
 {
@@ -5441,6 +5454,9 @@ copyObjectImpl(const void *from)
 		case T_AlterTableMoveAllStmt:
 			retval = _copyAlterTableMoveAllStmt(from);
 			break;
+		case T_AlterTableSetLoggedAllStmt:
+			retval = _copyAlterTableSetLoggedAllStmt(from);
+			break;
 		case T_CreateExtensionStmt:
 			retval = _copyCreateExtensionStmt(from);
 			break;
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index a2ef853dc2..4f13a1762b 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -1872,6 +1872,18 @@ _equalAlterTableMoveAllStmt(const AlterTableMoveAllStmt *a,
 	return true;
 }
 
+static bool
+_equalAlterTableSetLoggedAllStmt(const AlterTableSetLoggedAllStmt *a,
+								 const AlterTableSetLoggedAllStmt *b)
+{
+	COMPARE_STRING_FIELD(tablespacename);
+	COMPARE_SCALAR_FIELD(objtype);
+	COMPARE_SCALAR_FIELD(logged);
+	COMPARE_SCALAR_FIELD(nowait);
+
+	return true;
+}
+
 static bool
 _equalCreateExtensionStmt(const CreateExtensionStmt *a, const CreateExtensionStmt *b)
 {
@@ -3494,6 +3506,9 @@ equal(const void *a, const void *b)
 		case T_AlterTableMoveAllStmt:
 			retval = _equalAlterTableMoveAllStmt(a, b);
 			break;
+		case T_AlterTableSetLoggedAllStmt:
+			retval = _equalAlterTableSetLoggedAllStmt(a, b);
+			break;
 		case T_CreateExtensionStmt:
 			retval = _equalCreateExtensionStmt(a, b);
 			break;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 31c95443a5..2222fd8fe3 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -1934,6 +1934,26 @@ AlterTableStmt:
 					n->nowait = $13;
 					$$ = (Node *)n;
 				}
+		|	ALTER TABLE ALL IN_P TABLESPACE name SET LOGGED opt_nowait
+				{
+					AlterTableSetLoggedAllStmt *n =
+						makeNode(AlterTableSetLoggedAllStmt);
+					n->tablespacename = $6;
+					n->objtype = OBJECT_TABLE;
+					n->logged = true;
+					n->nowait = $9;
+					$$ = (Node *)n;
+				}
+		|	ALTER TABLE ALL IN_P TABLESPACE name SET UNLOGGED opt_nowait
+				{
+					AlterTableSetLoggedAllStmt *n =
+						makeNode(AlterTableSetLoggedAllStmt);
+					n->tablespacename = $6;
+					n->objtype = OBJECT_TABLE;
+					n->logged = false;
+					n->nowait = $9;
+					$$ = (Node *)n;
+				}
 		|	ALTER INDEX qualified_name alter_table_cmds
 				{
 					AlterTableStmt *n = makeNode(AlterTableStmt);
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 53a511f1da..16606448bf 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -161,6 +161,7 @@ ClassifyUtilityCommandAsReadOnly(Node *parsetree)
 		case T_AlterTSConfigurationStmt:
 		case T_AlterTSDictionaryStmt:
 		case T_AlterTableMoveAllStmt:
+		case T_AlterTableSetLoggedAllStmt:
 		case T_AlterTableSpaceOptionsStmt:
 		case T_AlterTableStmt:
 		case T_AlterTypeStmt:
@@ -1732,6 +1733,12 @@ ProcessUtilitySlow(ParseState *pstate,
 				commandCollected = true;
 				break;
 
+			case T_AlterTableSetLoggedAllStmt:
+				AlterTableSetLoggedAll((AlterTableSetLoggedAllStmt *) parsetree);
+				/* commands are stashed in AlterTableSetLoggedAll */
+				commandCollected = true;
+				break;
+
 			case T_DropStmt:
 				ExecDropStmt((DropStmt *) parsetree, isTopLevel);
 				/* no commands stashed for DROP */
@@ -2619,6 +2626,10 @@ CreateCommandTag(Node *parsetree)
 			tag = AlterObjectTypeCommandTag(((AlterTableMoveAllStmt *) parsetree)->objtype);
 			break;
 
+		case T_AlterTableSetLoggedAllStmt:
+			tag = AlterObjectTypeCommandTag(((AlterTableSetLoggedAllStmt *) parsetree)->objtype);
+			break;
+
 		case T_AlterTableStmt:
 			tag = AlterObjectTypeCommandTag(((AlterTableStmt *) parsetree)->objtype);
 			break;
diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index 08c463d3c4..646928466d 100644
--- a/src/include/commands/tablecmds.h
+++ b/src/include/commands/tablecmds.h
@@ -42,6 +42,8 @@ extern void AlterTableInternal(Oid relid, List *cmds, bool recurse);
 
 extern Oid	AlterTableMoveAll(AlterTableMoveAllStmt *stmt);
 
+extern void AlterTableSetLoggedAll(AlterTableSetLoggedAllStmt *stmt);
+
 extern ObjectAddress AlterTableNamespace(AlterObjectSchemaStmt *stmt,
 										 Oid *oldschema);
 
diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index caed683ba9..16d91d3e1d 100644
--- a/src/include/nodes/nodes.h
+++ b/src/include/nodes/nodes.h
@@ -424,6 +424,7 @@ typedef enum NodeTag
 	T_AlterCollationStmt,
 	T_CallStmt,
 	T_AlterStatsStmt,
+	T_AlterTableSetLoggedAllStmt,
 
 	/*
 	 * TAGS FOR PARSE TREE NODES (parsenodes.h)
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index dc2bb40926..c3eab6f1ab 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2253,6 +2253,15 @@ typedef struct AlterTableMoveAllStmt
 	bool		nowait;
 } AlterTableMoveAllStmt;
 
+typedef struct AlterTableSetLoggedAllStmt
+{
+	NodeTag		type;
+	char	   *tablespacename;
+	ObjectType	objtype;		/* Object type to move */
+	bool		logged;
+	bool		nowait;
+} AlterTableSetLoggedAllStmt;
+
 /* ----------------------
  *		Create/Alter Extension Statements
  * ----------------------
-- 
2.27.0


----Next_Part(Thu_Jan_14_17_32_17_2021_289)----





^ permalink  raw  reply  [nested|flat] 4+ messages in thread

* Re: tablecmds: clarify recurse vs recusing
@ 2026-01-20 20:07  Tom Lane <[email protected]>
  0 siblings, 1 reply; 4+ messages in thread

From: Tom Lane @ 2026-01-20 20:07 UTC (permalink / raw)
  To: Chao Li <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Postgres hackers <[email protected]>

Chao Li <[email protected]> writes:
> Enhancing the header comments also helps here.
> PSA v2:

I had something more like the attached in mind.  I'm not generally
a fan of documenting only some of the arguments of a function, so
I don't care for the way you handled the issue for other functions
in tablecmds.c either.  We can either assume that people read
ATPrepCmd's comment and can extrapolate to the other functions,
or we can do something similar to this for all of them.

I do agree with your 0002, but I see no point in pushing that
separately.

			regards, tom lane



Attachments:

  [text/x-diff] v3-document-ATPrepCmd-arguments.patch (1.4K, ../../[email protected]/2-v3-document-ATPrepCmd-arguments.patch)
  download | inline diff:
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f976c0e5c7e..fcdb81dd4ea 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4901,8 +4901,22 @@ ATController(AlterTableStmt *parsetree,
  * Traffic cop for ALTER TABLE Phase 1 operations, including simple
  * recursion and permission checks.
  *
- * Caller must have acquired appropriate lock type on relation already.
+ * *wqueue: resulting AlteredTableInfo structs are added to this list
+ * rel: the relation we are currently considering
+ * cmd: the ALTER TABLE subcommand we are currently considering
+ * recurse: true to recurse to child tables of rel (including partitions)
+ * recursing: true if already recursing (rel is a descendant of original table)
+ * lockmode: lock level held on rel, and to be acquired on children
+ * context: context passed down from ProcessUtility, or NULL if none
+ *
+ * At top level, recurse is true unless the command specified ONLY.
+ * Internally, it can be true if we are descending the inheritance tree
+ * on-the-fly, or false if we already expanded the tree at an outer level
+ * (see ATSimpleRecursion).
+ *
+ * Caller must have acquired lockmode on rel already.
  * This lock should be held until commit.
+ * If we recurse, the same lockmode will be acquired on child tables.
  */
 static void
 ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,


^ permalink  raw  reply  [nested|flat] 4+ messages in thread

* Re: tablecmds: clarify recurse vs recusing
@ 2026-01-20 22:55  Chao Li <[email protected]>
  parent: Tom Lane <[email protected]>
  0 siblings, 1 reply; 4+ messages in thread

From: Chao Li @ 2026-01-20 22:55 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Postgres hackers <[email protected]>

On Jan 21, 2026, at 04:07, Tom Lane <[email protected]> wrote:

Chao Li <[email protected]> writes:

Enhancing the header comments also helps here.
PSA v2:


I had something more like the attached in mind.  I'm not generally
a fan of documenting only some of the arguments of a function, so
I don't care for the way you handled the issue for other functions
in tablecmds.c either.  We can either assume that people read
ATPrepCmd's comment and can extrapolate to the other functions,
or we can do something similar to this for all of them.


Got it, thanks. In v4, I’ve limited the changes to fully documenting all
ATPrepCmd() arguments in its header comment and removed the scattered
recurse/recursing references from other functions.


I do agree with your 0002, but I see no point in pushing that
separately.


Absolutely. 0002 was too trivial to be a separate commit. I have squashed
it into 0001 in v4.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/


Attachments:

  [application/octet-stream] v4-0001-tablecmds-clarify-recurse-recursing-semantics-in-.patch (3.9K, ../../CAEoWx2m=dditqdNvwC45HNMmzU0jVvN1_2dsP4ma2Gj4Wu7HhQ@mail.gmail.com/3-v4-0001-tablecmds-clarify-recurse-recursing-semantics-in-.patch)
  download | inline diff:
From 5c688981347b72ac07ff799e96f2664443d19d4b Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <[email protected]>
Date: Tue, 20 Jan 2026 09:57:24 +0800
Subject: [PATCH v4] tablecmds: clarify recurse/recursing semantics in
 ATPrepCmd()

Document all ATPrepCmd() arguments and centralize the explanation of how
recurse and recursing interact during ALTER TABLE recursion. Remove
scattered partial comments and keep other function comments lightweight.

No functional changes.

Author: Chao Li <[email protected]>
Reviewed-by: Tom Lane <[email protected]>
Discussion: https://postgr.es/m/CAEoWx2n9E6_zxPbqwMpaPuC1C_p4b3y635SjiDuCPSVm8GBjsA@mail.gmail.com
---
 src/backend/commands/tablecmds.c | 35 +++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f976c0e5c7e..6c3b8c57ef9 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4518,11 +4518,10 @@ AlterTableLookupRelation(AlterTableStmt *stmt, LOCKMODE lockmode)
  *
  * The caller must lock the relation, with an appropriate lock level
  * 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,
- * rather than reassess it at lower levels.
+ * 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, rather than reassess it at lower levels.
  *
  * The caller also provides a "context" which is to be passed back to
  * utility.c when we need to execute a subcommand such as CREATE INDEX.
@@ -4901,8 +4900,29 @@ ATController(AlterTableStmt *parsetree,
  * Traffic cop for ALTER TABLE Phase 1 operations, including simple
  * recursion and permission checks.
  *
- * Caller must have acquired appropriate lock type on relation already.
- * This lock should be held until commit.
+ * *wqueue: resulting AlteredTableInfo structs are added to this list
+ * rel: the relation we are currently considering
+ * cmd: the ALTER TABLE subcommand we are currently considering
+ * recurse: true to recurse to child tables of rel (including partitions)
+ * recursing: true if already recursing (rel is a descendant of original table)
+ * lockmode: lock level held on rel, and to be acquired on children
+ * context: context passed down from ProcessUtility, or NULL if none
+ *
+ * At top level, recurse is true unless the command specified ONLY.
+ * Internally, recurse may be true when descending the inheritance tree
+ * on-the-fly, or false when the tree has already been expanded at an
+ * outer level (see ATSimpleRecursion).
+ *
+ * recurse and recursing together control how recursion proceeds:
+ *
+ *   recurse=false, recursing=false: top-level call, no recursion
+ *   recurse=true,  recursing=false: top-level call, recurse if supported
+ *   recurse=true,  recursing=true:  recursive call, continue recursion
+ *   recurse=false, recursing=true:  recursive call, stop recursion
+ *
+ * Caller must have acquired lockmode on rel already. This lock should be held
+ * until commit. If we recurse, the same lockmode will be acquired on child
+ * tables.
  */
 static void
 ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
@@ -10746,6 +10766,7 @@ validateFkOnDeleteSetColumns(int numfks, const int16 *fkattnums,
  *      (...) clause
  * fkdelsetcols: the attnum array of the columns in the ON DELETE SET
  *      NULL/DEFAULT clause
+ * is_internal: true if this is an internally generated constraint
  * with_period: true if this is a temporal FK
  */
 static ObjectAddress
-- 
2.39.5 (Apple Git-154)



^ permalink  raw  reply  [nested|flat] 4+ messages in thread

* Re: tablecmds: clarify recurse vs recusing
@ 2026-05-28 09:26  Chao Li <[email protected]>
  parent: Chao Li <[email protected]>
  0 siblings, 0 replies; 4+ messages in thread

From: Chao Li @ 2026-05-28 09:26 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Postgres hackers <[email protected]>



> On Jan 21, 2026, at 06:55, Chao Li <[email protected]> wrote:
> 
> 
> 
>> On Jan 21, 2026, at 04:07, Tom Lane <[email protected]> wrote:
>> 
>> Chao Li <[email protected]> writes:
>>> Enhancing the header comments also helps here.
>>> PSA v2:
>> 
>> I had something more like the attached in mind.  I'm not generally
>> a fan of documenting only some of the arguments of a function, so
>> I don't care for the way you handled the issue for other functions
>> in tablecmds.c either.  We can either assume that people read
>> ATPrepCmd's comment and can extrapolate to the other functions,
>> or we can do something similar to this for all of them.
> 
> Got it, thanks. In v4, I’ve limited the changes to fully documenting all ATPrepCmd() arguments in its header comment and removed the scattered recurse/recursing references from other functions.
> 
>> 
>> I do agree with your 0002, but I see no point in pushing that
>> separately.
> 
> Absolutely. 0002 was too trivial to be a separate commit. I have squashed it into 0001 in v4.
> 
> Best regards,
> --
> Chao Li (Evan)
> HighGo Software Co., Ltd.
> https://www.highgo.com/
> 
> 
> 
> 
> <v4-0001-tablecmds-clarify-recurse-recursing-semantics-in-.patch>

Trying to bump this patch because, while working on patch [1], I needed to think about the meaning of “recurse” and “recursing”. That reminded me of this patch, and it helped me recall the distinction. So I still think this pure comment update patch is still helpful.

Attached v5 is just a rebased version, with no changes from v4.

[1] https://www.postgresql.org/message-id/E74C57FA-1DD0-4C8E-8FB1-538034752592%40gmail.com

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/






Attachments:

  [application/octet-stream] v5-0001-tablecmds-clarify-recurse-recursing-semantics-in-.patch (3.9K, ../../[email protected]/2-v5-0001-tablecmds-clarify-recurse-recursing-semantics-in-.patch)
  download | inline diff:
From 9108d4544a425ae8a1818e3a7dc722d00c65bb59 Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <[email protected]>
Date: Tue, 20 Jan 2026 09:57:24 +0800
Subject: [PATCH v5] tablecmds: clarify recurse/recursing semantics in
 ATPrepCmd()

Document all ATPrepCmd() arguments and centralize the explanation of how
recurse and recursing interact during ALTER TABLE recursion. Remove
scattered partial comments and keep other function comments lightweight.

No functional changes.

Author: Chao Li <[email protected]>
Reviewed-by: Tom Lane <[email protected]>
Discussion: https://postgr.es/m/CAEoWx2n9E6_zxPbqwMpaPuC1C_p4b3y635SjiDuCPSVm8GBjsA@mail.gmail.com
---
 src/backend/commands/tablecmds.c | 35 +++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index a1845240a98..63cd1df71bc 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4577,11 +4577,10 @@ AlterTableLookupRelation(AlterTableStmt *stmt, LOCKMODE lockmode)
  *
  * The caller must lock the relation, with an appropriate lock level
  * 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,
- * rather than reassess it at lower levels.
+ * 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, rather than reassess it at lower levels.
  *
  * The caller also provides a "context" which is to be passed back to
  * utility.c when we need to execute a subcommand such as CREATE INDEX.
@@ -4960,8 +4959,29 @@ ATController(AlterTableStmt *parsetree,
  * Traffic cop for ALTER TABLE Phase 1 operations, including simple
  * recursion and permission checks.
  *
- * Caller must have acquired appropriate lock type on relation already.
- * This lock should be held until commit.
+ * *wqueue: resulting AlteredTableInfo structs are added to this list
+ * rel: the relation we are currently considering
+ * cmd: the ALTER TABLE subcommand we are currently considering
+ * recurse: true to recurse to child tables of rel (including partitions)
+ * recursing: true if already recursing (rel is a descendant of original table)
+ * lockmode: lock level held on rel, and to be acquired on children
+ * context: context passed down from ProcessUtility, or NULL if none
+ *
+ * At top level, recurse is true unless the command specified ONLY.
+ * Internally, recurse may be true when descending the inheritance tree
+ * on-the-fly, or false when the tree has already been expanded at an
+ * outer level (see ATSimpleRecursion).
+ *
+ * recurse and recursing together control how recursion proceeds:
+ *
+ *   recurse=false, recursing=false: top-level call, no recursion
+ *   recurse=true,  recursing=false: top-level call, recurse if supported
+ *   recurse=true,  recursing=true:  recursive call, continue recursion
+ *   recurse=false, recursing=true:  recursive call, stop recursion
+ *
+ * Caller must have acquired lockmode on rel already. This lock should be held
+ * until commit. If we recurse, the same lockmode will be acquired on child
+ * tables.
  */
 static void
 ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
@@ -10819,6 +10839,7 @@ validateFkOnDeleteSetColumns(int numfks, const int16 *fkattnums,
  *      (...) clause
  * fkdelsetcols: the attnum array of the columns in the ON DELETE SET
  *      NULL/DEFAULT clause
+ * is_internal: true if this is an internally generated constraint
  * with_period: true if this is a temporal FK
  */
 static ObjectAddress
-- 
2.50.1 (Apple Git-155)



^ permalink  raw  reply  [nested|flat] 4+ messages in thread


end of thread, other threads:[~2026-05-28 09:26 UTC | newest]

Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-11-11 14:21 [PATCH v5 2/2] New command ALTER TABLE ALL IN TABLESPACE SET LOGGED/UNLOGGED Kyotaro Horiguchi <[email protected]>
2026-01-20 20:07 Re: tablecmds: clarify recurse vs recusing Tom Lane <[email protected]>
2026-01-20 22:55 ` Re: tablecmds: clarify recurse vs recusing Chao Li <[email protected]>
2026-05-28 09:26   ` Re: tablecmds: clarify recurse vs recusing Chao Li <[email protected]>

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox