Re: Support logical replication of DDLs

On Thu, 23 Mar 2023 at 09:22, Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Mon, Mar 20, 2023 at 8:17 PM houzj(dot)fnst(at)fujitsu(dot)com
> <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> >
> > Attach the new patch set which addressed above comments.
> > 0002,0003,0004 patch has been updated in this version.
> >
> > Best Regards,
> > Hou zj
>
> Attached a patch-set which adds support for ONLY token in ALTER TABLE..
> Changes are in patches 0003 and 0004.

Few comments:
1) The file name should be changed to 033_ddl_replication.pl as 032 is
used already:
diff --git a/src/test/subscription/t/032_ddl_replication.pl
b/src/test/subscription/t/032_ddl_replication.pl
new file mode 100644
index 0000000000..4bc4ff2212
--- /dev/null
+++ b/src/test/subscription/t/032_ddl_replication.pl
@@ -0,0 +1,465 @@
+# Copyright (c) 2022, PostgreSQL Global Development Group
+# Regression tests for logical replication of DDLs
+#
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+

2) Typos
2.a) subcriber should be subscriber:
(Note #2) For ATTACH/DETACH PARTITION, we haven't added extra logic on
the subscriber to handle the case where the table on the publisher is
a PARTITIONED
TABLE while the target table on the subcriber side is a NORMAL table. We will
research this more and improve it later.

2.b) subscrier should be subscriber:
+ For example, when enabled a CREATE TABLE command executed on the
publisher gets
+ WAL-logged, and forwarded to the subscriber to replay; a subsequent "ALTER
+ SUBSCRIPTION ... REFRESH PUBLICATION" is run on the subscrier
database so any
+ following DML changes on the new table can be replicated without a hitch.

3) Error reporting could use new style:
+ break;
+ default:
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid conversion specifier \"%c\"", *cp)));
+ }

4) We could also mention "or the initial tree content is known." as we
have mentioned for the first point:
* c) new_objtree_VA where the complete tree can be derived using some fixed
* content and/or some variable arguments.

5) we could simplify the code by changing:
/*
* Scan pg_constraint to fetch all constraints linked to the given
* relation.
*/
conRel = table_open(ConstraintRelationId, AccessShareLock);
if (OidIsValid(relationId))
{
ScanKeyInit(&key,
Anum_pg_constraint_conrelid,
BTEqualStrategyNumber, F_OIDEQ,
ObjectIdGetDatum(relationId));
scan = systable_beginscan(conRel, ConstraintRelidTypidNameIndexId,
true, NULL, 1, &key);
}
else
{
ScanKeyInit(&key,
Anum_pg_constraint_contypid,
BTEqualStrategyNumber, F_OIDEQ,
ObjectIdGetDatum(domainId));
scan = systable_beginscan(conRel, ConstraintTypidIndexId,
true, NULL, 1, &key);
}

to:

relid = (OidIsValid(relationId)) ? ConstraintRelidTypidNameIndexId
:ConstraintTypidIndexId;
ScanKeyInit(&key,
Anum_pg_constraint_conrelid,
BTEqualStrategyNumber, F_OIDEQ,
ObjectIdGetDatum(relationId));
scan = systable_beginscan(conRel, relid, true, NULL, 1, &key);

6) The tmpstr can be removed by changing:
+static inline ObjElem *
+deparse_Seq_Cache(Form_pg_sequence seqdata, bool alter_table)
+{
+ ObjTree *ret;
+ char *tmpstr;
+ char *fmt;
+
+ fmt = alter_table ? "SET CACHE %{value}s" : "CACHE %{value}s";
+
+ tmpstr = psprintf(INT64_FORMAT, seqdata->seqcache);
+ ret = new_objtree_VA(fmt, 2,
+ "clause",
ObjTypeString, "cache",
+ "value",
ObjTypeString, tmpstr);
+
+ return new_object_object(ret);
+}

to:
ret = new_objtree_VA(fmt, 2,
"clause", ObjTypeString, "cache",
"value", ObjTypeString,
psprintf(INT64_FORMAT, seqdata->seqcache));

7) Same change can be done here too:
static inline ObjElem *
deparse_Seq_IncrementBy(Form_pg_sequence seqdata, bool alter_table)
{
ObjTree *ret;
char *tmpstr;
char *fmt;

fmt = alter_table ? "SET INCREMENT BY %{value}s" : "INCREMENT BY %{value}s";

tmpstr = psprintf(INT64_FORMAT, seqdata->seqincrement);
ret = new_objtree_VA(fmt, 2,
"clause", ObjTypeString, "seqincrement",
"value", ObjTypeString, tmpstr);

return new_object_object(ret);
}

8) same change can be done here too:
static inline ObjElem *
deparse_Seq_Maxvalue(Form_pg_sequence seqdata, bool alter_table)
{
ObjTree *ret;
char *tmpstr;
char *fmt;

fmt = alter_table ? "SET MAXVALUE %{value}s" : "MAXVALUE %{value}s";

tmpstr = psprintf(INT64_FORMAT, seqdata->seqmax);
ret = new_objtree_VA(fmt, 2,
"clause", ObjTypeString, "maxvalue",
"value", ObjTypeString, tmpstr);

return new_object_object(ret);
}

9) same change can be done here too:
static inline ObjElem *
deparse_Seq_Minvalue(Form_pg_sequence seqdata, bool alter_table)
{
ObjTree *ret;
char *tmpstr;
char *fmt;

fmt = alter_table ? "SET MINVALUE %{value}s" : "MINVALUE %{value}s";

tmpstr = psprintf(INT64_FORMAT, seqdata->seqmin);
ret = new_objtree_VA(fmt, 2,
"clause", ObjTypeString, "minvalue",
"value", ObjTypeString, tmpstr);

return new_object_object(ret);
}

10) same change can be done here too:
static inline ObjElem *
deparse_Seq_Restart(int64 last_value)
{
ObjTree *ret;
char *tmpstr;

tmpstr = psprintf(INT64_FORMAT, last_value);
ret = new_objtree_VA("RESTART %{value}s", 2,
"clause", ObjTypeString, "restart",
"value", ObjTypeString, tmpstr);

return new_object_object(ret);
}

11) same change can be done here too:
static inline ObjElem *
deparse_Seq_Startwith(Form_pg_sequence seqdata, bool alter_table)
{
ObjTree *ret;
char *tmpstr;
char *fmt;

fmt = alter_table ? "SET START WITH %{value}s" : "START WITH %{value}s";

tmpstr = psprintf(INT64_FORMAT, seqdata->seqstart);
ret = new_objtree_VA(fmt, 2,
"clause", ObjTypeString, "start",
"value", ObjTypeString, tmpstr);

return new_object_object(ret);
}

12) Verbose syntax should be updated to include definition too:
* Verbose syntax
* CREATE %{persistence}s SEQUENCE %{identity}D
*/
static ObjTree *
deparse_CreateSeqStmt(Oid objectId, Node *parsetree)

13) Verbose syntax should include AUTHORIZATION too:
* Verbose syntax
* CREATE SCHEMA %{if_not_exists}s %{name}I %{authorization}s
*/
static ObjTree *
deparse_CreateSchemaStmt(Oid objectId, Node *parsetree)

14) DROP %s should be DROP %{objtype}s in verbose syntax:

* Verbose syntax
* DROP %s IF EXISTS %%{objidentity}s %{cascade}s
*/
char *
deparse_drop_command(const char *objidentity, const char *objecttype,
DropBehavior behavior)

15) ALTER %s should be ALTER %{objtype}s in verbose syntax:
*
* Verbose syntax
* ALTER %s %{identity}s SET SCHEMA %{newschema}I
*/
static ObjTree *
deparse_AlterObjectSchemaStmt(ObjectAddress address, Node *parsetree,
ObjectAddress old_schema)
{

16) ALTER %s should be ALTER %{identity}s in verbose syntax:

* Verbose syntax
* ALTER %s %{identity}s OWNER TO %{newowner}I
*/
static ObjTree *
deparse_AlterOwnerStmt(ObjectAddress address, Node *parsetree)

17) ALTER TYPE %{identity}D (%{definition: }s) should include SET in
verbose syntax:
* Verbose syntax
* ALTER TYPE %{identity}D (%{definition: }s)
*/
static ObjTree *
deparse_AlterTypeSetStmt(Oid objectId, Node *cmd)

18) extobjtype should be included in the verbose syntax:
* Verbose syntax
* ALTER EXTENSION %{extidentity}I ADD/DROP %{objidentity}s
*/
static ObjTree *
deparse_AlterExtensionContentsStmt(Oid objectId, Node *parsetree,
ObjectAddress objectAddress)

Regards,
Vignesh