public inbox for [email protected]
help / color / mirror / Atom feedFrom: Chao Li <[email protected]>
To: Tom Lane <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: Postgres hackers <[email protected]>
Subject: Re: tablecmds: clarify recurse vs recusing
Date: Thu, 28 May 2026 17:26:55 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAEoWx2m=dditqdNvwC45HNMmzU0jVvN1_2dsP4ma2Gj4Wu7HhQ@mail.gmail.com>
References: <CAEoWx2n9E6_zxPbqwMpaPuC1C_p4b3y635SjiDuCPSVm8GBjsA@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAEoWx2=QnOBUCuTg4rt7n0FjyvhBfztWMffSdObGCbGrz2tg6w@mail.gmail.com>
<[email protected]>
<CAEoWx2m=dditqdNvwC45HNMmzU0jVvN1_2dsP4ma2Gj4Wu7HhQ@mail.gmail.com>
> 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)
view thread (4+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: tablecmds: clarify recurse vs recusing
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox