public inbox for [email protected]
help / color / mirror / Atom feedLogical Replication - revisit `is_table_publication` function implementation
7+ messages / 3 participants
[nested] [flat]
* Logical Replication - revisit `is_table_publication` function implementation
@ 2026-04-07 07:02 Peter Smith <[email protected]>
2026-04-08 03:45 ` Re: Logical Replication - revisit `is_table_publication` function implementation vignesh C <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Peter Smith @ 2026-04-07 07:02 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>
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?
======
[1] https://www.postgresql.org/message-id/flat/CAHut%2BPv1UKR_bxmN7wcCCpQveHoYprvH-hbdFq8gsaH1Ye7B_w%40m...
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
[application/octet-stream] v1-0001-rewrite-is_table_publication.patch (5.1K, 2-v1-0001-rewrite-is_table_publication.patch)
download | inline diff:
From 4fb7f2180f5674e527635118a967b0e2fd228e5b Mon Sep 17 00:00:00 2001
From: Peter Smith <[email protected]>
Date: Tue, 7 Apr 2026 16:31:21 +1000
Subject: [PATCH v1] rewrite is_table_publication
---
src/backend/catalog/pg_publication.c | 38 ++++++++++++--------------
src/backend/commands/publicationcmds.c | 7 ++---
src/include/catalog/pg_publication.h | 4 +--
3 files changed, 22 insertions(+), 27 deletions(-)
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index a43d385c605..c60885db10a 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -275,19 +275,23 @@ filter_partitions(List *table_infos)
* schema is associated with the publication.
*/
bool
-is_schema_publication(Oid pubid)
+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;
pubschsrel = table_open(PublicationNamespaceRelationId, AccessShareLock);
ScanKeyInit(&scankey,
Anum_pg_publication_namespace_pnpubid,
BTEqualStrategyNumber, F_OIDEQ,
- ObjectIdGetDatum(pubid));
+ ObjectIdGetDatum(pubform->oid));
scan = systable_beginscan(pubschsrel,
PublicationNamespacePnnspidPnpubidIndexId,
@@ -302,41 +306,33 @@ is_schema_publication(Oid pubid)
}
/*
- * Returns true if the publication has explicitly included relation (i.e.,
- * not marked as EXCEPT).
+ * Returns true if the publication has explicitly included relations (e.g.,
+ * FOR TABLE).
*/
bool
-is_table_publication(Oid pubid)
+is_table_publication(Form_pg_publication pubform)
{
Relation pubrelsrel;
ScanKeyData scankey;
SysScanDesc scan;
HeapTuple tup;
- bool result = false;
+ bool result;
+
+ /* FOR TABLE cannot coexist with FOR ALL TABLES. */
+ if (pubform->puballtables)
+ return false;
pubrelsrel = table_open(PublicationRelRelationId, AccessShareLock);
ScanKeyInit(&scankey,
Anum_pg_publication_rel_prpubid,
BTEqualStrategyNumber, F_OIDEQ,
- ObjectIdGetDatum(pubid));
+ ObjectIdGetDatum(pubform->oid));
scan = systable_beginscan(pubrelsrel,
PublicationRelPrpubidIndexId,
true, NULL, 1, &scankey);
tup = systable_getnext(scan);
- if (HeapTupleIsValid(tup))
- {
- Form_pg_publication_rel pubrel;
-
- pubrel = (Form_pg_publication_rel) GETSTRUCT(tup);
-
- /*
- * For any publication, pg_publication_rel contains either only EXCEPT
- * entries or only explicitly included tables. Therefore, examining
- * the first tuple is sufficient to determine table inclusion.
- */
- result = !pubrel->prexcept;
- }
+ result = HeapTupleIsValid(tup);
systable_endscan(scan);
table_close(pubrelsrel, AccessShareLock);
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 440adb356ad..3685c711c49 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -1261,7 +1261,7 @@ AlterPublicationTables(AlterPublicationStmt *stmt, HeapTuple tup,
{
TransformPubWhereClauses(rels, queryString, pubform->pubviaroot);
- publish_schema |= is_schema_publication(pubid);
+ publish_schema |= is_schema_publication(pubform);
CheckPubRelationColumnList(stmt->pubname, rels, publish_schema,
pubform->pubviaroot);
@@ -1585,8 +1585,7 @@ CheckAlterPublication(AlterPublicationStmt *stmt, HeapTuple tup,
* If the publication already contains specific tables or schemas, we
* prevent switching to a ALL state.
*/
- if (is_table_publication(pubform->oid) ||
- is_schema_publication(pubform->oid))
+ if (is_table_publication(pubform) || is_schema_publication(pubform))
{
ereport(ERROR,
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
@@ -2202,7 +2201,7 @@ AlterPublicationOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
if (!superuser_arg(newOwnerId))
{
if (form->puballtables || form->puballsequences ||
- is_schema_publication(form->oid))
+ is_schema_publication(form))
ereport(ERROR,
errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied to change owner of publication \"%s\"",
diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h
index 89b4bb14f62..448373363fc 100644
--- a/src/include/catalog/pg_publication.h
+++ b/src/include/catalog/pg_publication.h
@@ -194,8 +194,8 @@ extern Oid GetTopMostAncestorInPublication(Oid puboid, List *ancestors,
int *ancestor_level);
extern bool is_publishable_relation(Relation rel);
-extern bool is_schema_publication(Oid pubid);
-extern bool is_table_publication(Oid pubid);
+extern bool is_schema_publication(Form_pg_publication pubform);
+extern bool is_table_publication(Form_pg_publication pubform);
extern bool check_and_fetch_column_list(Publication *pub, Oid relid,
MemoryContext mcxt, Bitmapset **cols);
extern ObjectAddress publication_add_relation(Oid pubid, PublicationRelInfo *pri,
--
2.47.3
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Logical Replication - revisit `is_table_publication` function implementation
2026-04-07 07:02 Logical Replication - revisit `is_table_publication` function implementation Peter Smith <[email protected]>
@ 2026-04-08 03:45 ` vignesh C <[email protected]>
2026-04-08 05:24 ` Re: Logical Replication - revisit `is_table_publication` function implementation shveta malik <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: vignesh C @ 2026-04-08 03:45 UTC (permalink / raw)
To: Peter Smith <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Tue, 7 Apr 2026 at 12:32, Peter Smith <[email protected]> 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
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Logical Replication - revisit `is_table_publication` function implementation
2026-04-07 07:02 Logical Replication - revisit `is_table_publication` function implementation Peter Smith <[email protected]>
2026-04-08 03:45 ` Re: Logical Replication - revisit `is_table_publication` function implementation vignesh C <[email protected]>
@ 2026-04-08 05:24 ` shveta malik <[email protected]>
2026-04-08 06:04 ` Re: Logical Replication - revisit `is_table_publication` function implementation Peter Smith <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: shveta malik @ 2026-04-08 05:24 UTC (permalink / raw)
To: Peter Smith <[email protected]>; +Cc: vignesh C <[email protected]>; PostgreSQL Hackers <[email protected]>; shveta malik <[email protected]>
On Wed, Apr 8, 2026 at 10:24 AM Peter Smith <[email protected]> wrote:
>
> On Wed, Apr 8, 2026 at 1:45 PM vignesh C <[email protected]> wrote:
> >
> > On Tue, 7 Apr 2026 at 12:32, Peter Smith <[email protected]> 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
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Logical Replication - revisit `is_table_publication` function implementation
2026-04-07 07:02 Logical Replication - revisit `is_table_publication` function implementation Peter Smith <[email protected]>
2026-04-08 03:45 ` Re: Logical Replication - revisit `is_table_publication` function implementation vignesh C <[email protected]>
2026-04-08 05:24 ` Re: Logical Replication - revisit `is_table_publication` function implementation shveta malik <[email protected]>
@ 2026-04-08 06:04 ` Peter Smith <[email protected]>
2026-04-08 06:27 ` Re: Logical Replication - revisit `is_table_publication` function implementation Peter Smith <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Peter Smith @ 2026-04-08 06:04 UTC (permalink / raw)
To: shveta malik <[email protected]>; +Cc: vignesh C <[email protected]>; PostgreSQL Hackers <[email protected]>
On Wed, Apr 8, 2026 at 3:25 PM shveta malik <[email protected]> wrote:
>
> On Wed, Apr 8, 2026 at 10:24 AM Peter Smith <[email protected]> wrote:
> >
> > On Wed, Apr 8, 2026 at 1:45 PM vignesh C <[email protected]> wrote:
> > >
> > > On Tue, 7 Apr 2026 at 12:32, Peter Smith <[email protected]> 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
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Logical Replication - revisit `is_table_publication` function implementation
2026-04-07 07:02 Logical Replication - revisit `is_table_publication` function implementation Peter Smith <[email protected]>
2026-04-08 03:45 ` Re: Logical Replication - revisit `is_table_publication` function implementation vignesh C <[email protected]>
2026-04-08 05:24 ` Re: Logical Replication - revisit `is_table_publication` function implementation shveta malik <[email protected]>
2026-04-08 06:04 ` Re: Logical Replication - revisit `is_table_publication` function implementation Peter Smith <[email protected]>
@ 2026-04-08 06:27 ` Peter Smith <[email protected]>
2026-04-09 04:09 ` Re: Logical Replication - revisit `is_table_publication` function implementation shveta malik <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Peter Smith @ 2026-04-08 06:27 UTC (permalink / raw)
To: shveta malik <[email protected]>; +Cc: vignesh C <[email protected]>; PostgreSQL Hackers <[email protected]>
On Wed, Apr 8, 2026 at 4:04 PM Peter Smith <[email protected]> wrote:
>
> On Wed, Apr 8, 2026 at 3:25 PM shveta malik <[email protected]> wrote:
> >
> > On Wed, Apr 8, 2026 at 10:24 AM Peter Smith <[email protected]> wrote:
> > >
> > > On Wed, Apr 8, 2026 at 1:45 PM vignesh C <[email protected]> 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
Attachments:
[application/octet-stream] v2-0001-rewrite-is_table_publication.patch (5.2K, 2-v2-0001-rewrite-is_table_publication.patch)
download | inline diff:
From f09ebae190716e440a6ce7dd135056a6343cd9d0 Mon Sep 17 00:00:00 2001
From: Peter Smith <[email protected]>
Date: Wed, 8 Apr 2026 16:22:37 +1000
Subject: [PATCH v2] rewrite is_table_publication
---
src/backend/catalog/pg_publication.c | 44 ++++++++++++++------------
src/backend/commands/publicationcmds.c | 7 ++--
src/include/catalog/pg_publication.h | 4 +--
3 files changed, 28 insertions(+), 27 deletions(-)
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index a43d385c605..e2003cd9203 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -275,19 +275,26 @@ filter_partitions(List *table_infos)
* schema is associated with the publication.
*/
bool
-is_schema_publication(Oid pubid)
+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.
+ * FOR TABLES IN SCHEMA cannot coexist with FOR ALL SEQUENCES.
+ */
+ if (pubform->puballtables || pubform->puballsequences)
+ return false;
pubschsrel = table_open(PublicationNamespaceRelationId, AccessShareLock);
ScanKeyInit(&scankey,
Anum_pg_publication_namespace_pnpubid,
BTEqualStrategyNumber, F_OIDEQ,
- ObjectIdGetDatum(pubid));
+ ObjectIdGetDatum(pubform->oid));
scan = systable_beginscan(pubschsrel,
PublicationNamespacePnnspidPnpubidIndexId,
@@ -302,41 +309,36 @@ is_schema_publication(Oid pubid)
}
/*
- * Returns true if the publication has explicitly included relation (i.e.,
- * not marked as EXCEPT).
+ * Returns true if the publication has explicitly included relations (e.g.,
+ * FOR TABLE).
*/
bool
-is_table_publication(Oid pubid)
+is_table_publication(Form_pg_publication pubform)
{
Relation pubrelsrel;
ScanKeyData scankey;
SysScanDesc scan;
HeapTuple tup;
- bool result = false;
+ bool result;
+
+ /*
+ * FOR TABLE cannot coexist with FOR ALL TABLES.
+ * FOR TABLE cannot coexist with FOR ALL SEQUENCES.
+ */
+ if (pubform->puballtables || pubform->puballsequences)
+ return false;
pubrelsrel = table_open(PublicationRelRelationId, AccessShareLock);
ScanKeyInit(&scankey,
Anum_pg_publication_rel_prpubid,
BTEqualStrategyNumber, F_OIDEQ,
- ObjectIdGetDatum(pubid));
+ ObjectIdGetDatum(pubform->oid));
scan = systable_beginscan(pubrelsrel,
PublicationRelPrpubidIndexId,
true, NULL, 1, &scankey);
tup = systable_getnext(scan);
- if (HeapTupleIsValid(tup))
- {
- Form_pg_publication_rel pubrel;
-
- pubrel = (Form_pg_publication_rel) GETSTRUCT(tup);
-
- /*
- * For any publication, pg_publication_rel contains either only EXCEPT
- * entries or only explicitly included tables. Therefore, examining
- * the first tuple is sufficient to determine table inclusion.
- */
- result = !pubrel->prexcept;
- }
+ result = HeapTupleIsValid(tup);
systable_endscan(scan);
table_close(pubrelsrel, AccessShareLock);
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 440adb356ad..3685c711c49 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -1261,7 +1261,7 @@ AlterPublicationTables(AlterPublicationStmt *stmt, HeapTuple tup,
{
TransformPubWhereClauses(rels, queryString, pubform->pubviaroot);
- publish_schema |= is_schema_publication(pubid);
+ publish_schema |= is_schema_publication(pubform);
CheckPubRelationColumnList(stmt->pubname, rels, publish_schema,
pubform->pubviaroot);
@@ -1585,8 +1585,7 @@ CheckAlterPublication(AlterPublicationStmt *stmt, HeapTuple tup,
* If the publication already contains specific tables or schemas, we
* prevent switching to a ALL state.
*/
- if (is_table_publication(pubform->oid) ||
- is_schema_publication(pubform->oid))
+ if (is_table_publication(pubform) || is_schema_publication(pubform))
{
ereport(ERROR,
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
@@ -2202,7 +2201,7 @@ AlterPublicationOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
if (!superuser_arg(newOwnerId))
{
if (form->puballtables || form->puballsequences ||
- is_schema_publication(form->oid))
+ is_schema_publication(form))
ereport(ERROR,
errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied to change owner of publication \"%s\"",
diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h
index 89b4bb14f62..448373363fc 100644
--- a/src/include/catalog/pg_publication.h
+++ b/src/include/catalog/pg_publication.h
@@ -194,8 +194,8 @@ extern Oid GetTopMostAncestorInPublication(Oid puboid, List *ancestors,
int *ancestor_level);
extern bool is_publishable_relation(Relation rel);
-extern bool is_schema_publication(Oid pubid);
-extern bool is_table_publication(Oid pubid);
+extern bool is_schema_publication(Form_pg_publication pubform);
+extern bool is_table_publication(Form_pg_publication pubform);
extern bool check_and_fetch_column_list(Publication *pub, Oid relid,
MemoryContext mcxt, Bitmapset **cols);
extern ObjectAddress publication_add_relation(Oid pubid, PublicationRelInfo *pri,
--
2.47.3
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Logical Replication - revisit `is_table_publication` function implementation
2026-04-07 07:02 Logical Replication - revisit `is_table_publication` function implementation Peter Smith <[email protected]>
2026-04-08 03:45 ` Re: Logical Replication - revisit `is_table_publication` function implementation vignesh C <[email protected]>
2026-04-08 05:24 ` Re: Logical Replication - revisit `is_table_publication` function implementation shveta malik <[email protected]>
2026-04-08 06:04 ` Re: Logical Replication - revisit `is_table_publication` function implementation Peter Smith <[email protected]>
2026-04-08 06:27 ` Re: Logical Replication - revisit `is_table_publication` function implementation Peter Smith <[email protected]>
@ 2026-04-09 04:09 ` shveta malik <[email protected]>
2026-05-18 04:49 ` Re: Logical Replication - revisit `is_table_publication` function implementation Peter Smith <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: shveta malik @ 2026-04-09 04:09 UTC (permalink / raw)
To: Peter Smith <[email protected]>; +Cc: vignesh C <[email protected]>; PostgreSQL Hackers <[email protected]>; shveta malik <[email protected]>
On Wed, Apr 8, 2026 at 11:58 AM Peter Smith <[email protected]> wrote:
>
> On Wed, Apr 8, 2026 at 4:04 PM Peter Smith <[email protected]> wrote:
> >
> > On Wed, Apr 8, 2026 at 3:25 PM shveta malik <[email protected]> wrote:
> > >
> > > On Wed, Apr 8, 2026 at 10:24 AM Peter Smith <[email protected]> wrote:
> > > >
> > > > On Wed, Apr 8, 2026 at 1:45 PM vignesh C <[email protected]> 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
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Logical Replication - revisit `is_table_publication` function implementation
2026-04-07 07:02 Logical Replication - revisit `is_table_publication` function implementation Peter Smith <[email protected]>
2026-04-08 03:45 ` Re: Logical Replication - revisit `is_table_publication` function implementation vignesh C <[email protected]>
2026-04-08 05:24 ` Re: Logical Replication - revisit `is_table_publication` function implementation shveta malik <[email protected]>
2026-04-08 06:04 ` Re: Logical Replication - revisit `is_table_publication` function implementation Peter Smith <[email protected]>
2026-04-08 06:27 ` Re: Logical Replication - revisit `is_table_publication` function implementation Peter Smith <[email protected]>
2026-04-09 04:09 ` Re: Logical Replication - revisit `is_table_publication` function implementation shveta malik <[email protected]>
@ 2026-05-18 04:49 ` Peter Smith <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Peter Smith @ 2026-05-18 04:49 UTC (permalink / raw)
To: shveta malik <[email protected]>; +Cc: vignesh C <[email protected]>; PostgreSQL Hackers <[email protected]>
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
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2026-05-18 04:49 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-04-07 07:02 Logical Replication - revisit `is_table_publication` function implementation Peter Smith <[email protected]>
2026-04-08 03:45 ` vignesh C <[email protected]>
2026-04-08 05:24 ` shveta malik <[email protected]>
2026-04-08 06:04 ` Peter Smith <[email protected]>
2026-04-08 06:27 ` Peter Smith <[email protected]>
2026-04-09 04:09 ` shveta malik <[email protected]>
2026-05-18 04:49 ` Peter Smith <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox