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>
+ 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.
+ 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.
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.
David J.
