Remove unused for_all_tables field from AlterPublicationStmt
Hi,
I found that the for_all_table field in the AlterPublicationStmt is
not used at all unless I'm missing something. I've attached the patch
for only master that removes it.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Attachments:
0001-Remove-unused-for_all_tables-field-from-AlterPublica.patchapplication/octet-stream; name=0001-Remove-unused-for_all_tables-field-from-AlterPublica.patchDownload
From 5560437e1fe3ce0e0bc1f4c42ddedb4e4c4d9f4d Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Thu, 25 Sep 2025 12:33:01 -0700
Subject: [PATCH] Remove unused for_all_tables field from AlterPublicationStmt.
No backpatch as AlterPublicationStmt struct is exposed.
Reviewed-by:
Discussion: https://postgr.es/m/
---
src/backend/commands/publicationcmds.c | 4 ----
src/include/nodes/parsenodes.h | 1 -
2 files changed, 5 deletions(-)
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 3de5687461c..f4fc17acbe1 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -1855,8 +1855,6 @@ PublicationAddTables(Oid pubid, List *rels, bool if_not_exists,
{
ListCell *lc;
- Assert(!stmt || !stmt->for_all_tables);
-
foreach(lc, rels)
{
PublicationRelInfo *pub_rel = (PublicationRelInfo *) lfirst(lc);
@@ -1934,8 +1932,6 @@ PublicationAddSchemas(Oid pubid, List *schemas, bool if_not_exists,
{
ListCell *lc;
- Assert(!stmt || !stmt->for_all_tables);
-
foreach(lc, schemas)
{
Oid schemaid = lfirst_oid(lc);
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 4ed14fc5b78..f1706df58fd 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -4320,7 +4320,6 @@ typedef struct AlterPublicationStmt
* objects.
*/
List *pubobjects; /* Optional list of publication objects */
- bool for_all_tables; /* Special publication for all tables in db */
AlterPublicationAction action; /* What action to perform with the given
* objects */
} AlterPublicationStmt;
--
2.47.3
On Sep 26, 2025, at 04:47, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
Hi,
I found that the for_all_table field in the AlterPublicationStmt is
not used at all unless I'm missing something. I've attached the patch
for only master that removes it.
Yep, based on code of gram.y and the doc, FOR ALL TABLE is only supposed by CREATE PUBLICATION.
I agree to remove the field from AlterPublicationStmt, but I think we should retain "Assert(!stmt)”. Because Assert() is a way to detect programming bug. During development and debug builds, it prints a diagnostic message which is helpful for identifying bugs. Without the Assert(!stmt), it will just silently discard the bug by “if (stmt)” in case that stmt happens to be NULL.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On Fri, Sep 26, 2025 at 09:24:03AM +0800, Chao Li wrote:
On Sep 26, 2025, at 04:47, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
I found that the for_all_table field in the AlterPublicationStmt is
not used at all unless I'm missing something. I've attached the patch
for only master that removes it.Yep, based on code of gram.y and the doc, FOR ALL TABLE is only
supposed by CREATE PUBLICATION.
Indeed, good catch. There's no point for this field currently. As
far as I can see it is wrong since 665d1fad99e7, so I am betting on a
rebase mistake when the original feature has been reviewed.
--
Michael
On 2025-Sep-26, Chao Li wrote:
I agree to remove the field from AlterPublicationStmt, but I think we
should retain "Assert(!stmt)”. Because Assert() is a way to detect
programming bug. During development and debug builds, it prints a
diagnostic message which is helpful for identifying bugs. Without the
Assert(!stmt), it will just silently discard the bug by “if (stmt)” in
case that stmt happens to be NULL.
CreatePublication() calls this with an empty stmt, so if you keep the
assertion, the program would crash (unless that callsite is dead code,
in which case it should probably be modified as well). In any case,
such a simple assertion is not very useful: the program would crash
anyway as soon as we tried to dereference stmt, which is exactly the
same the assertion would do.
I'm going to bet that Masahiko has the code right.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Thou shalt check the array bounds of all strings (indeed, all arrays), for
surely where thou typest "foo" someone someday shall type
"supercalifragilisticexpialidocious" (5th Commandment for C programmers)
On Fri, Sep 26, 2025 at 8:02 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
On 2025-Sep-26, Chao Li wrote:
I agree to remove the field from AlterPublicationStmt, but I think we
should retain "Assert(!stmt)”. Because Assert() is a way to detect
programming bug. During development and debug builds, it prints a
diagnostic message which is helpful for identifying bugs. Without the
Assert(!stmt), it will just silently discard the bug by “if (stmt)” in
case that stmt happens to be NULL.CreatePublication() calls this with an empty stmt, so if you keep the
assertion, the program would crash (unless that callsite is dead code,
in which case it should probably be modified as well). In any case,
such a simple assertion is not very useful: the program would crash
anyway as soon as we tried to dereference stmt, which is exactly the
same the assertion would do.I'm going to bet that Masahiko has the code right.
Thank you everyone for reviewing the patch.
I agree with Álvaro's point. I've pushed the proposed patch.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
On Sat, Sep 27, 2025 at 2:28 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Fri, Sep 26, 2025 at 8:02 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
On 2025-Sep-26, Chao Li wrote:
I agree to remove the field from AlterPublicationStmt, but I think we
should retain "Assert(!stmt)”. Because Assert() is a way to detect
programming bug. During development and debug builds, it prints a
diagnostic message which is helpful for identifying bugs. Without the
Assert(!stmt), it will just silently discard the bug by “if (stmt)” in
case that stmt happens to be NULL.CreatePublication() calls this with an empty stmt, so if you keep the
assertion, the program would crash (unless that callsite is dead code,
in which case it should probably be modified as well). In any case,
such a simple assertion is not very useful: the program would crash
anyway as soon as we tried to dereference stmt, which is exactly the
same the assertion would do.I'm going to bet that Masahiko has the code right.
Thank you everyone for reviewing the patch.
I agree with Álvaro's point. I've pushed the proposed patch.
Hi, Sorry, I stumbled upon this thread late...
I feel that using ALTER PUBLICATION commands, a user should be able to
construct any publication equivalent to one that they could have made
using CREATE PUBLICATION in the first place.
Now, it is perfectly fine to make an "empty" publication (e.g. CREATE
PUBLICATION pubname;). But currently, there is no way to alter that
empty publication to become a FOR ALL TABLES publication.
Therefore, it seems to me that the ALTER PUBLICATION syntax is
incomplete. e.g. There also needs to be the ability to do ALTER
PUBLICATION ADD/SET FOR ALL TABLES;
Although the member that was removed might be currently unused, won't
it be needed again whenever those missing ALTER PUBLICATION ADD/SET
FOR ALL TABLES are implemented?
======
Kind Regards,
Peter Smith.
Fujitsu Australia
On 2025-Nov-10, Peter Smith wrote:
Therefore, it seems to me that the ALTER PUBLICATION syntax is
incomplete. e.g. There also needs to be the ability to do ALTER
PUBLICATION ADD/SET FOR ALL TABLES;Although the member that was removed might be currently unused, won't
it be needed again whenever those missing ALTER PUBLICATION ADD/SET
FOR ALL TABLES are implemented?
Sure, but it's easy to add again later when it is needed.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"I'm always right, but sometimes I'm more right than other times."
(Linus Torvalds)
https://lore.kernel.org/git/Pine.LNX.4.58.0504150753440.7211@ppc970.osdl.org/