RE: Support logical replication of DDLs

On Tues, Mar 14, 2023 12:17 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> On Mon, Mar 13, 2023 at 2:24 AM Zheng Li <zhengli10(at)gmail(dot)com> wrote:
> >
> > Thanks for working on the test coverage for CREATE and ALTER TABLE.
> > I've made fixes for some of the failures in the v79 patch set (0002,
> > 0003 and 0004 are updated). The changes includes:
> > 1. Fixed a syntax error caused by ON COMMIT clause placement in
> > deparse_CreateStmt.
> > 2. Fixed deparse_Seq_As and start using it in deparse_CreateSeqStmt,
> > this issue is also reported in [1].
> > 3. Fixed a bug in append_not_present: the 'present: false' element
> > can't be omitted even in non-verbose mode. It will cause syntax error
> > on reformed command if 'present: false' element is missing but the fmt
> > string indicates the corresponding object must be present.
> > 4. Replaced if_not_exists with if_exists in deparse of
> > AT_DropConstraint and AT_DropColumn.
> > 5. Added missing CASCADE clause for AT_DropConstraint deparse.
> > 6. Enabled the fixed test cases.
> >
>
> I found out that the option ONLY was not parsed in the "CREATE INDEX"
> command,
> for eg: CREATE UNIQUE INDEX ... ON ONLY table_name ...
>
> I've fixed this in patch 0002.

Thanks for the new patch set.

Here are some comments:

For v-80-0002* patch.
1. The comments atop the function deparse_IndexStmt.
+ * Verbose syntax
+ * CREATE %{unique}s INDEX %{concurrently}s %{if_not_exists}s %{name}I ON
+ * %{table}D USING %{index_am}s %{definition}s %{with}s %{tablespace}s
+ * %{where_clause}s %{nulls_not_distinct}s
+ */
+static ObjTree *
+deparse_IndexStmt(Oid objectId, Node *parsetree)

Since we added decoding for the [ONLY] option in this version, it seems that we
also need to add related comments, like this:
```
%{table}D USING %{index_am}s %{definition}s %{with}s %{tablespace}s
->
%{only}s %{table}D USING %{index_am}s %{definition}s %{with}s %{tablespace}s
```

===

For v-80-0003* patch.
2. In the function deparse_CreateTrigStmt.
I think we need to parse the [OR REPLACE] option for CREATE TRIGGER command.

And I think there are two similar missing in the functions
deparse_DefineStmt_Aggregate (option [OR REPLACE]) and
deparse_DefineStmt_Collation (option [IF NOT EXISTS]).

===

For v-80-0004* patch.
3. There are some whitespace errors:
Applying: Introduce the test_ddl_deparse_regress test module.
.git/rebase-apply/patch:163: new blank line at EOF.
+
.git/rebase-apply/patch:3020: new blank line at EOF.
+
.git/rebase-apply/patch:4114: new blank line at EOF.
+
warning: 3 lines add whitespace errors.

Hou zj and I will try to address these comments soon.

Regards,
Wang wei