pgsql-hackers
❮
ATPrepCmd: cleanup unreachable AT_AddIndexConstraint handling
- Jump to comment-1Chao Li<li.evan.chao@gmail.com>Jan 28, 2026, 6:15 AM UTCHi Hackers,
While working on the patch [1], I was looking at the handling of AT_AddIndexConstraint in ATPrepCmd():
I initially thought the comment about “never recurses” was stale, but after some debugging, I found that this branch is actually unreachable. So leaving the code and comments in an unreachable branch would be confusing for readers.ATPrepCmd() { case AT_AddIndexConstraint: /* ADD CONSTRAINT USING INDEX */ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_PARTITIONED_TABLE); /* This command never recurses */ /* No command-specific prep needed */ pass = AT_PASS_ADD_INDEXCONSTR; }
This patch cleans up the handling by putting an Assert(false) there and adding a comment to explain why this code path is unreachable. I did think about just deleting the branch, but decided to keep it: if it were removed entirely, readers might wonder why AT_AddIndexConstraint is not handled in ATPrepCmd() and end up spending time debugging this themselves.
Two things to note in the patch:
1) The test edits are purely cosmetic. They just change lowercase keywords to uppercase to align with surrounding SQL statements, and remove an unnecessary whitespace. I noticed this while debugging ADD CONSTRAINT USING INDEX, and the change felt too trivial to file a separate patch.
2) There is an unnecessary empty line after "case AT_AddIndexConstraint:" that was added by pgindent. I believe this is a known pgindent issue that I reported before.
With this patch applied, all regression tests pass.
[1] https://postgr.es/m/CAEoWx2kggo1N2kDH6OSfXHL_5gKg3DqQ0PdNuL4LH4XSTKJ3-g@mail.gmail.com
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/- Jump to comment-1Chao Li<li.evan.chao@gmail.com>Jan 29, 2026, 2:00 AM UTC
On Jan 28, 2026, at 14:14, Chao Li <li.evan.chao@gmail.com> wrote:
Hi Hackers,
While working on the patch [1], I was looking at the handling of AT_AddIndexConstraint in ATPrepCmd():
```
ATPrepCmd()
{
case AT_AddIndexConstraint: / ADD CONSTRAINT USING INDEX /ATSimplePermissions(cmd->subtype, rel, ATTTABLE | ATTPARTITIONED_TABLE);
/ This command never recurses /
I thought over and decided to delete AT_AddIndexConstraint from ATPrepCmd, which should be cleaner.
/ No command-specific prep needed /
pass = ATPASSADD_INDEXCONSTR;
}
```
I initially thought the comment about “never recurses” was stale, but after some debugging, I found that this branch is actually unreachable. So leaving the code and comments in an unreachable branch would be confusing for readers.
This patch cleans up the handling by putting an Assert(false) there and adding a comment to explain why this code path is unreachable. I did think about just deleting the branch, but decided to keep it: if it were removed entirely, readers might wonder why AT_AddIndexConstraint is not handled in ATPrepCmd() and end up spending time debugging this themselves.
PFA v2:
* Deleted AT_AddIndexConstraint from ATPrepCmd
* Added a comment under ATAddConstraint to explain how ATAddIndexConstraint is handled
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/- Jump to comment-1Tom Lane<tgl@sss.pgh.pa.us>Jan 29, 2026, 5:13 AM UTCChao Li <li.evan.chao@gmail.com> writes:
On Jan 28, 2026, at 14:14, Chao Li <li.evan.chao@gmail.com> wrote:
I initially thought the comment about “never recurses” was stale, but after some debugging, I found that this branch is actually unreachable. So leaving the code and comments in an unreachable branch would be confusing for readers.
This patch cleans up the handling by putting an Assert(false) there and adding a comment to explain why this code path is unreachable. I did think about just deleting the branch, but decided to keep it: if it were removed entirely, readers might wonder why AT_AddIndexConstraint is not handled in ATPrepCmd() and end up spending time debugging this themselves.I thought over and decided to delete AT_AddIndexConstraint from ATPrepCmd, which should be cleaner.
Your first version was very substantially better. The Assert is
important to help debug things if somebody changes the parsing
logic in a way that falsifies the assumption that we can't get
here for AT_AddIndexConstraint. And, as you thought originally,
it's better to clearly document why we think this case is
unreachable than to leave it looking like possibly an oversight.
(I do not think a comment in some other case-branch accomplishes
that.)
Also, a look at the code coverage report suggests that the same
might be true for AT_AddIndex. Can we replace that branch too
with an Assert(false)?
Matter of taste perhaps, but if I were committing this I would
drop these case-folding-only changes in the regression tests.
That's just useless code churn, accomplishing nothing except
to create a hazard for possible future back-patches.regards, tom lane- Jump to comment-1Chao Li<li.evan.chao@gmail.com>Jan 29, 2026, 6:37 AM UTC
On Jan 29, 2026, at 13:12, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Chao Li <li.evan.chao@gmail.com> writes:
On Jan 28, 2026, at 14:14, Chao Li <li.evan.chao@gmail.com> wrote:
I initially thought the comment about “never recurses” was stale, but after some debugging, I found that this branch is actually unreachable. So leaving the code and comments in an unreachable branch would be confusing for readers.
This patch cleans up the handling by putting an Assert(false) there and adding a comment to explain why this code path is unreachable. I did think about just deleting the branch, but decided to keep it: if it were removed entirely, readers might wonder why AT_AddIndexConstraint is not handled in ATPrepCmd() and end up spending time debugging this themselves.I thought over and decided to delete AT_AddIndexConstraint from ATPrepCmd, which should be cleaner.
Thanks for the guidance.
Your first version was very substantially better. The Assert is
important to help debug things if somebody changes the parsing
logic in a way that falsifies the assumption that we can't get
here for AT_AddIndexConstraint. And, as you thought originally,
it's better to clearly document why we think this case is
unreachable than to leave it looking like possibly an oversight.
(I do not think a comment in some other case-branch accomplishes
that.)
Also, a look at the code coverage report suggests that the same
might be true for AT_AddIndex. Can we replace that branch too
with an Assert(false)?
Matter of taste perhaps, but if I were committing this I would
drop these case-folding-only changes in the regression tests.
That's just useless code churn, accomplishing nothing except
to create a hazard for possible future back-patches.
regards, tom lane
I verified AT_AddIndex with:
“Add primary key” is also initially parsed as ATAddConstraint, then transformed to ATAddIndex during the execution phase.create table t1 (id int); alter table t1 add primary key (id);
So in v3 I reverted back to the v1 approach and placed ATAddIndex next to ATAddIndexConstraint in ATPrepCmd, putting them in the same branch so they share the same assertion and explanatory comment.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/