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

On Tue, Apr 12, 2022 at 2:23 PM vignesh C <vignesh21@gmail.com> wrote:
> The patch does not apply on top of HEAD because of the recent commit,
> attached patch is rebased on top of HEAD.
Thanks for your patches.

Here are some comments for v1-0001:
1.
I found the patch add the following two new functions in gram.y:
preprocess_alltables_pubobj_list, check_skip_in_pubobj_list.
These two functions look similar. So could we just add one new function?
Besides, do we need the API `location` in new function
preprocess_alltables_pubobj_list? It seems that "location" is not used in this
new function.
In addition, the location of error cursor in the messages seems has a little
problem. For example:
postgres=# create publication pub for all TABLES skip all tables in schema public, table test;
ERROR:  only SKIP ALL TABLES IN SCHEMA or SKIP TABLE can be specified with ALL TABLES option
LINE 1: create publication pub for all TABLES skip all tables in sch...
        ^
(The location of error cursor is under 'create')

2. I think maybe there is a minor missing in function
preprocess_alltables_pubobj_list and check_skip_in_pubobj_list:
We seem to be missing the CURRENT_SCHEMA case.
For example(In function preprocess_alltables_pubobj_list) :
+        /* Only SKIP ALL TABLES IN SCHEMA option supported with ALL TABLES */
+        if (pubobj->pubobjtype != PUBLICATIONOBJ_TABLES_IN_SCHEMA ||
+            !pubobj->skip)
maybe need to be changed like this:
+        /* Only SKIP ALL TABLES IN SCHEMA option supported with ALL TABLES */
+        if ((pubobj->pubobjtype != PUBLICATIONOBJ_TABLES_IN_SCHEMA &&
+            pubobj->pubobjtype != PUBLICATIONOBJ_TABLES_IN_CUR_SCHEMA) &&
+            pubobj->skip)

3. I think maybe there are some minor missing in create_publication.sgml.
+    [ FOR ALL TABLES [SKIP ALL TABLES IN SCHEMA { <replaceable class="parameter">schema_name</replaceable> |
CURRENT_SCHEMA}]
 
maybe need to be changed to this:
+    [ FOR ALL TABLES [SKIP ALL TABLES IN SCHEMA { <replaceable class="parameter">schema_name</replaceable> |
CURRENT_SCHEMA} [, ... ]]
 

4. The error message of function CreatePublication.
Does the message below need to be modified like the comment?
In addition, I think maybe "FOR/SKIP" is better.
@@ -835,18 +843,21 @@ CreatePublication(ParseState *pstate, CreatePublicationStmt *stmt)
-        /* FOR ALL TABLES IN SCHEMA requires superuser */
+        /* FOR [SKIP] ALL TABLES IN SCHEMA requires superuser */
         if (list_length(schemaidlist) > 0 && !superuser())
             ereport(ERROR,
                     errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
                     errmsg("must be superuser to create FOR ALL TABLES IN SCHEMA publication"));

5.
I think there are some minor missing in tab-complete.c.
+             Matches("CREATE", "PUBLICATION", MatchAny, "FOR", "SKIP", "ALL", "TABLES", "IN", "SCHEMA"))
maybe need to be changed to this:
+             Matches("CREATE", "PUBLICATION", MatchAny, "FOR", "ALL", "TABLES", "SKIP", "ALL", "TABLES", "IN",
"SCHEMA"))

+              Matches("CREATE", "PUBLICATION", MatchAny, "SKIP", "FOR", "ALL", "TABLES", "IN", "SCHEMA", MatchAny))
&&
maybe need to be changed to this:
+              Matches("CREATE", "PUBLICATION", MatchAny, "FOR", "ALL", "TABLES", "SKIP", "ALL", "TABLES", "IN",
"SCHEMA",MatchAny)) &&
 

6.
In function get_rel_sync_entry, do we need `if (!publish)` in below code?
I think `publish` is always false here, as we delete the check for
"pub->alltables".
```
-            /*
-             * If this is a FOR ALL TABLES publication, pick the partition root
-             * and set the ancestor level accordingly.
-             */
-            if (pub->alltables)
-            {
-                ......
-            }
-
             if (!publish)
```

Regards,
Wang wei