On Fri, Apr 7, 2023 11:23 AM Hou, Zhijie/侯 志杰 <houzj.f...@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
