Re: Support logical replication of DDLs

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) Empty () should be appended in case if there are no table elements:
+ tableelts = deparse_TableElements(relation,
node->tableElts, dpcontext,
+
false, /* not typed table */
+
false); /* not composite */
+ tableelts = obtainConstraints(tableelts, objectId, InvalidOid);
+
+ append_array_object(createStmt, "(%{table_elements:,
}s)", tableelts);

This is required for:
CREATE TABLE ihighway () INHERITS (road);

2)
2.a)
Here cell2 will be of type RoleSpec, the below should be changed:
+ foreach(cell2, (List *) opt->arg)
+ {
+ String *val = lfirst_node(String, cell2);
+ ObjTree *obj =
new_objtree_for_role(strVal(val));
+
+ roles = lappend(roles, new_object_object(obj));
+ }

to:
foreach(cell2, (List *) opt->arg)
{
RoleSpec *rolespec = lfirst(cell2);
ObjTree *obj = new_objtree_for_rolespec(rolespec);

roles = lappend(roles, new_object_object(obj));
}

This change is required for:
ALTER DEFAULT PRIVILEGES FOR ROLE regress_selinto_user REVOKE INSERT
ON TABLES FROM regress_selinto_user;

2.b) After the above change the following function cna be removed:
+/*
+ * Helper routine for %{}R objects, with role specified by name.
+ */
+static ObjTree *
+new_objtree_for_role(char *rolename)
+{
+ ObjTree *role;
+
+ role = new_objtree_VA(NULL,2,
+ "is_public",
ObjTypeBool, strcmp(rolename, "public") == 0,
+ "rolename",
ObjTypeString, rolename);
+ return role;
+}

3) There was a crash in this materialized view scenario:
+ /* add the query */
+ Assert(IsA(node->query, Query));
+ append_string_object(createStmt, "AS %{query}s",
+
pg_get_querydef((Query *) node->query, false));
+
+ /* add a WITH NO DATA clause */
+ tmp = new_objtree_VA("WITH NO DATA", 1,
+ "present", ObjTypeBool,
+ node->into->skipData
? true : false);

CREATE TABLE mvtest_t (id int NOT NULL PRIMARY KEY, type text NOT
NULL, amt numeric NOT NULL);
CREATE VIEW mvtest_tv AS SELECT type, sum(amt) AS totamt FROM mvtest_t
GROUP BY type;
CREATE VIEW mvtest_tvv AS SELECT sum(totamt) AS grandtot FROM mvtest_tv;
CREATE MATERIALIZED VIEW mvtest_tvvm AS SELECT * FROM mvtest_tvv;
CREATE VIEW mvtest_tvvmv AS SELECT * FROM mvtest_tvvm;
CREATE MATERIALIZED VIEW mvtest_bb AS SELECT * FROM mvtest_tvvmv;

#0 0x0000560d45637897 in AcquireRewriteLocks (parsetree=0x0,
forExecute=false, forUpdatePushedDown=false) at rewriteHandler.c:154
#1 0x0000560d45637b93 in AcquireRewriteLocks
(parsetree=0x560d467c4778, forExecute=false,
forUpdatePushedDown=false) at rewriteHandler.c:269
#2 0x0000560d457f792a in get_query_def (query=0x560d467c4778,
buf=0x7ffeb8059bd0, parentnamespace=0x0, resultDesc=0x0,
colNamesVisible=true, prettyFlags=2, wrapColumn=0, startIndent=0) at
ruleutils.c:5566
#3 0x0000560d457ee869 in pg_get_querydef (query=0x560d467c4778,
pretty=false) at ruleutils.c:1639
#4 0x0000560d453437f6 in deparse_CreateTableAsStmt_vanilla
(objectId=24591, parsetree=0x560d467c4748) at ddl_deparse.c:7076
#5 0x0000560d45348864 in deparse_simple_command (cmd=0x560d467c3b98)
at ddl_deparse.c:9158
#6 0x0000560d45348b75 in deparse_utility_command (cmd=0x560d467c3b98,
verbose_mode=false) at ddl_deparse.c:9273
#7 0x0000560d45351627 in publication_deparse_ddl_command_end
(fcinfo=0x7ffeb8059e90) at event_trigger.c:2517
#8 0x0000560d4534eeb1 in EventTriggerInvoke
(fn_oid_list=0x560d467b5450, trigdata=0x7ffeb8059ef0) at
event_trigger.c:1082
#9 0x0000560d4534e61c in EventTriggerDDLCommandEnd
(parsetree=0x560d466e8a88) at event_trigger.c:732
#10 0x0000560d456b6ee2 in ProcessUtilitySlow (pstate=0x560d467cdee8,
pstmt=0x560d466e9a18, queryString=0x560d466e7c38 "CREATE MATERIALIZED
VIEW mvtest_bb AS SELECT * FROM mvtest_tvvmv;",
context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0,
dest=0x560d467cb5d8, qc=0x7ffeb805a6f0) at utility.c:1926

4) The following statements crashes:
BEGIN;
CREATE TABLE t (c int);
SAVEPOINT q;
CREATE RULE "_RETURN" AS ON SELECT TO t DO INSTEAD
SELECT * FROM generate_series(1,5) t0(c); -- fails due to policy p on t
ROLLBACK TO q;
CREATE RULE "_RETURN" AS ON SELECT TO t DO INSTEAD
SELECT * FROM generate_series(1,5) t0(c); -- succeeds
ROLLBACK;

#4 0x00007f3f7c8eb7b7 in __GI_abort () at abort.c:79
#5 0x0000561e569a819c in ExceptionalCondition
(conditionName=0x561e56b932f0 "rel->pgstat_info->relation == NULL",
fileName=0x561e56b932ab "pgstat_relation.c", lineNumber=142) at
assert.c:66
#6 0x0000561e567f3569 in pgstat_assoc_relation (rel=0x7f3d6cc9d4e8)
at pgstat_relation.c:142
#7 0x0000561e5628ade3 in initscan (scan=0x561e57a67648, key=0x0,
keep_startblock=false) at heapam.c:340
#8 0x0000561e5628c7be in heap_beginscan (relation=0x7f3d6cc9d4e8,
snapshot=0x561e579c4da0, nkeys=0, key=0x0, parallel_scan=0x0,
flags=449) at heapam.c:1220
#9 0x0000561e5674ff5a in table_beginscan (rel=0x7f3d6cc9d4e8,
snapshot=0x561e579c4da0, nkeys=0, key=0x0) at
../../../src/include/access/tableam.h:891
#10 0x0000561e56750fa8 in DefineQueryRewrite (rulename=0x561e57991660
"_RETURN", event_relid=40960, event_qual=0x0, event_type=CMD_SELECT,
is_instead=true, replace=false, action=0x561e57a60648)
at rewriteDefine.c:447
#11 0x0000561e567505cc in DefineRule (stmt=0x561e57991d68,
queryString=0x561e57990c38 "CREATE RULE \"_RETURN\" AS ON SELECT TO t
DO INSTEAD\n SELECT * FROM generate_series(1,5) t0(c);") at
rewriteDefine.c:213
#12 0x0000561e567d157a in ProcessUtilitySlow (pstate=0x561e579bae18,
pstmt=0x561e579920a8,
queryString=0x561e57990c38 "CREATE RULE \"_RETURN\" AS ON SELECT
TO t DO INSTEAD\n SELECT * FROM generate_series(1,5) t0(c);",
context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0,
dest=0x561e57992188, qc=0x7ffcf2482ea0) at utility.c:1657

5) Where clause should come before instead:
5.a) Where clause should come before instead:
+ append_string_object(ruleStmt, "DO %{instead}s",
+ node->instead ?
"INSTEAD" : "ALSO");
+
+ ev_qual = heap_getattr(rewrTup, Anum_pg_rewrite_ev_qual,
+
RelationGetDescr(pg_rewrite), &isnull);
+ ev_actions = heap_getattr(rewrTup, Anum_pg_rewrite_ev_action,
+
RelationGetDescr(pg_rewrite), &isnull);
+
+ pg_get_ruledef_detailed(ev_qual, ev_actions, &qual, &actions);
+
+ tmp = new_objtree_VA("WHERE %{clause}s", 0);
+
+ if (qual)
+ append_string_object(tmp, "clause", qual);
+ else
+ {
+ append_null_object(tmp, "clause");
+ append_bool_object(tmp, "present", false);
+ }
+
+ append_object_object(ruleStmt, "where_clause", tmp);

5.b) clause should be changed to %{clause}s in both places
It can be changed to:
.....
ev_qual = heap_getattr(rewrTup, Anum_pg_rewrite_ev_qual,
RelationGetDescr(pg_rewrite), &isnull);
ev_actions = heap_getattr(rewrTup, Anum_pg_rewrite_ev_action,
RelationGetDescr(pg_rewrite), &isnull);
pg_get_ruledef_detailed(ev_qual, ev_actions, &qual, &actions);
tmp = new_objtree_VA("WHERE", 0);
if (qual)
append_string_object(tmp, "%{clause}s", qual);
else
{
append_null_object(tmp, "%{clause}s");
append_bool_object(tmp, "present", false);
}

append_object_object(ruleStmt, "%{where_clause}s", tmp);

append_string_object(ruleStmt, "DO %{instead}s",
node->instead ? "INSTEAD" : "ALSO");
.....

CREATE RULE qqq AS ON INSERT TO public.copydml_test DO INSTEAD WHERE
(new.t OPERATOR(pg_catalog.<>) 'f'::pg_catalog.text) DELETE FROM
public.copydml_test

6) Rename table constraint not handled:
+/*
+ * Deparse a RenameStmt.
+ */
+static ObjTree *
+deparse_RenameStmt(ObjectAddress address, Node *parsetree)
+{
+ RenameStmt *node = (RenameStmt *) parsetree;
+ ObjTree *renameStmt;
+ char *fmtstr;
+ const char *objtype;
+ Relation relation;
+ Oid schemaId;
+

ALTER TABLE onek ADD CONSTRAINT onek_check_constraint CHECK (unique1 >= 0);
ALTER TABLE onek RENAME CONSTRAINT onek_check_constraint TO
onek_check_constraint_foo;

7) The following deparsing of index fails:
CREATE TABLE covering_index_heap (f1 int, f2 int, f3 text);
CREATE UNIQUE INDEX covering_index_index on covering_index_heap
(f1,f2) INCLUDE(f3);

8) default should be %{default}s
+deparse_CreateConversion(Oid objectId, Node *parsetree)
+{
+ HeapTuple conTup;
+ Relation convrel;
+ Form_pg_conversion conForm;
+ ObjTree *ccStmt;
+ ObjTree *tmpObj;
+
+ convrel = table_open(ConversionRelationId, AccessShareLock);
+ conTup = get_catalog_object_by_oid(convrel,
Anum_pg_conversion_oid, objectId);
+ if (!HeapTupleIsValid(conTup))
+ elog(ERROR, "cache lookup failed for conversion with
OID %u", objectId);
+ conForm = (Form_pg_conversion) GETSTRUCT(conTup);
+
+ /*
+ * Verbose syntax
+ *
+ * CREATE %{default}s CONVERSION %{identity}D FOR %{source}L TO %{dest}L
+ * FROM %{function}D
+ */
+ ccStmt = new_objtree("CREATE");
+
+
+ /* Add the DEFAULT clause */
+ append_string_object(ccStmt, "default",
+ conForm->condefault ?
"DEFAULT" : "");

9) Rename of Domain constraint not handled:
+/*
+ * Deparse a RenameStmt.
+ */
+static ObjTree *
+deparse_RenameStmt(ObjectAddress address, Node *parsetree)
+{
+ RenameStmt *node = (RenameStmt *) parsetree;
+ ObjTree *renameStmt;
+ char *fmtstr;
+ const char *objtype;
+ Relation relation;
+ Oid schemaId;
+

Regards,
Vignesh