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

On Thu, 27 Oct 2022 at 16:02, vignesh C <vignesh21@gmail.com> wrote:
>
> 2) This function should handle "alter procedure" too:
> +/*
> + * Deparse an AlterFunctionStmt (ALTER FUNCTION/ROUTINE)
> + *
> + * Given a function OID and the parse tree that created it, return the JSON
> + * blob representing the alter command.
> + */
> +static ObjTree *
> +deparse_AlterFunction(Oid objectId, Node *parsetree)
> +{
> +       AlterFunctionStmt *node = (AlterFunctionStmt *) parsetree;
> +       ObjTree    *alterFunc;
> +       ObjTree    *sign;
> +       HeapTuple       procTup;
>
> Currently "alter procedure" statement are replicated as "alter
> function" statements in the subscriber.

Fixed this.

> 3) In few of the extensions we execute "alter operator family" like in
> hstore extension, we should exclude replicating "alter operator
> family" when create extension is in progress:
>   /* Don't deparse SQL commands generated while creating extension */
>   if (cmd->in_extension)
>     return NULL;
>
> The above check should be included in the below code, else the create
> extension statment will fail as internal statements will be executed:
>
> +static ObjTree *
> +deparse_AlterOpFamily(CollectedCommand *cmd)
> +{
> +       ObjTree    *alterOpFam;
> +       AlterOpFamilyStmt *stmt = (AlterOpFamilyStmt *) cmd->parsetree;
> +       HeapTuple       ftp;
> +       Form_pg_opfamily opfForm;
> +       List       *list;
> +       ListCell   *cell;
> +
> +       ftp = SearchSysCache1(OPFAMILYOID,
> +
> ObjectIdGetDatum(cmd->d.opfam.address.objectId));
> +       if (!HeapTupleIsValid(ftp))
> +               elog(ERROR, "cache lookup failed for operator family %u",
> +                        cmd->d.opfam.address.objectId);
> +       opfForm = (Form_pg_opfamily) GETSTRUCT(ftp);
> +

Fixed this.

> 4) This if...else can be removed, the nspid and typname can be handled
> for others in default. *nspid can be set to InvalidOid at the
> beginning.
> +       if (type_oid == INTERVALOID ||
> +               type_oid == TIMESTAMPOID ||
> +               type_oid == TIMESTAMPTZOID ||
> +               type_oid == TIMEOID ||
> +               type_oid == TIMETZOID)
> +       {
> +               switch (type_oid)
> +               {
> +                       case INTERVALOID:
> +                               *typname = pstrdup("INTERVAL");
> +                               break;
> +                       case TIMESTAMPTZOID:
> +                               if (typemod < 0)
> +                                       *typname = pstrdup("TIMESTAMP
> WITH TIME ZONE");
> +                               else
> +                                       /* otherwise, WITH TZ is added
> by typmod. */
> +                                       *typname = pstrdup("TIMESTAMP");
> +                               break;
> +                       case TIMESTAMPOID:
> +                               *typname = pstrdup("TIMESTAMP");
> +                               break;
> +                       case TIMETZOID:
> +                               if (typemod < 0)
> +                                       *typname = pstrdup("TIME WITH
> TIME ZONE");
> +                               else
> +                                       /* otherwise, WITH TZ is added
> by typmod. */
> +                                       *typname = pstrdup("TIME");
> +                               break;
> +                       case TIMEOID:
> +                               *typname = pstrdup("TIME");
> +                               break;
> +               }
> +               *nspid = InvalidOid;
> +       }
> +       else
> +       {
> +               /*
> +                * No additional processing is required for other
> types, so get the
> +                * type name and schema directly from the catalog.
> +                */
> +               *nspid = typeform->typnamespace;
> +               *typname = pstrdup(NameStr(typeform->typname));
> +       }
>

Changed this.

> 5) The following includes are not required in ddl_deparse.c:
> #include "catalog/pg_attribute.h"
> #include "catalog/pg_class.h"
> #include "lib/ilist.h"
> #include "nodes/makefuncs.h"
> #include "nodes/parsenodes.h"
> #include "utils/memutils.h"
>

Fixed this.

> 6) Inconsistent error reporting:
> In few places elog is used and in few places ereport is used:
> +       HeapTuple       polTup = get_catalog_object_by_oid(polRel,
> Anum_pg_policy_oid, policyOid);
> +       Form_pg_policy polForm;
> +
> +       if (!HeapTupleIsValid(polTup))
> +               elog(ERROR, "cache lookup failed for policy %u", policyOid);
>
>
> +               char       *rolename;
> +
> +               roltup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleoid));
> +               if (!HeapTupleIsValid(roltup))
> +                       ereport(ERROR,
> +                                       (errcode(ERRCODE_UNDEFINED_OBJECT),
> +                                        errmsg("role with OID %u does
> not exist", roleoid)));
>
> We can try to use the same style of error reporting.
>

Changed all errors to elog

> 8) Inclusion ordering in ddl_deparse.c:
> 8.a) The following should be slightly reordered
> +#include "access/amapi.h"
> +#include "access/table.h"
> +#include "access/relation.h"
>
> 8.b) The following should be slightly reordered
> +#include "postgres.h"
> +#include "tcop/ddl_deparse.h"
> +#include "access/amapi.h"
>
> 9) In few places multi line comment can be changed to single line comment:
> 9.a)
> +       /*
> +        * Fetch the pg_class tuple of the index relation
> +        */
>
> 9.b)
> +       /*
> +        * Fetch the pg_am tuple of the index' access method
> +        */
>
> 9.c)
> +       /*
> +        * Reject unsupported case right away.
> +        */
>
> 10)  This should also specify ROUTINE in the comment
>   /*
>    * Verbose syntax
>    *
>    * ALTER FUNCTION %{signature}s %{definition: }s
>    */
>   alterFunc = new_objtree_VA(node->objtype == OBJECT_ROUTINE ?
>                  "ALTER ROUTINE" : "ALTER FUNCTION", 0);
>
> 11) This can be changed in alphabetical order(collation first and then column):
> 11.a)
>  +               case OBJECT_COLUMN:
> +                       return "COLUMN";
> +               case OBJECT_COLLATION:
> +                       return "COLLATION";
> +               case OBJECT_CONVERSION:
> +                       return "CONVERSION";
>
> 11.b) similarly here:
>     case OBJECT_FDW:
>       return "FOREIGN DATA WRAPPER";
>     case OBJECT_FOREIGN_SERVER:
>       return "SERVER";
>     case OBJECT_FOREIGN_TABLE:
>       return "FOREIGN TABLE";
>
> 11.c) similarly here:
>     case OBJECT_FUNCTION:
>       return "FUNCTION";
>     case OBJECT_ROUTINE:
>       return "ROUTINE";
>     case OBJECT_INDEX:
>       return "INDEX";
>
> 11.d) similarly here:
>     case OBJECT_OPCLASS:
>       return "OPERATOR CLASS";
>     case OBJECT_OPERATOR:
>       return "OPERATOR";
>     case OBJECT_OPFAMILY:
>       return "OPERATOR FAMILY";
>
> 11.e) similarly here:
>     case OBJECT_TRIGGER:
>       return "TRIGGER";
>     case OBJECT_TSCONFIGURATION:
>       return "TEXT SEARCH CONFIGURATION";
>
>     /*
>      * case OBJECT_TSCONFIG_MAPPING:
>      *    return "TEXT SEARCH CONFIGURATION MAPPING";
>      */
>     case OBJECT_TSDICTIONARY:
>       return "TEXT SEARCH DICTIONARY";
>     case OBJECT_TSPARSER:
>       return "TEXT SEARCH PARSER";
>     case OBJECT_TSTEMPLATE:
>       return "TEXT SEARCH TEMPLATE";
>     case OBJECT_TYPE:
>       return "TYPE";
>

Fixed this.

> 12) new_objtree can be used instead of new_objtree_VA when there is no
> arguments, one additional check can be avoided
>
> 12.a) alterFunc = new_objtree_VA(node->objtype == OBJECT_ROUTINE ?
>                  "ALTER ROUTINE" : "ALTER FUNCTION", 0);
>
> 12.b)     ObjTree    *tmpobj = new_objtree_VA("", 0);
>
> 12.c)       tmpobj = new_objtree_VA(strVal(defel->arg), 0);
>
> 12.d)       tmpobj = new_objtree_VA("ROWS", 0);
>
> 12.e)   grantStmt = new_objtree_VA(fmt, 0);
>
> 12.f)     tmp = new_objtree_VA("ALL PRIVILEGES", 0);
>
> 12.g)         tmpobj2 = new_objtree_VA("FOR ORDER BY", 0);
>
> 12.h)   composite = new_objtree_VA("CREATE TYPE", 0);
>
> 12.i)     tmp = new_objtree_VA("OPTIONS", 0);
>
> 12.j)     tmp = new_objtree_VA("NO HANDLER", 0);
>
> 12.k) .... similarly in few more places .....
>

Fixed these.

On Thu, Nov 3, 2022 at 1:51 AM vignesh C <vignesh21@gmail.com> wrote:
>
> 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);
>

Fixed this.

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

Fixed this.

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

Fixed this.

I've addressed a few comments from Vignesh, there are quite a few more
comments remaining. I will update them in my next patch.

regards,
Ajin Cherian
Fujitsu Australia