Hi, On Mon, Feb 23, 2026 at 11:37 AM Shlok Kyal <[email protected]> wrote: > > On Fri, 20 Feb 2026 at 18:17, Ashutosh Sharma <[email protected]> wrote: > > > > Hi, > > > > On Fri, Feb 20, 2026 at 2:08 PM vignesh C <[email protected]> wrote: > > > > > > On Thu, 19 Feb 2026 at 16:06, Amit Kapila <[email protected]> wrote: > > > > > > > > On Thu, Feb 19, 2026 at 10:13 AM Shlok Kyal <[email protected]> > > > > wrote: > > > > > > > > > > Thanks for reviewing the patch. I have addressed the remaining > > > > > comments in the v46 patch.. > > > > > > > > > > > > > Can we think of some ideas to split this patch? One possibility is > > > > that in the first patch we give an ERROR, if a non-root partitioned > > > > table is present in EXCEPT Clause. I see that a lot of code is written > > > > to handle partitions in EXCEPT clause. I feel such a split will make > > > > code easier to review and validate. > > > > > > Split it accordingly. > > > > > > > Few other comments: > > > > ================= > > > > 1. > > > > if (stmt->for_all_tables) > > > > { > > > > + /* Process EXCEPT table list */ > > > > + if (relations != NIL) > > > > + { > > > > + Assert(rels != NIL); > > > > + PublicationAddTables(puboid, rels, true, NULL); > > > > + } > > > > + > > > > /* > > > > * Invalidate relcache so that publication info is rebuilt. Sequences > > > > * publication doesn't require invalidation, as replica identity > > > > CacheInvalidateRelcacheAll(); > > > > > > > > Here, the relations listed in the except table list will be > > > > invalidated twice, once inside > > > > PublicationAddTables->publication_add_relation, and second time via > > > > CacheInvalidateRelcacheAll. Can we avoid that by adding a parameter to > > > > PublicationAddTables? > > > > > > Fixed this > > > > > > > 2. > > > > - root_relids = GetPublicationRelations(pubform->oid, > > > > - PUBLICATION_PART_ROOT); > > > > + root_relids = GetIncludedRelationsByPub(pubform->oid, > > > > + PUBLICATION_PART_ROOT); > > > > > > > > To follow the previous function naming pattern, can we rename > > > > GetIncludedRelationsByPub to GetIncludedPublicationRelations? If we > > > > agree to this then rename the excluded version of the function as > > > > well. > > > > > > Modified > > > > > > > 3. > > > > +/* > > > > + * Return the list of relation Oids for a publication. > > > > + * > > > > + * For a FOR TABLE publication, this returns the list of relations > > > > explicitly > > > > + * included in the publication. > > > > + * > > > > + * Publications declared with FOR ALL TABLES/SEQUENCES should use > > > > + * GetAllPublicationRelations() to obtain the complete set of > > > > tables/sequences > > > > + * covered by the publication. > > > > + */ > > > > +List * > > > > +GetIncludedRelationsByPub(Oid pubid, PublicationPartOpt pub_partopt) > > > > > > > > This is equivalent to the existing function GetPublicationRelations() > > > > which has more precise comments atop. We can use the same comments > > > > unless there is any functionality difference. > > > > > > Updated it > > > > > > > 4. > > > > --- a/src/backend/catalog/pg_publication.c > > > > +++ b/src/backend/catalog/pg_publication.c > > > > @@ -29,6 +29,7 @@ > > > > #include "catalog/pg_publication.h" > > > > #include "catalog/pg_publication_namespace.h" > > > > #include "catalog/pg_publication_rel.h" > > > > +#include "catalog/pg_subscription.h" > > > > > > > > It appears odd to include pg_subscription.h in the publication code. > > > > Is there a reason for the same? If not then avoid it. > > > > > > This was required because we now started using GetPublicationsStr. I > > > have moved GetPublicationsStr to logical.c from pg_subscription which > > > is common to both pub and sub. > > > > > > > Apart from above, a few cosmetic changes are attached. > > > > > > Merged them. > > > > > > The attached v47 version patch has the changes for the same. > > > > > > > SIGABRT with normal tables: > > -------------------------------------- > > > > ashutosh@test=# select > > pg_get_publication_effective_tables('t1'::regclass, > > '{all_pub}'::text[]); > > server closed the connection unexpectedly > > This probably means the server terminated abnormally > > before or while processing the request. > > The connection to the server was lost. Attempting reset: Failed. > > > > Since the function is user-facing, I think we should have test coverage for > > it. > > > Hi Ashutosh, > Thanks for reviewing the patch. > > We have fixed this issue. > This function is not user facing. We have a similar function > 'pg_get_publication_tables'. We do not have documentation and test > coverage for this function. > I think 'pg_get_publication_effective_tables' is a similar function, > so we should not add documentation or tests for it. > > We have also addressed the comments in [1]. I have also done minor > code and comment modifications > I have also modified the error message as suggested by Shveta in [2]. > Attached the latest v48 patch. > > [1]: > https://www.postgresql.org/message-id/CAE9k0P=sfawuybosmk9zxwey774ojw5g4gb7+fa3cq22j83...@mail.gmail.com > [2]: > https://www.postgresql.org/message-id/CAJpy0uAHr8=yuksyebs8g4p0zggb6mcur-yjdb56tdymf9y...@mail.gmail.com >
Thanks for the fix. A couple of quick observations: + list_free_deep(pubnames_with_except); + pfree(except_publications); The memory cleanup is inconsistent - some lists are freed with list_free_deep while others use a plain pfree. Could you clarify the reasoning, or make this consistent? -- + /* walrcv_clear_result(res); */ + ExecDropSingleTupleTableSlot(slot); walrcv_clear_result(res) appears to have been commented out — was this intentional? If so, what's the reason? -- There are multiple places (at least 3) where we have multi-pub conflict checks to ensure not more than one pub with except clause exist, it doesn't seem like we can make them common, but since they are somewhat duplicate its possible that when we make changes at one place, some other place where the similar change is required gets missed. So if possible (and if it makes sense to you) I think it would be good to have some kind of comments saying "this is one of N places that enforces this constraint, see also X and Y. -- With Regards, Ashutosh Sharma.
