RE: Skipping schema changes in publication

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