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

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.1
0001 patch, in deparse_RenameStmt().
OBJECT_ATTRIBUTE is type's attribute.

+            tmp_obj = new_objtree("CASCADE");
+            if (node->renameType != OBJECT_ATTRIBUTE ||
+                node->behavior != DROP_CASCADE)
+                append_not_present(tmp_obj);
+            append_object_object(ret, "cascade", tmp_obj);

1.2
0001 patch, in deparse_AlterRelation().

+            case AT_AddColumnToView:
+                /* CREATE OR REPLACE VIEW -- nothing to do here */
+                break;

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));
+}

1.4
0001 patch, in deparse_AlterSeqStmt().

I think only "owned_by" option is needed, other options can't be generated by
"alter table" command.

+    foreach(cell, ((AlterSeqStmt *) parsetree)->options)
+    {
+        DefElem    *elem = (DefElem *) lfirst(cell);
+        ObjElem    *newelm;
+
+        if (strcmp(elem->defname, "cache") == 0)
+            newelm = deparse_Seq_Cache(seqform, false);
+        else if (strcmp(elem->defname, "cycle") == 0)
+            newelm = deparse_Seq_Cycle(seqform, false);
+        else if (strcmp(elem->defname, "increment") == 0)
+            newelm = deparse_Seq_IncrementBy(seqform, false);
+        else if (strcmp(elem->defname, "minvalue") == 0)
+            newelm = deparse_Seq_Minvalue(seqform, false);
+        else if (strcmp(elem->defname, "maxvalue") == 0)
+            newelm = deparse_Seq_Maxvalue(seqform, false);
+        else if (strcmp(elem->defname, "start") == 0)
+            newelm = deparse_Seq_Startwith(seqform, false);
+        else if (strcmp(elem->defname, "restart") == 0)
+            newelm = deparse_Seq_Restart(seqvalues->last_value);
+        else if (strcmp(elem->defname, "owned_by") == 0)
+            newelm = deparse_Seq_OwnedBy(objectId, false);
+        else if (strcmp(elem->defname, "as") == 0)
+            newelm = deparse_Seq_As(seqform);
+        else
+            elog(ERROR, "invalid sequence option %s", elem->defname);
+
+        elems = lappend(elems, newelm);
+    }

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;

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;

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

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?

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

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.

Regards,
Shi Yu