Re: Skipping schema changes in publication

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);

======
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.

~~~

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?

======
src/backend/commands/publicationcmds.c

CheckPublicationDefValues:

4.
Is this function needed?

~~~

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?

======
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.

======
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.

======
Kind Regards,
Peter Smith.
Fujitsu Australia