Re: Support logical replication of DDLs

On Fri, 4 Nov 2022 at 15:06, vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Wed, 2 Nov 2022 at 05:13, vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > On Mon, 31 Oct 2022 at 16:17, vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > >
> > > On Thu, 27 Oct 2022 at 16:02, vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > > >
> > > > On Thu, 27 Oct 2022 at 02:09, Zheng Li <zhengli10(at)gmail(dot)com> wrote:
> > > > >
> > > > > > Adding support for deparsing of CREATE/ALTER/DROP LANGUAGE for ddl replication.
> > > > >
> > > > > Adding support for deparsing of:
> > > > > COMMENT
> > > > > ALTER DEFAULT PRIVILEGES
> > > > > CREATE/DROP ACCESS METHOD
> > > >
> > > > Adding support for deparsing of:
> > > > ALTER/DROP ROUTINE
> > > >
> > > > The patch also includes fixes for the following issues:
> > >
> >
> Few comments:
> 1) If the user has specified a non-existing object, then we will throw
> the wrong error.
> +Datum
> +publication_deparse_ddl_command_start(PG_FUNCTION_ARGS)
> +{
> + EventTriggerData *trigdata;
> + char *command = psprintf("Drop table command start");
> + DropStmt *stmt;
> + ListCell *cell1;
> +
> + if (!CALLED_AS_EVENT_TRIGGER(fcinfo))
> + elog(ERROR, "not fired by event trigger manager");
> +
> + trigdata = (EventTriggerData *) fcinfo->context;
> + stmt = (DropStmt *) trigdata->parsetree;
> +
> + /* extract the relid from the parse tree */
> + foreach(cell1, stmt->objects)
> + {
> + char relpersist;
> + Node *object = lfirst(cell1);
> + ObjectAddress address;
> + Relation relation = NULL;
> +
> + address = get_object_address(stmt->removeType,
> + object,
> +
> &relation,
> +
> AccessExclusiveLock,
> + true);
> +
> + relpersist = get_rel_persistence(address.objectId);
>
> We could check relation is NULL after getting address and skip
> processing that object

Modified

> 2) Materialized view handling is missing:
> + switch (rel->rd_rel->relkind)
> + {
> + case RELKIND_RELATION:
> + case RELKIND_PARTITIONED_TABLE:
> + reltype = "TABLE";
> + break;
> + case RELKIND_INDEX:
> + case RELKIND_PARTITIONED_INDEX:
> + reltype = "INDEX";
> + break;
> + case RELKIND_VIEW:
> + reltype = "VIEW";
> + break;
> + case RELKIND_COMPOSITE_TYPE:
> + reltype = "TYPE";
> + istype = true;
> + break;
>
> We could use this scenario for debugging and verifying:
> ALTER MATERIALIZED VIEW testschema.amv SET TABLESPACE regress_tblspace;

Modified

> 3) Readdition of alter table readd statistics is not handled:
>
> + case AT_DropIdentity:
> + tmpobj = new_objtree_VA("ALTER COLUMN
> %{column}I DROP IDENTITY", 2,
> +
> "type", ObjTypeString, "drop identity",
> +
> "column", ObjTypeString, subcmd->name);
> +
> + append_string_object(tmpobj,
> "%{if_not_exists}s",
> +
> subcmd->missing_ok ? "IF EXISTS" : "");
> +
> + subcmds = lappend(subcmds,
> new_object_object(tmpobj));
> + break;
> + default:
> + elog(WARNING, "unsupported alter table
> subtype %d",
> + subcmd->subtype);
> + break;
> + }
>
>
> We could use this scenario for debugging and verifying:
> CREATE TABLE functional_dependencies (
> filler1 TEXT,
> filler2 NUMERIC,
> a INT,
> b TEXT,
> filler3 DATE,
> c INT,
> d TEXT
> )
> WITH (autovacuum_enabled = off);
> CREATE STATISTICS func_deps_stat (dependencies) ON a, b, c FROM
> functional_dependencies;
> TRUNCATE functional_dependencies;
> ALTER TABLE functional_dependencies ALTER COLUMN c TYPE numeric;

Modified

> 4) "Alter sequence as" option not hanlded
>
> + if (strcmp(elem->defname, "cache") == 0)
> + newelm = deparse_Seq_Cache(alterSeq, seqform, false);
> + else if (strcmp(elem->defname, "cycle") == 0)
> + newelm = deparse_Seq_Cycle(alterSeq, seqform, false);
> + else if (strcmp(elem->defname, "increment") == 0)
> + newelm = deparse_Seq_IncrementBy(alterSeq,
> seqform, false);
> + else if (strcmp(elem->defname, "minvalue") == 0)
> + newelm = deparse_Seq_Minvalue(alterSeq, seqform, false);
> + else if (strcmp(elem->defname, "maxvalue") == 0)
> + newelm = deparse_Seq_Maxvalue(alterSeq, seqform, false);
> + else if (strcmp(elem->defname, "start") == 0)
> + newelm = deparse_Seq_Startwith(alterSeq,
> seqform, false);
> + else if (strcmp(elem->defname, "restart") == 0)
> + newelm = deparse_Seq_Restart(alterSeq, seqdata);
> + else if (strcmp(elem->defname, "owned_by") == 0)
> + newelm = deparse_Seq_OwnedBy(alterSeq, objectId, false);
> + else
> + elog(ERROR, "invalid sequence option %s",
> elem->defname);
>
> We could use this scenario for debugging and verifying:
> ALTER SEQUENCE seq1 AS smallint;

Modified

> 5) alter table row level security is not handled:
>
> + case AT_DropIdentity:
> + tmpobj = new_objtree_VA("ALTER COLUMN
> %{column}I DROP IDENTITY", 2,
> +
> "type", ObjTypeString, "drop identity",
> +
> "column", ObjTypeString, subcmd->name);
> +
> + append_string_object(tmpobj,
> "%{if_not_exists}s",
> +
> subcmd->missing_ok ? "IF EXISTS" : "");
> +
> + subcmds = lappend(subcmds,
> new_object_object(tmpobj));
> + break;
> + default:
> + elog(WARNING, "unsupported alter table
> subtype %d",
> + subcmd->subtype);
> + break;
>
> We could use this scenario for debugging and verifying:
> CREATE TABLE r1 (a int);
> ALTER TABLE r1 FORCE ROW LEVEL SECURITY;

Modified

> 6) alter table add primary key is not handled:
>
> + case AT_DropIdentity:
> + tmpobj = new_objtree_VA("ALTER COLUMN
> %{column}I DROP IDENTITY", 2,
> +
> "type", ObjTypeString, "drop identity",
> +
> "column", ObjTypeString, subcmd->name);
> +
> + append_string_object(tmpobj,
> "%{if_not_exists}s",
> +
> subcmd->missing_ok ? "IF EXISTS" : "");
> +
> + subcmds = lappend(subcmds,
> new_object_object(tmpobj));
> + break;
> + default:
> + elog(WARNING, "unsupported alter table
> subtype %d",
> + subcmd->subtype);
> + break;
>
> We could use this scenario for debugging and verifying:
> create table idxpart (a int) partition by range (a);
> create table idxpart0 (like idxpart);
> alter table idxpart0 add primary key (a);
> alter table idxpart attach partition idxpart0 for values from (0) to (1000);
> alter table only idxpart add primary key (a);

Modified

> 7) Code not updated based on new change:
>
> 7.a) identity_column should be removed from new_objtree_VA
> + case AT_AddIdentity:
> + {
> + AttrNumber attnum;
> + Oid seq_relid;
> + ObjTree *seqdef;
> + ColumnDef *coldef =
> (ColumnDef *) subcmd->def;
> +
> + tmpobj = new_objtree_VA("ALTER
> COLUMN %{column}I ADD %{identity_column}s", 2,
> +
> "type", ObjTypeString, "add identity",
> +
> "column", ObjTypeString, subcmd->name);
> +
> + attnum =
> get_attnum(RelationGetRelid(rel), subcmd->name);
> + seq_relid =
> getIdentitySequence(RelationGetRelid(rel), attnum, true);
> + seqdef =
> deparse_ColumnIdentity(seq_relid, coldef->identity, false);
> +
> + append_object_object(tmpobj,
> "identity_column", seqdef);
>
> 7.b) identity_column should be changed to "%{identity_column}s" in
> append_object_object
>
> We could use this scenario for debugging and verifying:
> CREATE TABLE itest4 (a int NOT NULL, b text);
> ALTER TABLE itest4 ALTER COLUMN a ADD GENERATED ALWAYS AS IDENTITY;

Modified
> 8) SearchSysCache1 copied twice, one of it should be removed
> + /*
> + * Lookup up object in the catalog, so we don't have to deal with
> + * current_user and such.
> + */
> +
> + tp = SearchSysCache1(USERMAPPINGOID, ObjectIdGetDatum(objectId));
> + if (!HeapTupleIsValid(tp))
> + elog(ERROR, "cache lookup failed for user mapping %u",
> objectId);
> +
> + form = (Form_pg_user_mapping) GETSTRUCT(tp);
> +
> + /*
> + * Lookup up object in the catalog, so we don't have to deal with
> + * current_user and such.
> + */
> +
> + tp = SearchSysCache1(USERMAPPINGOID, ObjectIdGetDatum(objectId));
> + if (!HeapTupleIsValid(tp))
> + elog(ERROR, "cache lookup failed for user mapping %u",
> objectId);

Modified

> 9) Create table with INCLUDING GENERATED not handled:
> + case AT_DropIdentity:
> + tmpobj = new_objtree_VA("ALTER COLUMN
> %{column}I DROP IDENTITY", 2,
> +
> "type", ObjTypeString, "drop identity",
> +
> "column", ObjTypeString, subcmd->name);
> +
> + append_string_object(tmpobj,
> "%{if_not_exists}s",
> +
> subcmd->missing_ok ? "IF EXISTS" : "");
> +
> + subcmds = lappend(subcmds,
> new_object_object(tmpobj));
> + break;
> + default:
> + elog(WARNING, "unsupported alter table
> subtype %d",
> + subcmd->subtype);
> + break;
>
> We could use this scenario for debugging and verifying:
> CREATE TABLE gtest28a (a int, b int, c int, x int GENERATED ALWAYS
> AS (b * 2) STORED);
> CREATE TABLE gtest28b (LIKE gtest28a INCLUDING GENERATED);

Modified

The attached v36 patch has the changes for the same.

Regards,
Vignesh