Re: Support logical replication of DDLs - Mailing list pgsql-hackers

I have presented some comments below:

1. AT_AddColumn

> + tmpobj = new_objtree_VA("ADD %{objtype}s %{definition}s", 3,
[ IF NOT EXISTS ] is missing here.

2.  AT_DropColumn
> + tmpobj = new_objtree_VA("DROP %{objtype}s %{column}I", 3,
[ IF EXISTS ] is missing here.

3. AT_DropConstraint
> + tmpobj = new_objtree_VA("DROP CONSTRAINT %{constraint}I", 2,
[ IF EXISTS ] is missing here.

4. AT_DetachPartition
> + tmpobj = new_objtree_VA("DETACH PARTITION %{partition_identity}D", 2,
[ CONCURRENTLY | FINALIZE ] is missing here.

5. deparse_CreateSeqStmt
> + ret = new_objtree_VA("CREATE %{persistence}s SEQUENCE %{identity}D %{definition: }s", 3,
[ IF NOT EXISTS ] is missing here.

6. deparse_IndexStmt
> + ret = new_objtree_VA("CREATE %{unique}s INDEX %{concurrently}s %{if_not_exists}s %{name}I ON %{table}D USING
%{index_am}s(%{definition}s)", 7, 
[ INCLUDE ] and [ ONLY ] are  missing here.

7. deparse_RuleStmt
> + foreach(cell, actions)
> + list = lappend(list, new_string_object(lfirst(cell)));

if (actions == NIL)
 list = lappend(list, new_string_object("NOTHING"));
else
{
  foreach(cell, actions)
  list = lappend(list, new_string_object(lfirst(cell)));
}

8. AT_AddIndexConstraint
> + tmpobj = new_objtree_VA("ADD CONSTRAINT %{name}I %{constraint_type}s USING INDEX %{index_name}I %{deferrable}s
%{init_deferred}s",6, 
> + "type", ObjTypeString, "add constraint using index",
> + "name", ObjTypeString, get_constraint_name(constrOid),
> + "constraint_type", ObjTypeString,
> + istmt->deferrable ? "DEFERRABLE" : "NOT DEFERRABLE",

"constraint_type", ObjTypeString,
istmt->primary ? "PRIMARY KEY" : "UNIQUE",

9. regress test

Zheng Li <zhengli10@gmail.com> 于2022年12月12日周一 12:58写道：


>
> Hi,
>
> Attached please find the DDL deparser testing module in the v45-0007
> patch, this testing module is written by Runqi Tian in [1] with minor
> modification from myself. I think we can
> start adding more tests to the module now that we're getting close to
> finish implementing the DDL deparser.
>
> This testing module ddl_deparse_regress aims to achieve the following
> four testing goals for the DDL deparser:
>     1. Test that the generated JSON blob is expected using SQL tests.
>     2. Test that the re-formed DDL command is expected using SQL tests.
>     3. Test that the re-formed DDL command has the same effect as the
> original command
>        by comparing the results of pg_dump, using the SQL tests in 1 and 2.
>     4. Test that any new DDL syntax is handled by the DDL deparser by
> capturing and deparsing
>        DDL commands ran by pg_regress.
>
> 1 and 2 is tested with SQL tests, by comparing the deparsed JSON blob
> and the re-formed command.
> 3 is tested with TAP framework in t/001_compare_dumped_results.pl
> 4 is tested with TAP framework and pg_regress in 002_regress_tests.pl,
> the execution is currently commented out because it will fail due
> unimplemented commands in the DDL deparser.
>
> [1]
https://www.postgresql.org/message-id/flat/CAH8n8_jMTunxxtP4L-3tc%3DGNamg%3Dmg1X%3DtgHr9CqqjjzFLwQng%40mail.gmail.com
>

The test patch is very useful.
 I see that the sql case in test_ddl_deparse_regress is similar to the
one in test_ddl_deparse.
Why don't we merge the test cases in test_ddl_deparse_regress into
test_ddl_deparse,
as the sql case can be completely reused with the sql files in test_ddl_deparse?
I believe this will make the tests more comprehensive and reduce redundancy.


Regards,
li jie