tablecmds: clarify recurse vs recusing

On 19.01.26 08:14, Chao Li wrote:

Many ALTER TABLE-related functions take two boolean parameters,
"recurse" and "recursing", whose names are easy to confuse.

I'm not bothered by this.

To reduce this confusion, I’m proposing to rename "recurse" to
"no_only", which more directly reflects its meaning.

This seems worse. Especially since no_only is almost a double negative.

On Mon, Jan 19, 2026 at 9:20 PM Peter Eisentraut <peter@eisentraut.org> wrote:

On 19.01.26 08:14, Chao Li wrote:

Many ALTER TABLE-related functions take two boolean parameters,
"recurse" and "recursing", whose names are easy to confuse.

I'm not bothered by this.

+1. I had stumbled upon this when working on identity support. I had
the same initial reaction that it's confusing. But soon I got used to
it and any other option wasn't improving the situation. AFAIR, all the
4 possible combinations of those two booleans are possible, but I
can't remember what could recurse = false, recursing = true mean.
recurse = true, recursing = false happens at the start of the
inheritance tree. recurse = false, recursing = false; maybe for a
table which isn't part of inheritance or partition tree. recurse =
true, recursing = true happens when working on a non-leaf non-root
table. If we could find out the impossible combinations and then unify
these two booleans into a single enum variable, that might make code
clearer - at least we will know which combinations are not possible
and which combinations are.

--
Best Wishes,
Ashutosh Bapat

Peter Eisentraut <peter@eisentraut.org> writes:

On 19.01.26 08:14, Chao Li wrote:

Many ALTER TABLE-related functions take two boolean parameters,
"recurse" and "recursing", whose names are easy to confuse.

I'm not bothered by this.

To reduce this confusion, I’m proposing to rename "recurse" to
"no_only", which more directly reflects its meaning.

This seems worse. Especially since no_only is almost a double negative.

Yeah, I don't find this change an improvement either. I think the
actual problem here is lack of documentation: unlike many other
places, there is next to zero commentary in tablecmds.c about what
all the function parameters are. It would probably help to define
these, along the lines of

* recurse: true if we should recurse to children of this table
* recursing: true if we are already recursing from some parent table

If we fleshed out the header comment for ATPrepCmd and maybe a few
other key functions along these lines, that would make the logic
a good deal more intelligible, I think.

regards, tom lane

Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> writes:

... but I
can't remember what could recurse = false, recursing = true mean.

It'd mean that we are operating on a child table but should not
recurse to its children (if any). Not sure if the case actually
arises, although it could make sense if we flattened the inheritance
tree into a list of target tables earlier on, which does happen in
many cases. But most of the code paths that do it that way don't
bother with recurse/recursing parameters.

regards, tom lane

On Jan 20, 2026, at 00:14, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Peter Eisentraut <peter@eisentraut.org> writes:

On 19.01.26 08:14, Chao Li wrote:

Many ALTER TABLE-related functions take two boolean parameters,
"recurse" and "recursing", whose names are easy to confuse.

I'm not bothered by this.

To reduce this confusion, I’m proposing to rename "recurse" to
"no_only", which more directly reflects its meaning.

This seems worse. Especially since no_only is almost a double negative.

Yeah, I don't find this change an improvement either. I think the
actual problem here is lack of documentation: unlike many other
places, there is next to zero commentary in tablecmds.c about what
all the function parameters are. It would probably help to define
these, along the lines of

* recurse: true if we should recurse to children of this table
* recursing: true if we are already recursing from some parent table

If we fleshed out the header comment for ATPrepCmd and maybe a few
other key functions along these lines, that would make the logic
a good deal more intelligible, I think.

regards, tom lane

Enhancing the header comments also helps here.

PSA v2:

0001 - Adds detailed descriptions in the header comments of ATController()
and ATPrepCmd(). For the other 16 functions that take both “recurse” and
“recursing”, I only added brief references to those header comments.

0002 - While working on 0001, I noticed that the header comment of
addFkConstraint() explains all parameters except “is_internal”. I added a
line for “is_internal”, so that the comment fully reflects the function
signature. This change is really trivial and can be squashed into 0001 if
preferred.

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

Chao Li <li.evan.chao@gmail.com> 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

On Jan 21, 2026, at 04:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Chao Li <li.evan.chao@gmail.com> 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/