RE: Support logical replication of DDLs

On Mon, May 22, 2023 1:57 PM shveta malik <shveta.malik@gmail.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