RE: Support logical replication of DDLs

On Fri, Apr 7, 2023 11:23 AM Hou, Zhijie/侯 志杰 <houzj(dot)fnst(at)fujitsu(dot)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