RE: Support logical replication of DDLs

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