Re: Skipping schema changes in publication - Mailing list pgsql-hackers

Hi Vignesh,

I had a look at patch v24-0001.

FYI -- a rebase is needed

[postgres@CentOS7-x64 oss_postgres_misc]$ git apply
../patches_misc/v24-0001-Add-RESET-clause-to-Alter-Publication-which-will.patch
error: patch failed: doc/src/sgml/ref/alter_publication.sgml:69
error: doc/src/sgml/ref/alter_publication.sgml: patch does not apply

Here are some other review comments:

======

1.
There seems to be some basic omission of the ALTER PUBLICATION in that
it does not support "ALL TABLES" as a publication_object.

Therefore, if you have:
CREATE PUBLICATION mypub FOR ALL TABLES;

and then you do:
ALTER PUBLICATION mypub RESET;

There seems to be no way to restore mpub to be an ALL TABLES publication again!

~~~

I think if you are going to implement a RESET, then you also need a
way to get back to what you had before you did the reset. So you'll
also need to implement the ALTER PUBLICATION mypub SET ALL TABLES;

IMO, supporting  "SET ALL TABLES" should be your new 0001 patch
because AFAIK, this situation already exists if the user had created
an "empty" publication:
CREATE PUBLICATION myemptypub;

======
doc/src/sgml/ref/alter_publication.sgml

2.
Probably need to mention ALL SEQUENCES now too?

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

3.
+/* CREATE PUBLICATION default values for flags and publication parameters */
+#define PUB_DEFAULT_ACTION_INSERT true
+#define PUB_DEFAULT_ACTION_UPDATE true
+#define PUB_DEFAULT_ACTION_DELETE true
+#define PUB_DEFAULT_ACTION_TRUNCATE true
+#define PUB_DEFAULT_VIA_ROOT false
+#define PUB_DEFAULT_ALL_TABLES false
+#define PUB_DEFAULT_GENCOLS PUBLISH_GENCOLS_NONE
+

Is it better to put all these in the catalog/pg_publication.h where
the catalog was defined?

~~~

AlterPublicationReset:

4.
+ /* Set ALL TABLES flag to false */
+ if (pubform->puballtables)
+ {
+ values[Anum_pg_publication_puballtables - 1] =
BoolGetDatum(PUB_DEFAULT_ALL_TABLES);
+ replaces[Anum_pg_publication_puballtables - 1] = true;
+ CacheInvalidateRelcacheAll();
+ }

Why not just do this anyway without the condition?

======
src/backend/parser/gram.y

6.
It would be nicer if all these grammar productions were coded in the
same order as the comment above them.

======
src/include/nodes/parsenodes.h

7.
  AP_AddObjects, /* add objects to publication */
  AP_DropObjects, /* remove objects from publication */
  AP_SetObjects, /* set list of objects */
+ AP_ResetPublication, /* reset the publication */
 } AlterPublicationAction;

It is already called "AlterPublicationAction", so maybe the enum value
only needs to be AP_Reset instead of AP_ResetPublication.


======
src/test/regress/expected/publication.out

8.
Expected output all needs rebasing now that there is a new "All
sequences" column.

======
src/test/regress/sql/publication.sql

9.
+ALTER PUBLICATION testpub_reset ADD TABLE pub_sch1.tbl1;
+
+-- Verify that associated tables are removed from the publication after RESET
+\dRp+ testpub_reset
+ALTER PUBLICATION testpub_reset RESET;
+\dRp+ testpub_reset
+

I felt the ADD TABLE should be after the comment.

And ditto for all the other test cases -- should be that same pattern too.

# comment about test
ALTER .. do something
\dRp+ pub
ALTER .. RESET
\dRp+ pub

~~~

10.
+-- Verify that associated schemas are reomved from the publication after RESET

typo: /reomved/removed/

~~~

11.
+-- Verify that only superuser can reset a publication
+ALTER PUBLICATION testpub_reset OWNER TO regress_publication_user2;
+SET ROLE regress_publication_user2;
+ALTER PUBLICATION testpub_reset RESET; -- fail - must be superuser
+SET ROLE regress_publication_user;
+

Perhaps this should be the first test?

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