RE: Support logical replication of DDLs

On Wed, May 31, 2023 5:41 PM shveta malik <shveta(dot)malik(at)gmail(dot)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