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
