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

On Thu, 23 Mar 2023 at 19:06, vignesh C <vignesh21@gmail.com> wrote:
>
> 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;
> +

Modified

> 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.

Modified

> 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.

Modified

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

Modified

>
> 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.

Modified

>  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);

Modified

> 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));

Modified

> 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);
> }

Modified

> 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);
> }

Modified

> 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);
> }

Modified

> 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);
> }

Modified

> 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);
> }

Modified

> 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)

Modified

> 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)

Modified

> 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)

Modified

> 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)
> {

Modified

> 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)

Modified

> 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)

Modified

> 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)

Modified

Since there are many people working on this project and we do minor
changes on top of each version, I felt it is better to use DATE along
with the patches.
The attached patch has the changes for the same.

Regards,
Vignesh