Re: Support logical replication of DDLs

On Fri, 11 Nov 2022 at 10:48, Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Fri, Nov 11, 2022 at 4:09 PM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > On Fri, Nov 11, 2022 at 3:47 PM Peter Smith <smithpb2250@gmail.com> wrote:
> > >
> > > Here are more review comments for the v32-0001 file ddl_deparse.c
> > >
> > > *** NOTE - my review post became too big, so I split it into smaller parts.
> >
>
> THIS IS PART 3 OF 4.
>
> =======
>
> src/backend/commands/ddl_deparse.c
>
> 50. deparse_DefineStmt_Operator
>
> +/*
> + * Deparse a DefineStmt (CREATE OPERATOR)
> + *
> + * Given a trigger OID and the parse tree that created it, return an ObjTree
> + * representing the creation command.
> + */
> +static ObjTree *
> +deparse_DefineStmt_Operator(Oid objectId, DefineStmt *define)
>
> "trigger OID" ?? Is that right?

Modified

>
> 51.
>
>     /* MERGES */
>     tmpobj = new_objtree_VA("MERGES", 1,
>                             "clause", ObjTypeString, "merges");
>     if (!oprForm->oprcanmerge)
>         append_bool_object(tmpobj, "present", false);
>     list = lappend(list, new_object_object(tmpobj));
>
>     /* HASHES */
>     tmpobj = new_objtree_VA("HASHES", 1,
>                             "clause", ObjTypeString, "hashes");
>     if (!oprForm->oprcanhash)
>         append_bool_object(tmpobj, "present", false);
>     list = lappend(list, new_object_object(tmpobj));
>
>
> Maybe HASHES and MERGES should be done in a different order, just to
> be consistent with the PG documentation [2].

Modified

> 52. deparse_DefineStmt_Type
>
> + /* Shortcut processing for shell types. */
> + if (!typForm->typisdefined)
> + {
> + stmt = new_objtree_VA("CREATE TYPE", 0);
> + append_object_object(stmt, "%{identity}D",
> + new_objtree_for_qualname(typForm->typnamespace,
> +   NameStr(typForm->typname)));
> + append_bool_object(stmt, "present", true);
> + ReleaseSysCache(typTup);
> + return stmt;
> + }
> +
> + stmt = new_objtree_VA("CREATE TYPE", 0);
> + append_object_object(stmt, "%{identity}D",
> + new_objtree_for_qualname(typForm->typnamespace,
> +   NameStr(typForm->typname)));
> + append_bool_object(stmt, "present", true);
>
> 52a.
> This code looked strange because everything is the same except the
> Release/return, so IMO it should be refactored to use the common code.

Modified to remove duplicate code

> 52b.
> The VA(0 args) should be combined with the subsequent appends to use
> fewer append_XXX calls.

Modified

> 53.
> Is it necessary to say append_bool_object(stmt, "present", true); ? --
> I'd assumed that is the default unless it explicitly says false.

Removed it.

> 54.
>
>     /* INPUT */
>     tmp = new_objtree_VA("(INPUT=", 1,
>                          "clause", ObjTypeString, "input");
>     append_object_object(tmp, "%{procedure}D",
>                          new_objtree_for_qualname_id(ProcedureRelationId,
>                                                      typForm->typinput));
>     list = lappend(list, new_object_object(tmp));
>
>     /* OUTPUT */
>     tmp = new_objtree_VA("OUTPUT=", 1,
>                          "clause", ObjTypeString, "output");
>     append_object_object(tmp, "%{procedure}D",
>                          new_objtree_for_qualname_id(ProcedureRelationId,
>                                                      typForm->typoutput));
>     list = lappend(list, new_object_object(tmp));
>
> These could each be simplified into single VA() function calls, the
> same as was done in deparse_DefineStmt_Operator PROCEDURE.
>
> And the same comment applies to other parts. e.g.:
> - /* CATEGORY */
> - /* ALIGNMENT */
> - STORAGE

Modified

> 55.
>
> + tmp = new_objtree_VA("STORAGE=", 1,
> + "clause", ObjTypeString, "storage");
>
> Missing comment above this to say /* STORAGE */

Modified

> 56.
>
> + /* INTERNALLENGTH */
> + if (typForm->typlen == -1)
> + {
> + tmp = new_objtree_VA("INTERNALLENGTH=VARIABLE", 0);
> + }
> + else
> + {
> + tmp = new_objtree_VA("INTERNALLENGTH=%{typlen}n", 1,
> + "typlen", ObjTypeInteger, typForm->typlen);
> + }
>
> 56a.
> The VA(args = 0) does not need to be a VA function.

This is already changed as part of another comment fix

> 56b.
> The { } blocks are unnecessary

Modified

> 57. deparse_DefineStmt_TSConfig
>
> +
> +static ObjTree *
> +deparse_DefineStmt_TSConfig(Oid objectId, DefineStmt *define,
> + ObjectAddress copied)
>
> Missing function comment.

Modified

> 58.
>
> + stmt = new_objtree("CREATE");
> +
> + append_object_object(stmt, "TEXT SEARCH CONFIGURATION %{identity}D",
> + new_objtree_for_qualname(tscForm->cfgnamespace,
> +   NameStr(tscForm->cfgname)));
>
> Why not combine these using VA() function?

Modified

> 59.
>
> + list = NIL;
> + /* COPY */
>
> Just assign NIL when declared.

Modified

> 60.
>
> + if (copied.objectId != InvalidOid)
>
> Use OidIsValid macro.

Modified

> 61. deparse_DefineStmt_TSParser
>
> +
> +static ObjTree *
> +deparse_DefineStmt_TSParser(Oid objectId, DefineStmt *define)
>
> Missing function comment.

Modified

> 62.
>
> + stmt = new_objtree("CREATE");
> +
> + append_object_object(stmt, "TEXT SEARCH PARSER %{identity}D",
> + new_objtree_for_qualname(tspForm->prsnamespace,
> +   NameStr(tspForm->prsname)));
>
> Why not combine as a single VA() function call?

Modified

> 63.
>
> + list = NIL;
>
> Just assign NIL when declared

Modified

> 64.
>
>     tmp = new_objtree_VA("START=", 1,
>                          "clause", ObjTypeString, "start");
>     append_object_object(tmp, "%{procedure}D",
>                          new_objtree_for_qualname_id(ProcedureRelationId,
>                                                      tspForm->prsstart));
>
>
> Easily combined to be a single VA() function call.
>
> The same comment applies for
> -  /* GETTOKEN */
> - /* END */
> - /* LEXTYPES */

Modified

> 65. deparse_DefineStmt_TSDictionary
>
> +static ObjTree *
> +deparse_DefineStmt_TSDictionary(Oid objectId, DefineStmt *define)
>
> Missing function comment.

Modified

> 66.
>
> + stmt = new_objtree("CREATE");
> +
> + append_object_object(stmt, "TEXT SEARCH DICTIONARY %{identity}D",
> + new_objtree_for_qualname(tsdForm->dictnamespace,
> +   NameStr(tsdForm->dictname)));
>
> Why not combine this as a single VA() function call?

Modified

> 67.
>
> + list = NIL;
>
> Just assign NIL when declared

Modified

> 68.
>
> + tmp = new_objtree_VA("", 0);
>
> Don't need VA() function for 0 args.

Modified

> 69. deparse_DefineStmt_TSTemplate
>
> +static ObjTree *
> +deparse_DefineStmt_TSTemplate(Oid objectId, DefineStmt *define)
>
> Missing function comment.

Modified

> 70.
>
> + stmt = new_objtree("CREATE");
> +
> + append_object_object(stmt, "TEXT SEARCH TEMPLATE %{identity}D",
> + new_objtree_for_qualname(tstForm->tmplnamespace,
> +   NameStr(tstForm->tmplname)));
>
> Combine this to be a single VA() function call.

Modified

> 71.
>
> + list = NIL;
>
> Just assign NIL when declared

Modified

> 72.
>
> + tmp = new_objtree_VA("LEXIZE=", 1,
> + "clause", ObjTypeString, "lexize");
> + append_object_object(tmp, "%{procedure}D",
> + new_objtree_for_qualname_id(ProcedureRelationId,
> + tstForm->tmpllexize));
>
> Combine this to be a single VA() function call.

Modified

> 73. deparse_AlterTSConfigurationStmt
>
> +static ObjTree *
> +deparse_AlterTSConfigurationStmt(CollectedCommand *cmd)
>
> Missing function comment.

Modified

> 74.
>
> + /* determine the format string appropriate to each subcommand */
> + switch (node->kind)
>
> Uppercase comment

Modified

> 75.
>
> + tmp = new_objtree_VA("IF EXISTS", 0);
>
> Should not use a VA() function with 0 args.

Modified

> 76.
>
> + case ALTER_TSCONFIG_ALTER_MAPPING_FOR_TOKEN:
> + append_object_object(config, "%{identity}D ALTER MAPPING",
> + new_objtree_for_qualname_id(cmd->d.atscfg.address.classId,
> + cmd->d.atscfg.address.objectId));
> + break;
> +
> + case ALTER_TSCONFIG_REPLACE_DICT:
> + append_object_object(config, "%{identity}D ALTER MAPPING",
> + new_objtree_for_qualname_id(cmd->d.atscfg.address.classId,
> + cmd->d.atscfg.address.objectId));
> + break;
> +
> + case ALTER_TSCONFIG_REPLACE_DICT_FOR_TOKEN:
> + append_object_object(config, "%{identity}D ALTER MAPPING",
> + new_objtree_for_qualname_id(cmd->d.atscfg.address.classId,
> + cmd->d.atscfg.address.objectId));
> + break;
>
> If all these 3 cases have identical code then why repeat it three times?

Modified

> 77.
>
> + /* add further subcommand-specific elements */
>
> Uppercase comment

Modified

> 78.
>
> + /* the REPLACE forms want old and new dictionaries */
> + Assert(cmd->d.atscfg.ndicts == 2);
>
> Uppercase comment.
>
> ------
>
> 79. deparse_AlterTSDictionaryStmt
>
> +
> +static ObjTree *
> +deparse_AlterTSDictionaryStmt(Oid objectId, Node *parsetree)
>
> Missing function comment

Modified

> 80.
>
> + alterTSD = new_objtree("ALTER TEXT SEARCH DICTIONARY");
> +
> + append_object_object(alterTSD, "%{identity}D",
> + new_objtree_for_qualname(tsdForm->dictnamespace,
> +   NameStr(tsdForm->dictname)));
>
> Combine this as a sing VA() function call

Modified

> 81.
>
> + tmp = new_objtree_VA("", 0);
>
> Don't use the VA() function for 0 args.

Modified

> 82. deparse_RelSetOptions
>
> + if (is_reset)
> + fmt = "RESET ";
> + else
> + fmt = "SET ";
> +
> + relset = new_objtree(fmt);
>
> 82a.
> Those format trailing spaces are a bit unusual. The append_XXX will
> take care of space separators anyhow so it is not needed like this.

This code will get removed because of your next comment

> 82b.
> This can all be simplified to one line:
>
> relset = new_objtree(is_reset ? "RESET" : "SET");

Modified

> 83. deparse_ViewStmt
>
> + * Given a view OID and the parsetree that created it, return an ObjTree
> + * representing the creation command.
> + */
>
> Be consistent with other function headers:
>
> "parsetree" -> "parse tree".

Modified

> 84.
>
> + viewStmt = new_objtree("CREATE");
> +
> + append_string_object(viewStmt, "%{or_replace}s",
> + node->replace ? "OR REPLACE" : "");
> +
> + append_string_object(viewStmt, "%{persistence}s",
> + get_persistence_str(relation->rd_rel->relpersistence));
> +
> + tmp = new_objtree_for_qualname(relation->rd_rel->relnamespace,
> +    RelationGetRelationName(relation));
> +
> + append_object_object(viewStmt, "VIEW %{identity}D", tmp);
> +
> + append_string_object(viewStmt, "AS %{query}s",
> + pg_get_viewdef_internal(objectId));
>
> IMO all of this can be combined in a single VA() function call.

Modified

> 85. deparse_CreateTableAsStmt_vanilla
>
> +/*
> + * Deparse CREATE Materialized View statement, it is a variant of
> CreateTableAsStmt
> + *
> + * Note that CREATE TABLE AS SELECT INTO can also be deparsed by
> + * deparse_CreateTableAsStmt to remove the SELECT INTO clause.
> + */
> +static ObjTree *
> +deparse_CreateTableAsStmt_vanilla(Oid objectId, Node *parsetree)
>
> The function comment refers to 'deparse_CreateTableAsStmt' but I don't
> see any such function. Maybe this was renamed causing the comment
> became stale?

deparse_CreateTableAsStmt is present in ddl_deparse.c file, it is
required to handle SCT_CreateTableAs case.

> 86.
>
> + /* Add identity */
> + append_object_object(createStmt, "%{identity}D",
> + new_objtree_for_qualname_id(RelationRelationId,
> + objectId));
>
> This could be included as another arg of the preceding VA() call/

Modified

> 87.
>
> + /* COLLUMNS clause */
> + if (node->into->colNames == NIL)
> + tmp = new_objtree_VA("", 1,
> + "present", ObjTypeBool, false);
> + else
>
> 87a.
> Typo "COLLUMNS"

Modified

> 87b.
> It might be more usual/natural to reverse this if/else to check the
> list is NOT empty. e.g.
>
> if (node->into->colNames)
> ...
> else
> tmp = new_objtree_VA("", 1,
> "present", ObjTypeBool, false);

Modified

> 88.
>
> + tmp = new_objtree("USING");
> + if (node->into->accessMethod)
> + append_string_object(tmp, "%{access_method}I", node->into->accessMethod);
> + else
> + {
> + append_null_object(tmp, "%{access_method}I");
> + append_bool_object(tmp, "present", false);
> + }
>
> I'm not sure why a null object is necessary when present = false.

This code was intended to generate a verbose json node for "USING
accessmethod". So that user can easily modify the command by changing
the value of access_method to generate a new ddl command with access
method specified. I'm retaining this for now.

> 89.
>
> + /* WITH clause */
> + tmp = new_objtree_VA("WITH", 0);
>
> VA() function call is not needed when there are 0 args.

This is already fixed.

> 90.
>
> + /* TABLESPACE clause */
> + tmp = new_objtree_VA("TABLESPACE %{tablespace}I", 0);
>
> VA() function call nor needed when there are 0 args.

This is already fixed

> 91.
>
> + else
> + {
> + append_null_object(tmp, "%{tablespace}I");
> + append_bool_object(tmp, "present", false);
> + }
>
> I'm not sure why a null object is necessary when present = false.

This code was intended to generate a verbose json node for "USING
tablespace". So that user can easily modify the command by changing
the value of tablespace to generate a new ddl command with tablespace
specified.

> 92.
>
> + /* add a WITH NO DATA clause */
> + tmp = new_objtree_VA("WITH NO DATA", 1,
> + "present", ObjTypeBool,
> + node->into->skipData ? true : false);
> + append_object_object(createStmt, "%{with_no_data}s", tmp);
>
> 92a.
> Uppercase comment.

Modified

> 92b.
> It is a bit confusing that this style of specifying empty tree (just
> saying present/not present) is used here. But elsewhere in this patch
> for similar syntax it just adds text or an empty string.
> e.g.
> + append_string_object(renameStmt, "%{if_exists}s",
> + node->missing_ok ? "IF EXISTS" : "");
>
> IMO it's better to apply a consistent deparse approach for everything.
> But without documentation of the deparse structure, it is kind of
> impossible to know even what *are* the rules?

 I have not handled this comment, I felt we can take this comment
separately along with the documentation of deparsing.

> 93. deparse_CreateTrigStmt
>
> + trigger = new_objtree("CREATE");
> +
> + append_string_object(trigger, "%{constraint}s",
> + node->isconstraint ? "CONSTRAINT" : "");
> +
> + append_string_object(trigger, "TRIGGER %{name}I", node->trigname);
>
> All this can be combined into a single VA() call.

Modified

> 94.
>
> + if (node->timing == TRIGGER_TYPE_BEFORE)
> + append_string_object(trigger, "%{time}s", "BEFORE");
> + else if (node->timing == TRIGGER_TYPE_AFTER)
> + append_string_object(trigger, "%{time}s", "AFTER");
> + else if (node->timing == TRIGGER_TYPE_INSTEAD)
> + append_string_object(trigger, "%{time}s", "INSTEAD OF");
> + else
> + elog(ERROR, "unrecognized trigger timing type %d", node->timing);
>
> It might be better to assign the value to a char* and then just have
> only a single append_string_object() call.
>
> char *tval =
> node->timing == TRIGGER_TYPE_BEFORE ? "BEFORE" :
> node->timing == TRIGGER_TYPE_AFTER ? "AFTER" :
> node->timing == TRIGGER_TYPE_INSTEAD ? "INSTEAD OF" :
> NULL;
> if (tval == NULL)
> elog(ERROR, "unrecognized trigger timing type %d", node->timing);
> append_string_object(trigger, "%{time}s", tval);

Modified

> 95.
>
> + tmpobj = new_objtree_VA("FROM", 0);
>
> VA() function call is not needed for 0 args.

This is fixed already as part of another comment fix

> 96.
>
> + tmpobj = new_objtree_VA("WHEN", 0);
>
> VA() function call is not needed for 0 args.

This is fixed already as part of another comment fix

> 97.
>
> Should use consistent wording for unexpected nulls.
>
> e.g.1
> + if (isnull)
> + elog(ERROR, "bogus NULL tgqual");
>
> e.g.2
> + if (isnull)
> + elog(ERROR, "invalid NULL tgargs");

Modified to keep it consistent

> 98.
>
> + append_format_string(tmpobj, "(");
> + append_array_object(tmpobj, "%{args:, }L", list); /* might be NIL */
> + append_format_string(tmpobj, ")");
>
> IMO probably that can be a single call to append_array_object which
> includes the enclosing parens.

We cannot change it to a single call because in some cases there is a
possibility that the list can be NULL, if list is empty then
append_array_object will return without appending "(". For empty list,
we should append it with (). I'm not making any change for this.

Thanks for the comments, the attached v37 patch has the changes for the same.

Regards,
Vignesh