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

On Wed, 2 Nov 2022 at 05:13, vignesh C <vignesh21@gmail.com> wrote:
>
> On Mon, 31 Oct 2022 at 16:17, vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Thu, 27 Oct 2022 at 16:02, vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > On Thu, 27 Oct 2022 at 02:09, Zheng Li <zhengli10@gmail.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

In case of a materialized view, if there is a possibility to optimize
the subquery, the tree will be changed accordingly. We will not be
able to get the query definition using this tree as the tree has been
changed and some of the nodes will be deleted. I have modified it so
that we make a copy of the tree before the actual execution (before
the tree is getting changed). The attached v45 patch has the changes
for the same.

Regards,
Vignesh