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

Hi Vignesh.

Some review comments for patch v4-0001.

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

1.
-    [ ONLY ] <replaceable class="parameter">table_name</replaceable> [ * ]
+    TABLE <replaceable class="parameter">table_object</replaceable> [, ... ]
+
+<phrase>and <replaceable class="parameter">table_object</replaceable>
is:</phrase>
+
+   [ ONLY ] <replaceable class="parameter">table_name</replaceable> [ * ]

With the introduction of 'table_object' I expected that
'table_and_columns' and 'publication_drop_object' would also make use
of it. Why not?

======
doc/src/sgml/ref/create_publication.sgml

2.
-    [ ONLY ] <replaceable class="parameter">table_name</replaceable> [ * ]
+    TABLE <replaceable class="parameter">table_object</replaceable> [, ... ]
+
+<phrase>and <replaceable class="parameter">table_object</replaceable>
is:</phrase>
+
+   [ ONLY ] <replaceable class="parameter">table_name</replaceable> [ * ]

With the introduction of 'table_object' I expected that
'table_and_columns' would also make use of it. Why not?

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

3.
@@ -11399,7 +11399,7 @@ pub_obj_list: PublicationObjSpec
  ;

 opt_pub_except_clause:
- EXCEPT TABLE '(' pub_except_obj_list ')' { $$ = $4; }
+ EXCEPT '(' TABLE pub_except_obj_list ')' { $$ = $4; }
  | /*EMPTY*/ { $$ = NIL; }
  ;

@@ -11439,8 +11439,8 @@ PublicationExceptObjSpec:

 pub_except_obj_list: PublicationExceptObjSpec
  { $$ = list_make1($1); }
- | pub_except_obj_list ',' PublicationExceptObjSpec
- { $$ = lappend($1, $3); }
+ | pub_except_obj_list ',' opt_table PublicationExceptObjSpec
+ { $$ = lappend($1, $4); }
  ;

IMO we should put 'table' in all those table-specific production names:
e.g. opt_pub_except_clause ==> opt_pub_except_table_clause
e.g. pub_except_obj_list ==> pub_except_tableobj_list
e.g. PublicationExceptObjSpec ==> PublicationExceptTableObjSpec

IIUC, in future when "FOR ALL SEQUENCES EXCEPT (SEQUENCES ...)" and/or
"FOR ALL SCHEMAS EXCEPT (SCHEMA ...)" are implemented then these names
won't be much good anymore, so I thought they should be made
table-specific now to avoid churning them later.

======
src/bin/psql/tab-complete.in.c

4.
- COMPLETE_WITH("EXCEPT TABLE (");
+ COMPLETE_WITH("EXCEPT ( TABLE");

(this is in several places in  ALTER and CREATE)

In v3 the space in "( TABLE" was changed to "(TABLE", but now in v4
the space is back again. AFAICT the v3 change was in response to
review [1] (comment #2). Was it reverted deliberately?

======
[1] https://www.postgresql.org/message-id/CAHg%2BQDe%2B7J0e1JLjpqe-NxVJMWXH6UDtqugrm%2BDP7rHEJLrBqQ%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia