RE: Support logical replication of DDLs

On Fri, Apr 7, 2023 11:23 AM Hou, Zhijie/侯 志杰 <houzj.fnst@fujitsu.com> wrote:
>

Thanks for updating the patch set.

Here are some comments:
1. The function deparse_drop_command in 0001 patch and the function
publication_deparse_ddl_command_end in 0002 patch.
```
+/*
+ * Handle deparsing of DROP commands.
+ *
+ * Verbose syntax
+ * DROP %s IF EXISTS %%{objidentity}s %{cascade}s
+ */
+char *
+deparse_drop_command(const char *objidentity, const char *objecttype,
+                     DropBehavior behavior)
+{
+   .....
+    stmt = new_objtree_VA("DROP %{objtype}s IF EXISTS %{objidentity}s", 2,
+                          "objtype", ObjTypeString, objecttype,
+                          "objidentity", ObjTypeString, identity);
```

I think the option "IF EXISTS" here will be forced to be parsed regardless of
whether it is actually specified by user. Also, I think we seem to be missing
another option for parsing (DropStmt->concurrent).

===
For patch 0002
2. The function parse_publication_options
2.a
```
 static void
 parse_publication_options(ParseState *pstate,
                           List *options,
+                          bool for_all_tables,
                           bool *publish_given,
                           PublicationActions *pubactions,
                           bool *publish_via_partition_root_given,
-                          bool *publish_via_partition_root)
+                          bool *publish_via_partition_root,
+                          bool *ddl_type_given)
 {
```

It seems there is nowhere to ues the parameter "for_all_tables". I think we
could revert this change.

2.b
```
@@ -123,7 +129,7 @@ parse_publication_options(ParseState *pstate,
             pubactions->pubtruncate = false;
 
             *publish_given = true;
-            publish = defGetString(defel);
+            publish = pstrdup(defGetString(defel));
 
             if (!SplitIdentifierString(publish, ',', &publish_list))
                 ereport(ERROR,
```

I think it is fine to only invoke the function defGetString here. Is there any
special reason to invoke the function pstrdup?

Regards,
Wang wei