ATPrepCmd: cleanup unreachable AT_AddIndexConstraint handling

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, ATT_TABLE | ATT_PARTITIONED_TABLE);
/* This command never recurses */
/* No command-specific prep needed */
pass = AT_PASS_ADD_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.

I thought over and decided to delete AT_AddIndexConstraint from ATPrepCmd, which should be cleaner.

PFA v2:

* Deleted AT_AddIndexConstraint from ATPrepCmd
* Added a comment under AT_AddConstraint to explain how AT_AddIndexConstraint is handled

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

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.

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

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.

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

Thanks for the guidance.

I verified AT_AddIndex with:
```
create table t1 (id int);
alter table t1 add primary key (id);
```

“Add primary key” is also initially parsed as AT_AddConstraint, then transformed to AT_AddIndex during the execution phase.

So in v3 I reverted back to the v1 approach and placed AT_AddIndex next to AT_AddIndexConstraint 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/