Re: Support EXCEPT for TABLES IN SCHEMA publications

On Fri, May 22, 2026 at 7:57 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Nisha.
>
> Here are some review comments for patch v6-0001.
>

Thanks for the review. All comments are addressed in v7. Please find
responses below for a few of the comments.

> ======
> src/backend/catalog/pg_publication.c
>
> GetTopMostAncestorInPublication:
>
> 1.
> +GetTopMostAncestorInPublication(Oid puboid, List *ancestors,
> + int *ancestor_level, List *except_pubids)
>
> I am having dificulty understanding this function. There needs to be a
> description what does the input parameter 'except_pubids' mean. The
> param name doesn't tell me anything much -- just that it is a list of
> pubids that "something" (what?) is excluded from. And how does that
> relate to the 'ancestors'?
>

Updated the function comments to explain except_pubids.

> ~~~
>
> 2.
> {
> aschemaPubids = GetSchemaPublications(get_rel_namespace(ancestor));
> - if (list_member_oid(aschemaPubids, puboid))
> + if (list_member_oid(aschemaPubids, puboid) &&
> + !list_member_oid(except_pubids, puboid))
>
> Is this new code in the right place? I'm not 100% sure of the
> 'except_pubids' details, but shouldn't it be checked sooner? e.g. if
> we know already that this pubid is no good
> (!list_member_oid(except_pubids, puboid)) then what is the point to
> even assign/check aschemaPubids?
>

except_pubids represents the set of publication OIDs from which the
root relation has been explicitly excluded via EXCEPT (TABLE ...).
But yes, we can check it before computing schemaPubids. Fixed.

> ~~~
>
> 3.
> + if (is_except)
> + ereport(ERROR,
> + (errcode(ERRCODE_DUPLICATE_OBJECT),
> + errmsg("table \"%s\" cannot be added because it is excluded from
> publication \"%s\"",
> + RelationGetQualifiedRelationName(targetrel),
> + pub->name)));
> + else
> + ereport(ERROR,
> + (errcode(ERRCODE_DUPLICATE_OBJECT),
> + errmsg("relation \"%s\" is already member of publication \"%s\"",
> + RelationGetRelationName(targetrel), pub->name)));
>
> Fully qualified 'targetrel' in the first error, but not in the second? Is it OK?
>

IMO, we should use fully qualified names in both cases. Though the
second error is not part of this patch, I’ve updated it as well.
Please let me know if you think otherwise.

> ======
> src/backend/replication/pgoutput/pgoutput.c
>
> get_rel_sync_entry:
>
...
>
> 5b.
> Actually, this is becoming a GENERAL comment. There too many ways that
> these EXCEPT tables are getting named, and it is causing confusion:
> - except_pubids
> - exceptlist
> - exceptrelations
> - exceptrels
> - except_relid
> - except_tables
> - schemaExceptPubids
>
> Can we standardize on some common names, to make all the code more consistent?
>

I have now used the "except_" prefix consistently for all names to
avoid confusion. Also updated the naming in all places as below:
except_rel_names: parse-level table names in EXCEPT
except_rels: internal relations in the EXCEPT list (opened/accessed)
except_relids: list of relation OIDs wherever used
except_pubids: publication OIDs from which a given relation (or its
root) is excluded

> ~
>
> 5c.
> Previously, the result of 'get_partition_ancestors' was being freed,
> but now it is not. I'm not sure how importatnt that is, because I
> found other examples in PG source code also not freeing...
>

Sorry, this was mistakenly removed in v6 while cleaning up the code,
and I did not verify it against the older version. Fixed now.

> ======
> src/bin/psql/tab-complete.in.c
>
> 6.
> + else if (Matches("CREATE", "PUBLICATION", MatchAny, "FOR", "TABLES",
> "IN", "SCHEMA", MatchAny, "EXCEPT", "(", "TABLE", MatchAnyN) &&
> ends_with(prev_wd, ','))
> + COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables);
>
> I'm not sure if this is working as intended.
>
> When testing for multiple except tables I get results like:
> ----
> test_pub=# create publication pub1 for tables IN SCHEMA myschema <TAB>
> EXCEPT ( TABLE WITH (
> test_pub=# create publication pub1 for tables IN SCHEMA myschema
> except ( table <TAB>
> test_pub=# create publication pub1 for tables IN SCHEMA myschema
> except ( table myschema.t<TAB>
> myschema.t1 myschema.t2 myschema.t3
> test_pub=# create publication pub1 for tables IN SCHEMA myschema
> except ( table myschema.t1,<TAB>
> information_schema. myschema. public. t1
> t2 t3
> ----
>
> Note: it is offering suggstions for schema names outside of the
> "myschema". Should this code be calling
> Query_for_list_of_tables_in_schema instead of
> Query_for_list_of_tables?
>

For this case, I don't think it's really possible to keep suggesting
after a comma. Even if we handle a fixed number of comma-separated
entries, the same problem reappears with the next entry.
Query_for_list_of_tables_in_schema needs a correct schema reference,
but with each comma the relative offset changes, so there is no single
fixed prev*_wd that can reliably point to the schema across all
entries.

But I see, the current call to Query_for_list_of_tables is clearly
incorrect here. I have now suppressed suggestions after a comma,
instead of showing incorrect suggestions.
Thoughts?

--
Thanks,
Nisha