RE: Support logical replication of DDLs

On Mon, May 22, 2023 1:57 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> Please find the new set of patches for object-tree Removal. The new
> changes are in patch 0008 only. The new changes address object tree
> removal for below commands.
>
> create sequence
> alter sequence
> alter object owner to
> alter object set schema
> alter object rename
>
> In this patch 0008, ddldeparse.c is now object-tree free for all the
> table related commands. Index related commands are yet to be done.
>

Thanks for updating the patch. Here are some comments.

0001 patch
-----
1.
+ colname = get_attname(ownerId, depform->refobjsubid, false);
+ if (colname == NULL)
+ continue;

missing_ok is false when calling get_attname(), so is there any case that
colname is NULL?

2.
+ case AT_SetStatistics:
+ {
+ Assert(IsA(subcmd->def, Integer));
+ if (subcmd->name)
+ tmp_obj = new_objtree_VA("ALTER COLUMN %{column}I SET STATISTICS %{statistics}n", 3,
+ "type", ObjTypeString, "set statistics",
+ "column", ObjTypeString, subcmd->name,
+ "statistics", ObjTypeInteger,
+ intVal((Integer *) subcmd->def));
+ else
+ tmp_obj = new_objtree_VA("ALTER COLUMN %{column}n SET STATISTICS %{statistics}n", 3,
+ "type", ObjTypeString, "set statistics",
+ "column", ObjTypeInteger, subcmd->num,
+ "statistics", ObjTypeInteger,
+ intVal((Integer *) subcmd->def));
+ subcmds = lappend(subcmds, new_object_object(tmp_obj));
+ }
+ break;

I think subcmd->name will be NULL only if relation type is index. So should it
be removed because currently only table commands are supported?

0002 patch
-----
3.
+ /* Skip adding constraint for inherits table sub command */
+ if (!constrOid)
+ continue;

Would it be better to use OidIsValid() here?

0008 patch
-----
4.
case AT_AddColumn:
/* XXX need to set the "recurse" bit somewhere? */
Assert(IsA(subcmd->def, ColumnDef));
- tree = deparse_ColumnDef(rel, dpcontext, false,
- (ColumnDef *) subcmd->def, true, &expr);

mark_function_volatile(context, expr);

After this change, `expr` is not assigned a value when mark_function_volatile is called.

Some problems I saw :
-----
5.
create table p1(f1 int);
create table p1_c1() inherits(p1);
alter table p1 add constraint inh_check_constraint1 check (f1 > 0);
alter table p1_c1 add constraint inh_check_constraint1 check (f1 > 0);

The re-formed command of the last command is "ALTER TABLE public.p1_c1", which
seems to be wrong.

6.
SET allow_in_place_tablespaces = true;
CREATE TABLESPACE ddl_tblspace LOCATION '';
RESET allow_in_place_tablespaces;
CREATE TABLE tbl_index_tblspe (a int, PRIMARY KEY(a) USING INDEX TABLESPACE ddl_tblspace) ;

The re-formed command of the last command seems incorrect:
CREATE TABLE public.tbl_index_tblspe (a pg_catalog.int4 STORAGE PLAIN , USING INDEX TABLESPACE ddl_tblspace)

7.
CREATE TABLE part2_with_multiple_storage_params(
id int,
name varchar
) WITH (autovacuum_enabled);

re-formed command: CREATE TABLE public.part2_with_multiple_storage_params (id pg_catalog.int4 STORAGE PLAIN , name pg_catalog."varchar" STORAGE EXTENDED COLLATE pg_catalog."default" ) WITH (vacuum_index_cleanup = 'on', autovacuum_vacuum_scale_factor = '0.2', vacuum_truncate = 'true', autovacuum_enabled = 'TRUE')

When the option is not specified, re-formed command used uppercase letters. The
reloptions column in pg_class of the original command is
"{autovacuum_enabled=true}", but that of the re-formed command is
"{autovacuum_enabled=TRUE}". I tried to add this case to
test_ddl_deparse_regress test module but the test failed because the dumped
results are different.

Regards,
Shi Yu