Re: ATPrepCmd: cleanup unreachable AT_AddIndexConstraint handling

Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> writes:
> On Jan 28, 2026, at 14:14, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)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