Chao Li <[email protected]> writes:
> On Jan 28, 2026, at 14:14, Chao Li <[email protected]> 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
