Logical Replication - revisit `is_table_publication` function implementation
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t139232psql -h localhost -U postgresBuilt from patchset v11 (message #11), August 23, 2026 at 06:38 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t139232_11 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t139232_11 && git checkout t139232_11Patchset v11 (message #11) is on t139232_11
Hi, after confirming my understanding of pg_publication_rel [1]/messages/by-id/CAHut+Pv1UKR_bxmN7wcCCpQveHoYprvH-hbdFq8gsaH1Ye7B_w@mail.gmail.com, I
revisited some logical replication internal functions.
Specifically.
* The `is_table_publication` function is for checking if the
publication has a clause like "FOR TABLE t1".
* The `is_schema_publication` function is for checking if the
publication has a clause like "FOR TABLES IN SCHEMA s1".
Notice that neither of these ("FOR TABLE", "FOR TABLES IN SCHEMA")
clauses are possible simultaneously with "FOR ALL TABLES".
And we can readily discover if "FOR ALL TABLES" (aka `puballtables`)
is present from the pubform.
We can use this to optimise and simplify the implementations of the
`is_schema_publication` and `is_table_publication` functions.
PSA patch v1.
AFAICT, the result is:
- less code + simpler logic. e.g. is_table_publication does not check
'prexcept' anymore
- more efficient. e.g. skips unnecessary scanning when puballtables is true.
- more consistent. e.g., both functions are now almost identical.
Thoughts?
======
[1]: /messages/by-id/CAHut+Pv1UKR_bxmN7wcCCpQveHoYprvH-hbdFq8gsaH1Ye7B_w@mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
v1-0001-rewrite-is_table_publication.patchapplication/octet-stream; name=v1-0001-rewrite-is_table_publication.patchDownload+22-28
On Tue, 7 Apr 2026 at 12:32, Peter Smith <smithpb2250@gmail.com> wrote:
Hi, after confirming my understanding of pg_publication_rel [1], I
revisited some logical replication internal functions.Specifically.
* The `is_table_publication` function is for checking if the
publication has a clause like "FOR TABLE t1".
* The `is_schema_publication` function is for checking if the
publication has a clause like "FOR TABLES IN SCHEMA s1".Notice that neither of these ("FOR TABLE", "FOR TABLES IN SCHEMA")
clauses are possible simultaneously with "FOR ALL TABLES".And we can readily discover if "FOR ALL TABLES" (aka `puballtables`)
is present from the pubform.We can use this to optimise and simplify the implementations of the
`is_schema_publication` and `is_table_publication` functions.PSA patch v1.
AFAICT, the result is:
- less code + simpler logic. e.g. is_table_publication does not check
'prexcept' anymore
- more efficient. e.g. skips unnecessary scanning when puballtables is true.
- more consistent. e.g., both functions are now almost identical.Thoughts?
I'm not sure if this additional check is sufficient in case of
is_schema_publication. Checking only puballtables can exclude FOR ALL
TABLES, but it still cannot distinguish regular table publications,
empty publications, or sequence publications. In all of those cases,
we still need to check pg_publication_namespace. And also why just
check for puballtables why not to check for puballsequences
+is_schema_publication(Form_pg_publication pubform)
{
Relation pubschsrel;
ScanKeyData scankey;
SysScanDesc scan;
HeapTuple tup;
- bool result = false;
+ bool result;
+
+ /* FOR TABLES IN SCHEMA cannot coexist with FOR ALL TABLES. */
+ if (pubform->puballtables)
+ return false;Regards,
Vignesh
On Wed, Apr 8, 2026 at 1:45 PM vignesh C <vignesh21@gmail.com> wrote:
On Tue, 7 Apr 2026 at 12:32, Peter Smith <smithpb2250@gmail.com> wrote:
Hi, after confirming my understanding of pg_publication_rel [1], I
revisited some logical replication internal functions.Specifically.
* The `is_table_publication` function is for checking if the
publication has a clause like "FOR TABLE t1".
* The `is_schema_publication` function is for checking if the
publication has a clause like "FOR TABLES IN SCHEMA s1".Notice that neither of these ("FOR TABLE", "FOR TABLES IN SCHEMA")
clauses are possible simultaneously with "FOR ALL TABLES".And we can readily discover if "FOR ALL TABLES" (aka `puballtables`)
is present from the pubform.We can use this to optimise and simplify the implementations of the
`is_schema_publication` and `is_table_publication` functions.PSA patch v1.
AFAICT, the result is:
- less code + simpler logic. e.g. is_table_publication does not check
'prexcept' anymore
- more efficient. e.g. skips unnecessary scanning when puballtables is true.
- more consistent. e.g., both functions are now almost identical.Thoughts?
Hi Vignesh. Thanks for reviewing!
I'm not sure if this additional check is sufficient in case of
is_schema_publication. Checking only puballtables can exclude FOR ALL
TABLES, but it still cannot distinguish regular table publications,
empty publications, or sequence publications. In all of those cases,
we still need to check pg_publication_namespace.
Yes, this condition is only an optimisation for FOR ALL TABLES, as the
comment says.
IMO, the overhead of 1 additional boolean check for cases where it
doesn't help is an insignificant trade-off for the savings when it can
return false.
And also why just check for puballtables why not to check for puballsequences
I think function is_schema_publication() is unrelated to 'puballsequences'.
e.g. all the following will still need to check
pg_publication_namespace, regardless of the 'puballsequences' value.
ex1. CREATE PUBLICATION ... FOR ALL SEQUENCES;
ex2. CREATE PUBLICATION ... FOR ALL SEQUENCES, FOR TABLES IN SCHEMA s1;
ex3. CREATE PUBLICATION ... FOR TABLES IN SCHEMA s1;
======
Kind Regards,
Peter Smith.
Fujitsu Austalia
On Wed, Apr 8, 2026 at 10:24 AM Peter Smith <smithpb2250@gmail.com> wrote:
On Wed, Apr 8, 2026 at 1:45 PM vignesh C <vignesh21@gmail.com> wrote:
On Tue, 7 Apr 2026 at 12:32, Peter Smith <smithpb2250@gmail.com> wrote:
Hi, after confirming my understanding of pg_publication_rel [1], I
revisited some logical replication internal functions.Specifically.
* The `is_table_publication` function is for checking if the
publication has a clause like "FOR TABLE t1".
* The `is_schema_publication` function is for checking if the
publication has a clause like "FOR TABLES IN SCHEMA s1".Notice that neither of these ("FOR TABLE", "FOR TABLES IN SCHEMA")
clauses are possible simultaneously with "FOR ALL TABLES".And we can readily discover if "FOR ALL TABLES" (aka `puballtables`)
is present from the pubform.We can use this to optimise and simplify the implementations of the
`is_schema_publication` and `is_table_publication` functions.PSA patch v1.
AFAICT, the result is:
- less code + simpler logic. e.g. is_table_publication does not check
'prexcept' anymore
- more efficient. e.g. skips unnecessary scanning when puballtables is true.
- more consistent. e.g., both functions are now almost identical.Thoughts?
Hi Vignesh. Thanks for reviewing!
I'm not sure if this additional check is sufficient in case of
is_schema_publication. Checking only puballtables can exclude FOR ALL
TABLES, but it still cannot distinguish regular table publications,
empty publications, or sequence publications. In all of those cases,
we still need to check pg_publication_namespace.Yes, this condition is only an optimisation for FOR ALL TABLES, as the
comment says.IMO, the overhead of 1 additional boolean check for cases where it
doesn't help is an insignificant trade-off for the savings when it can
return false.And also why just check for puballtables why not to check for puballsequences
I think function is_schema_publication() is unrelated to 'puballsequences'.
e.g. all the following will still need to check
pg_publication_namespace, regardless of the 'puballsequences' value.ex1. CREATE PUBLICATION ... FOR ALL SEQUENCES;
ex2. CREATE PUBLICATION ... FOR ALL SEQUENCES, FOR TABLES IN SCHEMA s1;
ex3. CREATE PUBLICATION ... FOR TABLES IN SCHEMA s1;
IIUC, we don't support mix of ALL SEQUENCES and TABLES IN SCHEMA s1.
So I could not understand your point, why FOR ALL SEQ still need to
check pg_publication_namespace?
thanks
Shveta
On Wed, Apr 8, 2026 at 3:25 PM shveta malik <shveta.malik@gmail.com> wrote:
On Wed, Apr 8, 2026 at 10:24 AM Peter Smith <smithpb2250@gmail.com> wrote:
On Wed, Apr 8, 2026 at 1:45 PM vignesh C <vignesh21@gmail.com> wrote:
On Tue, 7 Apr 2026 at 12:32, Peter Smith <smithpb2250@gmail.com> wrote:
Hi, after confirming my understanding of pg_publication_rel [1], I
revisited some logical replication internal functions.Specifically.
* The `is_table_publication` function is for checking if the
publication has a clause like "FOR TABLE t1".
* The `is_schema_publication` function is for checking if the
publication has a clause like "FOR TABLES IN SCHEMA s1".Notice that neither of these ("FOR TABLE", "FOR TABLES IN SCHEMA")
clauses are possible simultaneously with "FOR ALL TABLES".And we can readily discover if "FOR ALL TABLES" (aka `puballtables`)
is present from the pubform.We can use this to optimise and simplify the implementations of the
`is_schema_publication` and `is_table_publication` functions.PSA patch v1.
AFAICT, the result is:
- less code + simpler logic. e.g. is_table_publication does not check
'prexcept' anymore
- more efficient. e.g. skips unnecessary scanning when puballtables is true.
- more consistent. e.g., both functions are now almost identical.Thoughts?
Hi Vignesh. Thanks for reviewing!
I'm not sure if this additional check is sufficient in case of
is_schema_publication. Checking only puballtables can exclude FOR ALL
TABLES, but it still cannot distinguish regular table publications,
empty publications, or sequence publications. In all of those cases,
we still need to check pg_publication_namespace.Yes, this condition is only an optimisation for FOR ALL TABLES, as the
comment says.IMO, the overhead of 1 additional boolean check for cases where it
doesn't help is an insignificant trade-off for the savings when it can
return false.And also why just check for puballtables why not to check for puballsequences
I think function is_schema_publication() is unrelated to 'puballsequences'.
e.g. all the following will still need to check
pg_publication_namespace, regardless of the 'puballsequences' value.ex1. CREATE PUBLICATION ... FOR ALL SEQUENCES;
ex2. CREATE PUBLICATION ... FOR ALL SEQUENCES, FOR TABLES IN SCHEMA s1;
ex3. CREATE PUBLICATION ... FOR TABLES IN SCHEMA s1;IIUC, we don't support mix of ALL SEQUENCES and TABLES IN SCHEMA s1.
So I could not understand your point, why FOR ALL SEQ still need to
check pg_publication_namespace?
Oh! You are right.
(Sorry, Vignesh, I did not recognise that combination as unsupported).
I'll post a patch update to handle it.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, Apr 8, 2026 at 4:04 PM Peter Smith <smithpb2250@gmail.com> wrote:
On Wed, Apr 8, 2026 at 3:25 PM shveta malik <shveta.malik@gmail.com> wrote:
On Wed, Apr 8, 2026 at 10:24 AM Peter Smith <smithpb2250@gmail.com> wrote:
On Wed, Apr 8, 2026 at 1:45 PM vignesh C <vignesh21@gmail.com> wrote:
...
And also why just check for puballtables why not to check for puballsequences
I think function is_schema_publication() is unrelated to 'puballsequences'.
e.g. all the following will still need to check
pg_publication_namespace, regardless of the 'puballsequences' value.ex1. CREATE PUBLICATION ... FOR ALL SEQUENCES;
ex2. CREATE PUBLICATION ... FOR ALL SEQUENCES, FOR TABLES IN SCHEMA s1;
ex3. CREATE PUBLICATION ... FOR TABLES IN SCHEMA s1;IIUC, we don't support mix of ALL SEQUENCES and TABLES IN SCHEMA s1.
So I could not understand your point, why FOR ALL SEQ still need to
check pg_publication_namespace?Oh! You are right.
(Sorry, Vignesh, I did not recognise that combination as unsupported).
I'll post a patch update to handle it.
PSA patch v2.
Same as before, but now also doing a quick return false from both
functions if `puballsequences` is true.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, Apr 8, 2026 at 11:58 AM Peter Smith <smithpb2250@gmail.com> wrote:
On Wed, Apr 8, 2026 at 4:04 PM Peter Smith <smithpb2250@gmail.com> wrote:
On Wed, Apr 8, 2026 at 3:25 PM shveta malik <shveta.malik@gmail.com> wrote:
On Wed, Apr 8, 2026 at 10:24 AM Peter Smith <smithpb2250@gmail.com> wrote:
On Wed, Apr 8, 2026 at 1:45 PM vignesh C <vignesh21@gmail.com> wrote:
...
And also why just check for puballtables why not to check for puballsequences
I think function is_schema_publication() is unrelated to 'puballsequences'.
e.g. all the following will still need to check
pg_publication_namespace, regardless of the 'puballsequences' value.ex1. CREATE PUBLICATION ... FOR ALL SEQUENCES;
ex2. CREATE PUBLICATION ... FOR ALL SEQUENCES, FOR TABLES IN SCHEMA s1;
ex3. CREATE PUBLICATION ... FOR TABLES IN SCHEMA s1;IIUC, we don't support mix of ALL SEQUENCES and TABLES IN SCHEMA s1.
So I could not understand your point, why FOR ALL SEQ still need to
check pg_publication_namespace?Oh! You are right.
(Sorry, Vignesh, I did not recognise that combination as unsupported).
I'll post a patch update to handle it.
PSA patch v2.
Same as before, but now also doing a quick return false from both
functions if `puballsequences` is true.
Okay. I was trying to determine where this optimization would be beneficial.
In cases, where we attempt to add tables or schemas to an ALL TABLES
or ALL SEQUENCES publication, the operation will error out in
CheckAlterPublication() before is_table_publication() or
is_schema_publication() are even called. And in cases where we are
trying to add table or schema to a non ALL-TABLEs/SEQ pub, and we end
up invoking these functions, we still need to traverse pg_pub_rel.
The only scenario (as I understand it) that benefits from this change
is when we try to add EXCEPT to an ALL TABLES publication. In that
case, both of the concerned functions would not need to access
pg_pub_rel if the publication is already an ALL TABLES publication. So
this optimization helps in a positive (non-erroneous) case.
In cases where we need to throw an error (for example, adding EXCEPT
to a FOR TABLE publication), these checks would not provide any
benefit as we still need to traverse pg_pub_rel to see if it has any
valid tables or it is an emty publication (empty one is fine).
But since the optimization improves a valid, non-erroneous scenario,
IMO, it is good to include it. Let's see what others have to say on
this.
thanks
Shveta
This patch had received positive feedback in the last post, but the
thread has been inactive for about 5 weeks. What can I do to help get
this pushed?
======
Kind Regards
Peter Smith.
Fujitsu Australia
On Mon, May 18, 2026 at 2:49 PM Peter Smith <smithpb2250@gmail.com> wrote:
This patch had received positive feedback in the last post, but the
thread has been inactive for about 5 weeks. What can I do to help get
this pushed?
I've added a CF entry.
=======
[1]: https://commitfest.postgresql.org/patch/7042/
Kind Regards,
Peter Smith
Fujitsu Australia
On Wed, Jul 22, 2026 at 11:35 AM Peter Smith <smithpb2250@gmail.com> wrote:
I've added a CF entry.
[1] https://commitfest.postgresql.org/patch/7042/
Hi,
I've reviewed v2 of this patch. Summary: the simplification is correct and
I like it; one behavioral observation from corruption testing and a few
comment nits below.
Tested on master (36f7330b8b2): applies cleanly (two hunks in
pg_publication.c land with a 17-line offset, no fuzz), assert-enabled build
with zero warnings, core regression 245/245, subscription TAP 589/589.
Correctness of dropping the prexcept check: I verified the invariant this
relies on — pg_publication_rel holds EXCEPT rows iff puballtables is set:
- the grammar is strictly disjunctive (FOR ALL TABLES/SEQUENCES take no
object list together with FOR TABLE / FOR TABLES IN SCHEMA),
- CheckAlterPublication() rejects adding tables/schemas whenever either
flag is set,
- the only transition that clears puballtables (SET ALL SEQUENCES on an
all-tables publication) reconciles the EXCEPT rows in the same command
that flips the flag,
- GetIncludedPublicationRelations()/GetExcludedPublicationTables() already
Assert exactly this invariant.
So returning HeapTupleIsValid(tup) without inspecting prexcept is sound.
It may also be worth stating in the commit message that the early returns
are reachable from exactly one call site: CheckAlterPublication()
(publicationcmds.c:1588). In AlterPublicationTables() the flagged case
errors out earlier, and AlterPublicationOwner_internal() already guards on
form->puballtables || form->puballsequences before calling.
One behavioral difference I verified end-to-end (assert builds of master
with and without the patch, corrupting pg_publication_rel directly): if an
inclusion row ever exists on a FOR ALL TABLES publication — a state the
invariant rules out, reachable only through catalog corruption or the
pre-existing check-then-lock window in AlterPublication()
(CheckAlterPublication() runs before LockDatabaseObject()) — the old
code's first-tuple probe can be poisoned by it. When the stray row is the
scan's first tuple (equal prpubid keys come out in heap order), legitimate
DDL like ALTER PUBLICATION ... SET ALL TABLES EXCEPT / SET ALL SEQUENCES
fails with "does not support ALL TABLES/SEQUENCES operations" — a
misleading error, since the publication already IS all-tables — while the
stray row stays put. The new code short-circuits on the flag, so the DDL
proceeds; the stray row remains in both versions, i.e. neither heals the
state. So I don't see this as a point against the patch (arguably the new
behavior is the more robust one), but it does make the invariant truly
load-bearing: I'd suggest keeping a condensed version of the deleted
comment in is_table_publication(), e.g. "A FOR ALL TABLES publication can
have only EXCEPT entries in pg_publication_rel, so it never counts as a
table publication."
Framing nit for the commit message: the puballsequences clause added in v2
is a consistency/optimization change, not a correctness fix — with
puballsequences set, the scans were guaranteed to find nothing, so the old
code already returned false. Saying so plainly would be more accurate than
implying v1 had a bug here.
Comment nits:
- the new block comments have a double space after '*'; project style is a
single space (pgindent won't reflow it);
- the doc comment "(e.g., FOR TABLE)" should be "i.e." — FOR TABLE is
exactly the case, not an example; alternatively keep the old contract
sentence that EXCEPT-only publications return false;
- the two stacked coexistence sentences could be merged, and the
puballtables half could mention the EXCEPT-row rationale.
No new tests needed IMO: publication.sql and 037_except.pl already
exercise both the early-return and the scan-then-error paths. I also don't
think a benchmark is warranted — the saving is ~2 catalog index scans on a
DDL path; the honest justification is simplification, so the commit
message maybe shouldn't lead with "more efficient".
Process note: this patch currently has two CommitFest entries, #6705 and
#7042, both "Needs review" in PG20-2. Could you close one of them so
review activity lands in a single place?
With the invariant comment retained, this gets my +1.
Regards,
Bingshuai Li
On Tue, Aug 18, 2026 at 5:48 PM Bingshuai Li <lucian1412@outlook.com> wrote:
Thanks for your review.
PSA patch v3 which has modified the comments as suggested.
Process note: this patch currently has two CommitFest entries, #6705 and
#7042, both "Needs review" in PG20-2. Could you close one of them so
review activity lands in a single place?
Thanks for reporting the CF duplicate. I've "withdrawn" one of them.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
t139232_11v3-0001-rewrite-is_table_publication.patchapplication/octet-stream; name=v3-0001-rewrite-is_table_publication.patchDownload+29-30
Hi Peter,
Thanks for the quick v3 — the doc comments and the merged coexistence
sentences all look good.
One point from my review seems to have been dropped, though. The merged
comment in is_table_publication() now reads:
/*
* FOR TABLE cannot be used with FOR ALL TABLES or FOR ALL SEQUENCES.
*/
That states the grammar fact, but not why skipping the
pg_publication_rel scan is safe: a FOR ALL TABLES publication can still
have EXCEPT rows there, and returning false is correct precisely because
those are the only rows it can have. Would you consider keeping one more
sentence to that effect, e.g.
/*
* FOR TABLE cannot be used with FOR ALL TABLES or FOR ALL SEQUENCES.
* A FOR ALL TABLES publication can have only EXCEPT entries in
* pg_publication_rel, so it never counts as a table publication.
*/
The is_schema_publication() side is fine as written, since
pg_publication_namespace simply has no rows to find in that case.
BTW, v2 -> v3 is comment-only apart from the rebase, so the test results
I posted earlier still stand. With the comment above in place, this gets
my +1.
Regards,
Bingshuai Li
On Thu, Aug 20, 2026 at 11:40 AM Bingshuai Li <lucian1412@outlook.com> wrote:
Hi Peter,
Thanks for the quick v3 — the doc comments and the merged coexistence
sentences all look good.One point from my review seems to have been dropped, though. The merged
comment in is_table_publication() now reads:/*
* FOR TABLE cannot be used with FOR ALL TABLES or FOR ALL SEQUENCES.
*/That states the grammar fact, but not why skipping the
pg_publication_rel scan is safe: a FOR ALL TABLES publication can still
have EXCEPT rows there, and returning false is correct precisely because
those are the only rows it can have. Would you consider keeping one more
sentence to that effect, e.g./*
* FOR TABLE cannot be used with FOR ALL TABLES or FOR ALL SEQUENCES.
* A FOR ALL TABLES publication can have only EXCEPT entries in
* pg_publication_rel, so it never counts as a table publication.
*/
The name of the function is "is_table_publication". The purpose is in
the name. Just return T/F if there is "FOR TABLE" clause for the
publication; nothing more.
e.g., if the function was called "is_table_in_pg_publication_rel",
then I would want to explain about EXCEPT.
Since "FOR ALL TABLES" is already mutually exclusive from "FOR TABLE"
I didn't see any need to explain about "FOR ALL TABLES EXCEPT".
Was your review AI-based? It appeared overly concerned about this detail.
Anyway, I am marking this as "ready for committer" so a committer can
decide on this point.
======
Kind Regards,
Peter Smith.
Fujitsu Australia