Re: Skipping schema changes in publication

On Thu, 20 Nov 2025 at 11:54, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shlok.
>
> Thanks for splitting the patches.
>
> Here are some review comments for the new patch v28-0002 (ADD ALL TABLES).
>
> ======
> Commit Message
>
> 1.
> This patch adds support for using ADD ALL TABLES in ALTER PUBLICATION,
> allowing an existing publication to be changed into an ALL TABLES
> publication. This command is permitted only when the publication is
> in its default state, meaning it has no tables or schemas added, its
> ALL TABLES and ALL SEQUENCES flags are not set, and publication
> options such as publish_via_root_partition, publish_generated_columns,
> and publish are at their default values.
>
> ~
>
> IMO, the restrictions for this new command are too severe:
>
> e.g. If I already have a FOR ALL SEQUENCES publication, then I
> expected it should be possible to ADD ALL TABLES to that as well,
> right?
>
> Likewise, why are we enforcing that the publication parameters must be
> defaults? IOW, why is (i) below disallowed, but (ii) is allowed?
>
> (i)
> ALTER PUBLICATION pub SET (publish_generated_columns=stored);
> ALTER PUBLICATION pub ADD ALL TABLES;
>
> (ii)
> ALTER PUBLICATION pub ADD ALL TABLES;
> ALTER PUBLICATION pub SET (publish_generated_columns=stored);
>
I agree that the current restrictions were too strict. With the latest
patch we avoid adding ALL TABLES only when we have an existing list of
tables or schemas in a publication.

> ======
> doc/src/sgml/ref/alter_publication.sgml
>
> Description:
>
> 2.
> The "Description" part of this page is confusing because it was
> referring to "The first three variants" and later "The fourth
> variant". Now that the "ADD ALL TABLES" variant has been added, I
> have lost track of what "variants" this description is talking about.
> Those words should be replaced by something clearer. This could be an
> ongoing issue if it is not worded differently because the same problem
> will happen again, e.g. when more syntax gets added for ALL SEQUENCES,
> etc.
>
> ~~~
>
I have updated the description to avoid the wording "The first three
variants". Instead I have added a list to describe each command
separately. Similar to ALTER TABLE [1].

> 3.
> Note also that DROP TABLES IN SCHEMA will not drop any schema tables
> that were specified using FOR TABLE/ ADD TABLE.
>
> ~
>
> That sentence (above) is from the docs. Does that also need updating
> now that there is ADD ALL TABLES?
>
When we create a publication on a schema, we can also add specific
tables using FOR TABLE/ADD TABLE.
But in case of ALL TABLES publication we are not allowed to include
tables using FOR TABLE/ADD TABLE.

So for ALL TABLES case this wording is not required.

> ======
> src/backend/commands/publicationcmds.c
>
> CheckPublicationDefValues:
>
> 4.
> Is this function needed?
>
It is not needed. Modified the function to give proper error messages
for each case.

> ~~~
>
> AlterPublication:
>
> 5.
> + if (stmt->for_all_tables)
> + {
> + bool isdefault = CheckPublicationDefValues(tup);
> +
> + if (!isdefault)
> + ereport(ERROR,
> + errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> + errmsg("adding ALL TABLES requires the publication to have default
> publication parameter values"),
> + errdetail("ALL TABLES or ALL SEQUENCES flag should not be set and no
> tables/schemas should be associated."),
> + errhint("Use ALTER PUBLICATION ... RESET to reset the publication"));
> +
> + AlterPublicationSetAllTables(rel, tup);
> + }
> +
>
> Why do we need this self-imposed restriction?
>
See reply to comment 1.

> ======
> src/include/nodes/parsenodes.h
>
> 6.
> 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;
>
>
> There is no such "FOR" syntax like ALTER PUBLICATION ... FOR ALL
> TABLES, so I felt just 'puballtables' might be a better member name.
>
We have the same variable name in CreatePublicationStmt. I feel
keeping the name as 'for_all_tables' will keep it consistent and
easier to understand.

> ======
> src/test/regress/sql/publication.sql
>
> 7.
> Don't uppercase any of the publication parameters because they never
> appear in the docs/examples like that.
>
> ~
>
> 8.
> So that the last command is the one being tested, I felt that all the
> test cases should be doing RESET *first* instead of last.
>
> ~~~
>
> 9.
> You don't always need to use RESET. There should also be some tests
> using an "empty" publication just to be sure it works. e.g
>
> CREATE PUBLICATION pub_empty;
> ALTER PUBLICATION pub_empty ADD ALL TABLES;
>
> ~~~
>
> 10.
> As commented earlier, I felt the rules were too restrictive. So I
> think some test cases can be removed.
>
> ~~~
>
> 11.
> +-- Tests for ALTER PUBLICATION ... ADD ALL TABLES
>
> ~
>
> I noticed there is a "--
> ======================================================" separator
> between the major groups of tests.
>
> 11a. Should use this separator in patch 0001 for the RESET group of tests
>
> 11b. Should use this separator in patch 0002 for the ADD ALL TABLES
> groups of tests
>
> ~~~
>
> 12.
> +-- Can't add ALL TABLES to 'ALL TABLES' publication
> +ALTER PUBLICATION testpub_reset ADD ALL TABLES;
> +
>
> This test case seems to belong earlier, near the 'FOR TABLE' and the
> 'TABLES IN SCHEMA' tests.
>
I saw the patch needed a rebase. I have rebased it.
I have also addressed the remaining comments in this email and
comments in the email [2].

While addressing the comments I saw there were a couple of race
conditions when we run 'ALTER PUBLICATION ... RESET and ALTER
PUBLICATION ... ADD TABLE concurrently' and
'ALTER PUBLICATION ... ADD ALL TABLES and ALTER PUBLICATION ... ADD
TABLE concurrently'
I have addressed these in the v29 patch.
Will address comments for 0003 and 0004 patch by Peter and comments by
Shveta in next version.

[1]: https://www.postgresql.org/docs/current/sql-altertable.html
[2]: https://www.postgresql.org/message-id/CAHut%2BPv4d9EAjDQiOHiu2BrYP3ZA-oJgsgGZdygBaZnWDR7sDA%40mail.gmail.com

Thanks,
Shlok Kyal