MIME-Version: 1.0
References: 
 <CAAD30ULCxqOJp0sffm_y9jNC4BVPYv7Q7_va_JE8qyfRXkfu+g@mail.gmail.com>
 <CAFPTHDaaewvYUznZD1YjUQnvHycgvgMKNK4w=V8Q-8MKTpVDrw@mail.gmail.com>
 <OS3PR01MB6275025C397CC22E446B1F589EBC9@OS3PR01MB6275.jpnprd01.prod.outlook.com>
 <OS0PR01MB5716765B2D38786DA3943E0294809@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAFPTHDbAJPccMcZnOraBy14hM6qBJxYqcRwK6iV6=gtL6VZQwQ@mail.gmail.com>
In-Reply-To: 
 <CAFPTHDbAJPccMcZnOraBy14hM6qBJxYqcRwK6iV6=gtL6VZQwQ@mail.gmail.com>
From: vignesh C <vignesh21@gmail.com>
Date: Thu, 23 Mar 2023 19:06:32 +0530
Message-ID: 
 <CALDaNm3NUO8ofK64N7HMtNmUP=52R8_jWzrekqAm7m7wqZjwaQ@mail.gmail.com>
Subject: Re: Support logical replication of DDLs
To: Ajin Cherian <itsajin@gmail.com>
Cc: "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>,
	"wangw.fnst@fujitsu.com" <wangw.fnst@fujitsu.com>,
 Runqi Tian <runqidev@gmail.com>,
	Peter Smith <smithpb2250@gmail.com>, li jie <ggysxcq@gmail.com>,
	Dilip Kumar <dilipbalaut@gmail.com>,
 Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Amit Kapila <amit.kapila16@gmail.com>,
 Masahiko Sawada <sawada.mshk@gmail.com>,
	Japin Li <japinli@hotmail.com>, rajesh singarapu <rajesh.rs0541@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Zheng Li <zhengli10@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CALDaNm3NUO8ofK64N7HMtNmUP%3D52R8_jWzrekqAm7m7wqZjwaQ%40mail.gmail.com>
Precedence: bulk

On Thu, 23 Mar 2023 at 09:22, Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Mon, Mar 20, 2023 at 8:17=E2=80=AFPM 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 wi=
ll
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 "A=
LTER
+    SUBSCRIPTION ... REFRESH PUBLICATION" is run on the subscrier
database so any
+    following DML changes on the new table can be replicated without a hit=
ch.

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 fixe=
d
 *     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 =3D table_open(ConstraintRelationId, AccessShareLock);
if (OidIsValid(relationId))
{
ScanKeyInit(&key,
Anum_pg_constraint_conrelid,
BTEqualStrategyNumber, F_OIDEQ,
ObjectIdGetDatum(relationId));
scan =3D systable_beginscan(conRel, ConstraintRelidTypidNameIndexId,
  true, NULL, 1, &key);
}
else
{
ScanKeyInit(&key,
Anum_pg_constraint_contypid,
BTEqualStrategyNumber, F_OIDEQ,
ObjectIdGetDatum(domainId));
scan =3D systable_beginscan(conRel, ConstraintTypidIndexId,
  true, NULL, 1, &key);
}

to:

relid =3D (OidIsValid(relationId)) ? ConstraintRelidTypidNameIndexId
:ConstraintTypidIndexId;
ScanKeyInit(&key,
Anum_pg_constraint_conrelid,
BTEqualStrategyNumber, F_OIDEQ,
ObjectIdGetDatum(relationId));
scan =3D 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 =3D alter_table ? "SET CACHE %{value}s" : "CACHE %{value}s";
+
+       tmpstr =3D psprintf(INT64_FORMAT, seqdata->seqcache);
+       ret =3D new_objtree_VA(fmt, 2,
+                                                "clause",
ObjTypeString, "cache",
+                                                "value",
ObjTypeString, tmpstr);
+
+       return new_object_object(ret);
+}

to:
ret =3D 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 =3D alter_table ? "SET INCREMENT BY %{value}s" : "INCREMENT BY %{value}=
s";

tmpstr =3D psprintf(INT64_FORMAT, seqdata->seqincrement);
ret =3D 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 =3D alter_table ? "SET MAXVALUE %{value}s" : "MAXVALUE %{value}s";

tmpstr =3D psprintf(INT64_FORMAT, seqdata->seqmax);
ret =3D 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 =3D alter_table ? "SET MINVALUE %{value}s" : "MINVALUE %{value}s";

tmpstr =3D psprintf(INT64_FORMAT, seqdata->seqmin);
ret =3D 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 =3D psprintf(INT64_FORMAT, last_value);
ret =3D 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 =3D alter_table ? "SET START WITH %{value}s" : "START WITH %{value}s";

tmpstr =3D psprintf(INT64_FORMAT, seqdata->seqstart);
ret =3D 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