> On Jan 29, 2026, at 13:12, Tom Lane <[email protected]> wrote: > > 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
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/
v3-0001-tablecmds-cleanup-unreachable-AT_AddIndex-and-AT_.patch
Description: Binary data
