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

On Thu, 23 Mar 2023 at 09:22, Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Mon, Mar 20, 2023 at 8:17 PM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.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