Here are some review comments for the v22* patches. ////////// Patch 0001 //////////
src/backend/commands/publicationcmds.c OpenRelationList: 1. static List * -OpenTableList(List *tables) +OpenRelationList(List *relations) The static declaration of OpenRelationList and the associated CloseRelationList function called this parameter `rels`, so it might be better to stay consistent with that. ////////// Patch 0002 ////////// On Thu, Jul 30, 2026 at 10:50 PM Shlok Kyal <[email protected]> wrote: > ... > > EXCEPT: > > > > 3. > > Nisha's EXCEPT thread docs [2;patch 0005] have an EXCEPT parameter on this > > page. > > e.g. > > + <varlistentry> > > + <term><literal>EXCEPT</literal></term> > > > > I think you should do the same, because that will make merge/rebase > > easier later when both these patches eventually get pushed. Please > > refer to the other thread to make yours similar. > > > I am not sure why an EXCEPT parameter is need on > alter_publication.sgml. We have already described the behaviour of > EXCEPT with ALL SEQUENCE and ALL TABLES in a paragraph earlier in the > same page. > Also we have described the EXCEPT parameter in > create_publication.sgml. Won't it be redundant to add the same > description again? 1. Here are some reasons: a. IMO the current ALTER page's "Description" is already unwieldy in comparison to other pages (compare it to CREATE PUBLICATION), so I predict it will need a make-over sometime (that can be much later after both these threads are done). So adding EXCEPT now is just a small step in that direction. b. Description is already large, but Nisha's SCHEMA EXCEPT patch needed to add much more to it, so instead we opted for a Parameter EXCEPT in that thread to keep it manageable. c. There is no need for redundant information. Of course, ALTER ... EXCEPT can xref refer back to CREATE ... EXCEPT for common information, so it's not just a cut/paste of content. But there are some special considerations for ALTER to explain -- e.g. only way to modify exclusions is to use SET, and that doing so completely overwrites any pre-existing exclusions etc. Anyway, you can skip adding an EXCEPT parameter if you disagree. But AFAIK, the SCHEMA EXCEPT patch is likely to be pushed ahead of your EXCEPT SEQUENCE one, so you will have to rebase accordingly anyway. I'm just suggesting it's better to try to stay compatible with Nisha's patch now, instead of having to rebase too much later. ====== src/backend/commands/publicationcmds.c get_delete_rels: 2. + /* + * Validate the column list. If the column list or WHERE clause + * changes, then the validation done here will be duplicated + * inside PublicationAddRelations(). The validation is cheap + * enough that that seems harmless. + */ + if (newrelid == oldrelid && + equal(oldwhereexpr, newpubrel->whereClause)) + { + Bitmapset *newcolumns = pub_collist_validate(newpubrel->relation, + newpubrel->columns); + + if (bms_equal(oldcolumns, newcolumns)) + { + found = true; + break; + } + } The comment saying "Validate the column list" seems premature. The outer comment should be explaining the need to compare row filters and column lists. If you want to describe more about column list validation then that should be a comment *inside* the if where that validation happens. ====== Kind Regards, Peter Smith. Fujitsu Australia
