Horton

Thread

Commits

  1. Remove unused for_all_tables field from AlterPublicationStmt.

  1. Remove unused for_all_tables field from AlterPublicationStmt

    Masahiko Sawada <sawada.mshk@gmail.com> — 2025-09-25T20:47:36Z

    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

  2. Re: Remove unused for_all_tables field from AlterPublicationStmt

    Chao Li <li.evan.chao@gmail.com> — 2025-09-26T01:24:03Z

    
> 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/

  3. Re: Remove unused for_all_tables field from AlterPublicationStmt

    Michael Paquier <michael@paquier.xyz> — 2025-09-26T02:49:43Z

    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

  4. Re: Remove unused for_all_tables field from AlterPublicationStmt

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-09-26T15:02:05Z

    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)

  5. Re: Remove unused for_all_tables field from AlterPublicationStmt

    Masahiko Sawada <sawada.mshk@gmail.com> — 2025-09-26T16:27:28Z

    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

  6. Re: Remove unused for_all_tables field from AlterPublicationStmt

    Peter Smith <smithpb2250@gmail.com> — 2025-11-10T05:20:32Z

    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

  7. Re: Remove unused for_all_tables field from AlterPublicationStmt

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-11-10T11:30:18Z

    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/