Re: Support logical replication of DDLs

On Thu, 1 Jun 2023 at 07:42, Yu Shi (Fujitsu) <shiy.fnst@fujitsu.com> wrote:
>
> On Wed, May 31, 2023 5:41 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > 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 for updating the patch, here are some comments.
>
> 1.
> I think some code can be removed because it is not for supporting table
> commands.

> 1.2
> 0001 patch, in deparse_AlterRelation().
>
> +                       case AT_AddColumnToView:
> +                               /* CREATE OR REPLACE VIEW -- nothing to do here */
> +                               break;

Modified

> 1.3
> 0001 patch.
> ("alter table ... owner to ... " is deparsed in deparse_AlterRelation(), instead
> of this funciton.)
>
> +static ObjTree *
> +deparse_AlterOwnerStmt(ObjectAddress address, Node *parsetree)
> +{
> +       AlterOwnerStmt *node = (AlterOwnerStmt *) parsetree;
> +
> +       return new_objtree_VA("ALTER %{objtype}s %{identity}s OWNER TO %{newowner}I", 3,
> +                                                 "objtype", ObjTypeString,
> +                                                 stringify_objtype(node->objectType),
> +                                                 "identity", ObjTypeString,
> +                                                 getObjectIdentity(&address, false),
> +                                                 "newowner", ObjTypeString,
> +                                                 get_rolespec_name(node->newowner));
> +}

Modified

> 2. 0001 patch, in deparse_AlterTableStmt(),
>
> +                       case AT_CookedColumnDefault:
> +                               {
> +                                       Relation        attrrel;
> +                                       HeapTuple       atttup;
> +                                       Form_pg_attribute attStruct;
> ...
>
> I think this case can be removed because it is for "create table like", and in
> such case the function has returned before reaching here, see below.
>
> +       /*
> +        * ALTER TABLE subcommands generated for TableLikeClause is processed in
> +        * the top level CREATE TABLE command; return empty here.
> +        */
> +       if (stmt->table_like)
> +               return NULL;

Modified

> 3. 0001 patch, in deparse_AlterRelation().
>
> Is there any case that "ALTER TABLE" command adds an index which is not a
> constraint? If not, maybe we can remove the check or replace it with an assert.
>
> +                       case AT_AddIndex:
> +                               {
> ...
> +
> +                                       if (!istmt->isconstraint)
> +                                               break;

Modified to Assert

> 4. To run this test when building with meson, it seems we should add
> "test_ddl_deparse_regress" to src/test/modules/meson.build.

Modified

> 5.
> create table t1 (a int);
> create table t2 (a int);
> SET allow_in_place_tablespaces = true;
> CREATE TABLESPACE ddl_tblspace LOCATION '';
> RESET allow_in_place_tablespaces;
> ALTER TABLE ALL IN TABLESPACE pg_default SET TABLESPACE ddl_tblspace;
>
> In the last command, if multiple tables are altered, the command will be
> deparsed multiple times and there will be multiple re-formed commands. Is it OK?

Modified to  "ALTER TABLE .,, SET TABLESPACE" for each of the tables
who are getting altered. We have to generate subcommands for each
table because of the existing alter table trigger callbacks.

> 6.
> Cfbot failed because of compiler warnings. [1]
>
> [15:06:48.065] ddldeparse.c: In function ‘deparse_Seq_OwnedBy_toJsonb’:
> [15:06:48.065] ddldeparse.c:586:14: error: variable ‘value’ set but not used [-Werror=unused-but-set-variable]
> [15:06:48.065]   586 |  JsonbValue *value = NULL;
> [15:06:48.065]       |              ^~~~~
> [15:06:48.065] ddldeparse.c: In function ‘deparse_utility_command’:
> [15:06:48.065] ddldeparse.c:2729:18: error: ‘rel’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> [15:06:48.065]  2729 |      seq_relid = getIdentitySequence(RelationGetRelid(rel), attnum, true);
> [15:06:48.065]       |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> [15:06:48.065] ddldeparse.c:1935:11: note: ‘rel’ was declared here
> [15:06:48.065]  1935 |  Relation rel;
> [15:06:48.065]       |           ^~~
> [15:06:48.065] ddldeparse.c:2071:5: error: ‘dpcontext’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> [15:06:48.065]  2071 |     deparse_ColumnDef_toJsonb(state, rel, dpcontext,
> [15:06:48.065]       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> [15:06:48.065]  2072 |             false, (ColumnDef *) subcmd->def,
> [15:06:48.065]       |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> [15:06:48.065]  2073 |             true, NULL);
> [15:06:48.065]       |             ~~~~~~~~~~~
> [15:06:48.065] ddldeparse.c:1934:11: note: ‘dpcontext’ was declared here
> [15:06:48.065]  1934 |  List    *dpcontext;
> [15:06:48.065]       |           ^~~~~~~~~
> [15:06:48.065] cc1: all warnings being treated as errors
> [15:06:48.065] make[3]: *** [<builtin>: ddldeparse.o] Error 1
> [15:06:48.065] make[2]: *** [common.mk:37: commands-recursive] Error 2
> [15:06:48.065] make[2]: *** Waiting for unfinished jobs....
> [15:06:54.423] make[1]: *** [Makefile:42: all-backend-recurse] Error 2
> [15:06:54.423] make: *** [GNUmakefile:21: world-bin-src-recurse] Error 2
>
> [1] https://cirrus-ci.com/task/5140006247858176

Modified

> 7.
> In deparse_AlterRelation(),
>         stmt = (AlterTableStmt *) cmd->parsetree;
>
>         Assert(IsA(stmt, AlterTableStmt) || IsA(stmt, AlterTableMoveAllStmt));
>
>         initStringInfo(&fmtStr);
>         pushJsonbValue(&state, WJB_BEGIN_OBJECT, NULL);
>
>         /* Start constructing fmt string */
>         if (IsA(stmt, AlterTableStmt))
>         {
>                 stmt = (AlterTableStmt *) cmd->parsetree;
>
>                 /*
>                  * ALTER TABLE subcommands generated for TableLikeClause is processed in
>                  * the top level CREATE TABLE command; return empty here.
>                  */
>                 if (IsA(stmt, AlterTableStmt) && stmt->table_like)
>
>
> `stmt` is assigned twice, and `IsA(stmt, AlterTableStmt)` is checked twice.

Modified

Comments 1.1 and 1.4 are not yet fixed, it will be fixed in the
upcoming version. The rest of the comments are fixed in the patch
posted at [1].

[1] - https://www.postgresql.org/message-id/CAJpy0uBwCZCniPR6vh26L%2BwpSf4xzUN8omUa9DzF-x1CAud_xA%40mail.gmail.com

Regards,
Vignesh