MIME-Version: 1.0
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>
In-Reply-To: 
 <CAHut+PuiK_Pa=BkSgBxYzqf1PYh+mcUcUQCr8r1e69-y1r+hhw@mail.gmail.com>
From: Nisha Moond <nisha.moond412@gmail.com>
Date: Thu, 28 May 2026 16:56:56 +0530
Message-ID: 
 <CABdArM7rH+3GekRgufEOwrJxUeQk=LB182CQwJD35e0oN7q8ZA@mail.gmail.com>
Subject: Re: Support EXCEPT for TABLES IN SCHEMA publications
To: Peter Smith <smithpb2250@gmail.com>
Cc: shveta malik <shveta.malik@gmail.com>,
 Amit Kapila <amit.kapila16@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CABdArM7rH%2B3GekRgufEOwrJxUeQk%3DLB182CQwJD35e0oN7q8ZA%40mail.gmail.com>
Precedence: bulk

 On Fri, May 22, 2026 at 7:57=E2=80=AFAM Peter Smith <smithpb2250@gmail.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.

> =3D=3D=3D=3D=3D=3D
> 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 =3D 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=E2=80=99ve updated it as well.
Please let me know if you think otherwise.

> =3D=3D=3D=3D=3D=3D
> 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 consis=
tent?
>

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.

> =3D=3D=3D=3D=3D=3D
> 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=3D# create publication pub1 for tables IN SCHEMA myschema <TAB>
> EXCEPT ( TABLE  WITH (
> test_pub=3D# create publication pub1 for tables IN SCHEMA myschema
> except ( table <TAB>
> test_pub=3D# create publication pub1 for tables IN SCHEMA myschema
> except ( table myschema.t<TAB>
> myschema.t1  myschema.t2  myschema.t3
> test_pub=3D# 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