Обсуждение: 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

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