On Fri, May 15, 2026 at 12:49 PM Peter Smith <[email protected]> wrote: > > Hi Nisha. > > Some review comments for patch v5-0001.
Thanks Peter for the review. > > ~~~ > > GetIncludedPublicationRelations: > > 6. > - * This should only be used FOR ALL TABLES publications. > + * This is used for FOR ALL TABLES and FOR TABLES IN SCHEMA publications, > + * both of which support EXCEPT TABLE. > > So, because it might come from 2 places, shouldn't the assert in this > function be modified? > I think you meant GetExcludedPublicationTables. Patch-0003 updates the assert in this fuction for schema publications. > Also, since the assert was not yet modified, how does this even pass > the tests if 'alltables' was false? > As you pointed out in the next (7th) comment also, the first two patches are not calling GetExcludedPublicationTables(), but are using get_publication_relations() directly. Hence, the tests are passing even without the assert modification. But patch-0003 does call it, so the assert was updated there. > ~~~ > > GetAllSchemaPublicationRelations: > > 7. > + /* get the list of tables excluded via EXCEPT TABLE for this publication */ > + if (pubschemalist != NIL) > + exceptlist = get_publication_relations(pubid, pub_partopt, true); > + > > Should this be calling 'GetExcludedPublicationTables' instead of > directly calling get_publication_relations? > Agreed, I will make the change. With that, the above mentioned assert will also need to be updated in patch-1 itself. ~~~~ I agree with the rest of the comments and will update and upload the patches soon. -- Thanks, Nisha
