On Mon, Feb 16, 2026 at 8:50 AM vignesh C <[email protected]> wrote:
>
> On Thu, 12 Feb 2026 at 13:58, David G. Johnston
> <[email protected]> wrote:
> >
> > On Wed, Feb 11, 2026 at 11:58 PM Shlok Kyal <[email protected]>
> > wrote:
> >>
> >>
> >> We have addressed the comments in the latest v43 patch.
> >
> >
> > Non-comprehensive review.
> >
> > Shouldn't the early exits look like:
> >
> > <begin function>
> > if (list_length(publications) < 2)
> > return;
> >
> > perform query here, capture except_publications
> >
> > if (list_length(except_publications) < 2)
> > return;
> >
> > construct error message and ereport
> > <end function>
>
> Modified
>
> >
> > + if (publication_has_any_exception(puboid))
> > + {
> > + except_pub_names = lappend(except_pub_names,
> > + makeString(pubform->pubname.data));
> > + has_any_exclusion = true;
> > + except_pub_id = pubform->oid;
> > + }
> >
> > Either rename has_any_exclusion to has_any_exception or, given how
> > ambiguous that reads in code, maybe standardize on calling these exclusions
> > throughout the code and either just accept we've chosen EXCEPT for the
> > syntax for good reasons or consider whether to EXCLUDING (table1, table2)
> > would be a better choice.
>
> Changed it to has_any_exception
>
> > + errmsg("could not get non excluded table list for table \"%s.%s\" from
> > publisher: %s", -- triple negative; try to avoid "non excluded" as a term -
> > those are "included".
> >
> > pg_get_publication_effective_tables - the second input argument is an array
> > of publication names so the singular form here is a bit misleading. Should
> > this be subscription-oriented? Otherwise, pg_get_tables_from_publications
> > seems like a more accurate name. "effective" or "only the included ones"
> > seems reasonable to imply.
>
> We have used similarly in the existing pg_get_publication_tables, I
> felt the existing might be ok to keep it consistent unless there is a
> better consistent name.
>
> > Related, the check in there for "does at least one publication have an
> > exclusion list" makes sense but feels awfully similar to the check for "at
> > most one publication has an exclusion list"...too late for me to figure out
> > what if anything to do make/do about it though.
>
> Ideally this is not expected to happen. But under rare cases it can
> happen if the publication is dropped/recreated with except after
> creating a subscription. It is better to throw an error in these cases
> as we do not allow multiple publications except tables.
>
> Thanks for the comments, the attached v44 version patch has the
> changes for the same.
I started looking into the patch, I have a few comments/questions
1.
+ <para>
+ Similarly, if another publication includes the same table with a row
+ filter, but it is also covered by a
+ <literal>FOR ALL TABLES ... EXCEPT</literal> publication, the table is
+ published without applying the row filter, since row filters cannot be
+ specified for <literal>FOR ALL TABLES ... EXCEPT</literal> publications
+ and such publications are therefore treated as having no row filter.
+ </para>
I did not understand what this paragraph is trying to explain? what
do you mean by it is covered by FOR ALL TABLES ... EXCEPT, does it
mean the relation is not excluded by EXCEPT?
2.
+ <para>
+ Create a publication that publishes all sequences for synchronization, and
+ all changes in all tables except <structname>users</structname> and
+ <structname>departments</structname>:
+<programlisting>
+CREATE PUBLICATION all_sequences_tables_except FOR ALL SEQUENCES, ALL
TABLES EXCEPT (users, departments);
Do we support EXCEPT SEQUENCES as well or that's for the future?
3.
+/* Helper: Check syscache for prexcept flag */
+bool
+is_relid_published_as_except(Oid relid, Oid pubid)
+{
IMHO the function name doesn't look great for its purpose, we just
want to check whether the relid is excluded, published_as_except seems
confusing, no?
4.
+/*
+ * Throws an ERROR if multiple publications with exceptions are found.
+ *
+ * This check is mandatory because logical replication currently does not
+ * support subscribing to multiple publications if more than one of them
+ * uses an exclusion list.
+ */
IMHO explaining the reason why this is not supported or giving
reference to any other comments where it is already explained would be
useful.
5.
+ /*
+ * If the root ancestor is covered by a FOR ALL TABLES
+ * EXCEPT publication that uses
+ * publish_via_partition_root, we must publish changes via
+ * the root identity.
+ */
+ if (root_published_via_exceptpub)
+ {
+ pub_relid = root_ancestor;
+ ancestor_level = list_length(ancestors);
+ }
I find this comment a bit confusing, what does "covered by a FOR ALL
TABLES EXCEPT publication" means? It means relation is not in the
EXCEPT list or in the EXCEPT list?
--
Regards,
Dilip Kumar
Google
