Обсуждение: ATPrepCmd: cleanup unreachable AT_AddIndexConstraint handling

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
actuallyunreachable. 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
isunreachable. I did think about just deleting the branch, but decided to keep it: if it were removed entirely, readers
mightwonder 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
changefelt 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
aknown 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/

> 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
isactually 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
pathis unreachable. I did think about just deleting the branch, but decided to keep it: if it were removed entirely,
readersmight 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
isactually 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
pathis unreachable. I did think about just deleting the branch, but decided to keep it: if it were removed entirely,
readersmight 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
branchis 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
pathis unreachable. I did think about just deleting the branch, but decided to keep it: if it were removed entirely,
readersmight 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
themin 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/