Re: Support logical replication of DDLs

On Mon, May 29, 2023 at 11:45 AM Yu Shi (Fujitsu) <shiy.fnst@fujitsu.com> wrote:
>
>
>
> Thanks for updating the patch. Here are some comments.
>

Thanks Shi-san for the review.

> 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?
>

Removed this check in patch 0008 .

> 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?
>

Removed this check in patch 0008

> 0002 patch
> -----
> 3.
> +                                       /* Skip adding constraint for inherits table sub command */
> +                                       if (!constrOid)
> +                                               continue;
>
> Would it be better to use OidIsValid() here?
>

yes, modified in patch 0008

> 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.
>

Corrected.

> 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.
>

Fixed, second alter-table should actually be no-op in terms of
deparsing. But when it is run without running the first alter-table
command, it should generate the reformed command.

> 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)
>

Fixed.

> 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.
>

Changed to lowercase for the sake of tests.


PFA the set of patches consisting above changes. All the changes are
made in 0008 patch.

Apart from above changes, many partition attach/detach related tests
are uncommented in alter_table.sql in patch 0008.

thanks
Shveta