Hi Ayush, All the points from my last review are fixed.
A few small new comments, none are bugs: 1) No tab-completion after "BY NAME"/"BY POSITION" is typed (unlike OVERRIDING, which does complete SELECT/VALUES/TABLE after it). 2) "INSERT ... BY NAME TABLE other_table" works but has no regression test. Regards, Vaibhav Dalvi EnterpriseDB On Sat, Sep 12, 2026 at 9:12 PM Ayush Tiwari <[email protected]> wrote: > Hi, > > Thanks for the review. > > I've attached v2 that addresses both Marcos and your points. > > Marcos wrote: > > > it would be good to mention that only unnamed values don't work. > > The docs now call the unsupported form a "bare VALUES source", and show > the named form: > > INSERT INTO t1 BY NAME > SELECT * FROM (VALUES (1, 2)) AS v(c2, c1); > > > And is missing a test for select *, right? > > Added. > > On Thu, 10 Sept 2026 at 13:50, Vaibhav Dalvi > <[email protected]> wrote: > > > This is a solid patch but here are my observations: > > > > 1. The BY NAME + VALUES/DEFAULT VALUES rejection check runs before the > target > > table/column are validated, which masks the real error: > > > > postgres=# INSERT INTO no_such_table BY NAME VALUES (1,2); > > ERROR: cannot use BY NAME with VALUES > > HINT: BY NAME requires a query, such as a SELECT, as the data source. > > > > It reports "cannot use BY NAME with VALUES" instead of "relation does > not exist". > > This is an ordering bug, easy to fix by moving the check after > setTargetTable()/checkInsertTargets(). > > I moved it after setTargetTable() and checkInsertTargets(), so relation and > attname errors win, but bare VALUES is still rejected before source > transformation. Thanks for raising this. > > > 2. srccolnames list is not needed: It's a separate list built only to > > carry tle->resname values, but the same names are already available on > > selectQuery->targetList. transformInsertColsByName could just > > read them from there directly, one less list to carry around. > > Dropped. transformInsertColsByName() now walks selectQuery->targetList, > skips resjunk TLEs, matches on resname, and uses tle->expr for errposition. > > > 3. Grammar has too many hand-written alternatives: > > insert_rest grows from 5 to 13 almost-identical rules to cover > > every order of BY NAME/POSITION with OVERRIDING and the column > > list. Each one repeats the same 3 assignments by hand. The > > codebase already has a pattern for this kind of thing > > (opt_unique_null_treatment), worth factoring this the same way so > > a future change does not need to touch 8 places at once. > > I took a narrower route here: the old insert_rest productions stay as-is, > while insert_by_options covers only the new BY forms. My thought was to > avoid repeating the semantic actions without routing existing INSERT syntax > through new grammar. Does that seem like a reasonable boundary? > > > 4. matched Bitmapset duplicates new_attrnos: > > Both matched and new_attrnos are built in the same loop and hold > > the same information. bms_is_member(x, matched) could just be > > list_member_int(new_attrnos, x), one less variable to keep in > > sync. > > Dropped. The duplicate check now uses list_member_int(new_attrnos, > matchattno). > > I also added psql completion. > > Thoughts on the v2? > > Regards, > Ayush >
