On Sat, 28 Mar 2026 at 00:33, SATYANARAYANA NARLAPURAM <[email protected]> wrote: > > Hi, > > On Fri, Mar 27, 2026 at 12:50 AM Shlok Kyal <[email protected]> wrote: >> >> >> I have addressed the comments. Attached the updated patch. >> >> [1]: >> https://www.postgresql.org/message-id/CAA4eK1Jjm+w3hEGgsDu_r1Pysez=8mmtGu6=xwpe4muh+ey...@mail.gmail.com > > > A few minor comments: > > 1. Add a negative test for missing table keyword for example EXCEPT (t1, t2) > 2. NIT comment, remove the extra space between parenthesis and TABLE below > > else if (Matches("CREATE", "PUBLICATION", MatchAny, "FOR", "ALL", "TABLES")) > - COMPLETE_WITH("EXCEPT TABLE (", "WITH ("); > + COMPLETE_WITH("EXCEPT ( TABLE", "WITH ("); > > 3. for pg_dump, to improve readability can you just add TABLE keyword for > every tables in the except list? > > - /* Include EXCEPT TABLE clause if there are except_tables. */ > + /* Include EXCEPT (TABLE) clause if there are except_tables. */ > for (SimplePtrListCell *cell = pubinfo->except_tables.head; cell; cell = > cell->next) > { > TableInfo *tbinfo = (TableInfo *) cell->ptr; > > if (++n_except == 1) > - appendPQExpBufferStr(query, " EXCEPT TABLE ("); > + appendPQExpBufferStr(query, " EXCEPT (TABLE "); > else > appendPQExpBufferStr(query, ", "); > > 4. Is the Assert needed below, code already produces error message. > > + Assert(pubobj->pubobjtype == PUBLICATIONOBJ_EXCEPT_TABLE); > + > + if (pubobj->pubobjtype == PUBLICATIONOBJ_CONTINUATION) > + ereport(ERROR, > + errcode(ERRCODE_SYNTAX_ERROR), > + errmsg("invalid publication object list"), > + errdetail("TABLE must be specified before a standalone table."), > + parser_errposition(pubobj->location)); I think the Assert should stay as a defensive check to ensure this function is only used for EXCEPT (TABLE) cases.
Thanks for reviewing the patch. I have addressed the remaining comments and attached the patch in [1]. [1]: https://www.postgresql.org/message-id/CANhcyEUQvEK%2BHOH6Y8Fy30fNvC631ZopWKhwgskXjKnuXiGV5Q%40mail.gmail.com Thanks, Shlok Kyal
