public inbox for [email protected]
help / color / mirror / Atom feedFrom: Nisha Moond <[email protected]>
To: Peter Smith <[email protected]>
Cc: shveta malik <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Support EXCEPT for TABLES IN SCHEMA publications
Date: Thu, 28 May 2026 16:56:56 +0530
Message-ID: <CABdArM7rH+3GekRgufEOwrJxUeQk=LB182CQwJD35e0oN7q8ZA@mail.gmail.com> (raw)
In-Reply-To: <CAHut+PuiK_Pa=BkSgBxYzqf1PYh+mcUcUQCr8r1e69-y1r+hhw@mail.gmail.com>
References: <CABdArM5sw4Q1ZU8HGdo4BSc1A_+8xtUNq17j6wcir=yMUy19Cg@mail.gmail.com>
<CAHut+PvnH8QHa035Skoh1e9jm_H08DO9fQ=F-NAMsEpYf0RZ2Q@mail.gmail.com>
<CAJpy0uDu0LcNXcZCP0cR_LHqo+sau33KwPFHemmGVYf_JTxRBQ@mail.gmail.com>
<CAA4eK1KbCWBmEXH-rhQjKgNwq=onZp8vRR-QkRhPpbKwL-kQdw@mail.gmail.com>
<CAHut+Pvj4=GWoJEd4EBdp4pi6KxXQ46ioW=PV+=UktiXr2gCvg@mail.gmail.com>
<CABdArM75F0A+DGP8AOt-_b_XREX40rvFid1jRjnr_+S5b51t8Q@mail.gmail.com>
<CAJpy0uDTshb243L5yEYWB3uO-JrwSoRqQDNovh03K2GZuuR3Pg@mail.gmail.com>
<CAJpy0uDy97ULmJUwPacAzc5u2seuPK6RXgCS1rnsW2MfR4eeSw@mail.gmail.com>
<CABdArM6oXXXSAxxXFktTTfBf4kyxJCvdNtTbUZtSwJ=CepN+Xw@mail.gmail.com>
<CAJpy0uBqM+fq7+g1ZRATuY16H10MFP9i25wfFCYCE5MGu+PE0Q@mail.gmail.com>
<CABdArM4uKaS1coCQj6rAwMmHqU_cCJyEWNic-PFF1_ZjDDM82Q@mail.gmail.com>
<CAHut+Pu5VNakf5JAhKM7T-P_q37eN1Qgv5nvZUe+8RAAT41y4g@mail.gmail.com>
<CABdArM6WTm2gP4pcjVdHT1Nx6zdLKTq7nLPUkzZOhprc95a6Rw@mail.gmail.com>
<CAHut+Ptthc1X-UA8-6zG-iFeCDuoNd+oJRBZ1eCnJ9RNOXjfBQ@mail.gmail.com>
<CABdArM79m7-CTf6KGGGU2QBydFtuonGgfxRSqk-vhwTsH8z1ow@mail.gmail.com>
<CAHut+PuiK_Pa=BkSgBxYzqf1PYh+mcUcUQCr8r1e69-y1r+hhw@mail.gmail.com>
On Fri, May 22, 2026 at 7:57 AM Peter Smith <[email protected]> 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
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Support EXCEPT for TABLES IN SCHEMA publications
In-Reply-To: <CABdArM7rH+3GekRgufEOwrJxUeQk=LB182CQwJD35e0oN7q8ZA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox