Re: Support logical replication of DDLs

On Fri, 11 Nov 2022 at 11:03, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Fri, Nov 11, 2022 at 4:17 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > On Fri, Nov 11, 2022 at 4:09 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > >
> > > On Fri, Nov 11, 2022 at 3:47 PM Peter Smith <smithpb2250(at)gmail(dot)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 4 OF 4.
>
> =======
>
> src/backend/commands/ddl_deparse.c
>
> 99. deparse_CreateUserMappingStmt
>
> + /*
> + * Lookup up object in the catalog, so we don't have to deal with
> + * current_user and such.
> + */
>
> Typo "Lookup up"

Modified

> 100.
>
> + createStmt = new_objtree("CREATE USER MAPPING ");
> +
> + append_object_object(createStmt, "FOR %{role}R",
> new_objtree_for_role_id(form->umuser));
> +
> + append_string_object(createStmt, "SERVER %{server}I", server->servername);
>
> All this can be combined into a single VA() function call.

Modified

> 101.
>
> + /* add an OPTIONS clause, if any */
>
> Uppercase comment.

Modified

> 102. deparse_AlterUserMappingStmt
>
> + /*
> + * Lookup up object in the catalog, so we don't have to deal with
> + * current_user and such.
> + */
> +
> + tp = SearchSysCache1(USERMAPPINGOID, ObjectIdGetDatum(objectId));
>
> 102a.
> Typo "Lookup up"

Modified

> 102b.
> Unnecessary blank line.

Modified

> 103.
>
> + alterStmt = new_objtree("ALTER USER MAPPING");
> +
> + append_object_object(alterStmt, "FOR %{role}R",
> new_objtree_for_role_id(form->umuser));
> +
> + append_string_object(alterStmt, "SERVER %{server}I", server->servername);
>
> Can be combined into a single VA() function call.

Modified

> 104.
> + /* add an OPTIONS clause, if any */
>
> Uppercase comment

Modified

> 105. deparse_AlterStatsStmt
>
> + alterStat = new_objtree("ALTER STATISTICS");
> +
> + /* Lookup up object in the catalog */
> + tp = SearchSysCache1(STATEXTOID, ObjectIdGetDatum(objectId));
> + if (!HeapTupleIsValid(tp))
> + elog(ERROR, "cache lookup failed for statistic %u", objectId);
> +
> + statform = (Form_pg_statistic_ext) GETSTRUCT(tp);
> +
> + append_object_object(alterStat, "%{identity}D",
> + new_objtree_for_qualname(statform->stxnamespace,
> + NameStr(statform->stxname)));
> +
> + append_float_object(alterStat, "SET STATISTICS %{target}n",
> node->stxstattarget);
>
> 105a.
> This was a biff unusual to put the new_objtree even before the catalog lookup.

Modified

> 105b.
> All new_objtreee and append_XXX can be combined as a single VA()
> function call here.

Modified

> 106. deparse_RefreshMatViewStmt
>
> + refreshStmt = new_objtree_VA("REFRESH MATERIALIZED VIEW", 0);
> +
> + /* Add a CONCURRENTLY clause */
> + append_string_object(refreshStmt, "%{concurrently}s",
> + node->concurrent ? "CONCURRENTLY" : "");
> + /* Add the matview name */
> + append_object_object(refreshStmt, "%{identity}D",
> + new_objtree_for_qualname_id(RelationRelationId,
> + objectId));
> + /* Add a WITH NO DATA clause */
> + tmp = new_objtree_VA("WITH NO DATA", 1,
> + "present", ObjTypeBool,
> + node->skipData ? true : false);
> + append_object_object(refreshStmt, "%{with_no_data}s", tmp);
>
> 106a.
> Don't use VA() function for 0 args.

This has been fixed already

> 106b.
> Huh? There are 2 different implementation styles here for the optional clauses
> - CONCURRENTLY just replaces with an empty string
> - WITH NOT DATA - has a new ObjTree either present/not present

I have not made any changes for this, we can handle together when we
are taking care of present/not present consistency across all

> 106c.
> Most/all of this can be combined into a single VA call.

Modified

> 107. deparse_DefElem
>
> + set = new_objtree("");
> + optname = new_objtree("");
> +
> + if (elem->defnamespace != NULL)
> + append_string_object(optname, "%{schema}I.", elem->defnamespace);
> +
> + append_string_object(optname, "%{label}I", elem->defname);
> +
> + append_object_object(set, "%{label}s", optname);
>
> The set should be created *after* the optname, then it can be done
> something like:
>
> set = new_objtree_VA("%{label}s", 1, "label", OptTyeString, optname);

Modified

> 108.
>
> + append_string_object(set, " = %{value}L",
> + elem->arg ? defGetString(elem) :
> + defGetBoolean(elem) ? "TRUE" : "FALSE");
>
> The calling code does not need to prefix the format with spaces like
> this. The append_XXX will handle space separators automatically.

Modified

> 109. deparse_drop_command
>
> + stmt = new_objtree_VA(fmt, 1, "objidentity", ObjTypeString, identity);
> + stmt2 = new_objtree_VA("CASCADE", 1,
> + "present", ObjTypeBool, behavior == DROP_CASCADE);
> +
> + append_object_object(stmt, "%{cascade}s", stmt2);
>
> 109a.
> 'stmt2' is a poor name. "CASCADE" is not a statement. Even 'tmpobj'

Modified

> 109b.
> The 2 lines of cascade should be grouped together -- i.e. the blank
> line should be *above* the "CASCADE", not below it.

Modified

> 110. deparse_FunctionSet
>
> + obj = new_objtree("RESET");
> + append_string_object(obj, "%{set_name}I", name);
>
> This can be combined as a single VA() call with a format "RESET %{set_name}I".

Modified

> 111.
>
> + if (kind == VAR_RESET_ALL)
> + {
> + obj = new_objtree("RESET ALL");
> + }
> + else if (value != NULL)
>
>
> It seems a bit strange that the decision is judged sometimes by the
> *value*. Why isn’t this just deciding according to different
> VariableSetKind (e.g. VAR_SET_VALUE)

Modified

> 112. deparse_IndexStmt
>
> + indexStmt = new_objtree("CREATE");
> +
> + append_string_object(indexStmt, "%{unique}s",
> + node->unique ? "UNIQUE" : "");
> +
> + append_format_string(indexStmt, "INDEX");
> +
> + append_string_object(indexStmt, "%{concurrently}s",
> + node->concurrent ? "CONCURRENTLY" : "");
> +
> + append_string_object(indexStmt, "%{if_not_exists}s",
> + node->if_not_exists ? "IF NOT EXISTS" : "");
> +
> + append_string_object(indexStmt, "%{name}I",
> + RelationGetRelationName(idxrel));
> +
> + append_object_object(indexStmt, "ON %{table}D",
> + new_objtree_for_qualname(heaprel->rd_rel->relnamespace,
> + RelationGetRelationName(heaprel)));
> +
> + append_string_object(indexStmt, "USING %{index_am}s", index_am);
> +
> + append_string_object(indexStmt, "(%{definition}s)", definition);
>
> This could all be combined to a single VA() function call.

Modified

> 113. deparse_OnCommitClause
>
> + case ONCOMMIT_NOOP:
> + append_null_object(oncommit, "%{on_commit_value}s");
> + append_bool_object(oncommit, "present", false);
> + break;
>
> Why is the null object necessary when the entire "ON COMMIT" is present=false?

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

> 114. deparse_RenameStmt
>
> + renameStmt = new_objtree_VA(fmtstr, 0);
> + append_string_object(renameStmt, "%{if_exists}s",
> + node->missing_ok ? "IF EXISTS" : "");
> + append_object_object(renameStmt, "%{identity}D",
> + new_objtree_for_qualname(schemaId,
> + node->relation->relname));
> + append_string_object(renameStmt, "RENAME TO %{newname}I",
> + node->newname);
>
> 114a.
> Don't use VA() for 0 args.

This was already fixed.

> 114b.
> Anyway, all these can be combined to a single new_objtree_VA() call.

Modified

> 115.
>
> + renameStmt = new_objtree_VA("ALTER POLICY", 0);
> + append_string_object(renameStmt, "%{if_exists}s",
> + node->missing_ok ? "IF EXISTS" : "");
> + append_string_object(renameStmt, "%{policyname}I", node->subname);
> + append_object_object(renameStmt, "ON %{identity}D",
> + new_objtree_for_qualname_id(RelationRelationId,
> + polForm->polrelid));
> + append_string_object(renameStmt, "RENAME TO %{newname}I",
> + node->newname);
>
> All these can be combined into a single VA() call.

Modified

> 116.
>
> relation_close(pg_policy, AccessShareLock);
>
> }
> break;
>
> case OBJECT_ATTRIBUTE:
>
> Spurious blank line before the }

Modified

> 117.
>
> + objtype = stringify_objtype(node->relationType);
> + fmtstr = psprintf("ALTER %s", objtype);
> + renameStmt = new_objtree(fmtstr);
>
> The code seems over-complicated. All these temporary assignments are
> not really necessary.
>
> Maybe better remove the psprintf anyway, as per my general comment at
> top of this review post.

Here psprintf cannot be removed because node->relationType is not a
fixed type, variable string cannot be used in fmr argument of
new_objtree_VA. However objtype can be removed, I have removed it.

> 118.
>
> + relation_close(relation, AccessShareLock);
> +
> + break;
> + case OBJECT_FUNCTION:
>
>
> The misplaced blank line should be *after* the break; not before it.

Modified

> 119.
>
> + char *fmt;
> +
> + fmt = psprintf("ALTER %s %%{identity}D USING %%{index_method}s
> RENAME TO %%{newname}I",
> + stringify_objtype(node->renameType));
>
> Let's be consistent about the variable naming at least within the same
> function. Elsewhere was 'fmt' was 'fmtstr' so make them all the same
> (pick one).

Removed fmt variable and used the existing fmtstr variable

> 120.
>
> + objtype = stringify_objtype(node->renameType);
> + fmtstring = psprintf("ALTER %s", objtype);
> +
> + renameStmt = new_objtree_VA(fmtstring,
> + 0);
> + append_object_object(renameStmt, "%{identity}D",
> + new_objtree_for_qualname(DatumGetObjectId(objnsp),
> + strVal(llast(identity))));
> +
> + append_string_object(renameStmt, "RENAME TO %{newname}I",
> + node->newname);
>
> 120a.
> Simplify this by not going the assignment to 'objtype'

Modified

> 120b.
> All this can be combined as a single VA() call.

Modified

> 121. deparse_AlterDependStmt
>
> +deparse_AlterDependStmt(Oid objectId, Node *parsetree)
> +{
> + AlterObjectDependsStmt *node = (AlterObjectDependsStmt *) parsetree;
> + ObjTree *alterDependeStmt = NULL;
> +
> +
> + if (node->objectType == OBJECT_INDEX)
>
> Double blank lines?

Modified

> 122.
>
> + alterDependeStmt = new_objtree("ALTER INDEX");
> +
> + qualified = new_objtree_for_qualname(relation->rd_rel->relnamespace,
> + node->relation->relname);
> + append_object_object(alterDependeStmt, "%{identity}D", qualified);
>
> This could be combined into a single VA() call.
>
> In, fact everything could be if the code it refactored a bit better so
> only the assignment for 'qualified' was within the lock.
>
> SUGGESTION
>
> qualified = new_objtree_for_qualname(relation->rd_rel->relnamespace,
> node->relation->relname);
> relation_close(relation, AccessShareLock);
>
> stmt = new_objtree_VA("ALTER INDEX %{identity}D %{no}s DEPENDS
> ON EXTENSION %{newname}I", 3,
> "identity", ObjTypeObject, qualified,
> "no", ObjTypeString, node->remove ? "NO" : "",
> "newname", strVal(node->extname));

Modified

> 123.
>
> + append_string_object(alterDependeStmt, "%{NO}s",
> + node->remove ? "NO" : "");
>
> IMO it seemed more conventional for the substition marker to be
> lowercase. So this should say "%{no}s" instead.

Modified

> 124.
>
> AlterObjectDependsStmt *node = (AlterObjectDependsStmt *) parsetree;
> ObjTree *alterDependeStmt = NULL;
>
> Why 'alterDependeStmt' with the extra 'e' -- Is it a typo? Anyway, the
> name seems overkill - just 'stmt' would put be fine.

Modified

> 125. GENERAL comments for all the deparse_Seq_XXX functions
>
> Comments common for:
> - deparse_Seq_Cache
> - deparse_Seq_Cycle
> - deparse_Seq_IncrementBy
> - deparse_Seq_Maxvalue
> - deparse_Seq_Minvalue
> - deparse_Seq_OwnedBy
> - deparse_Seq_Restart
> - deparse_Seq_Startwith
>
> 125a
> Most of the deparse_Seq_XXX functions are prefixed with "SET" which is
> needed for ALTER TABLE only.
>
> e.g.
>
> if (alter_table)
> fmt = "SET %{no}s CYCLE";
> else
> fmt = "%{no}s CYCLE";
>
> IMO all these "SET" additions should be done at the point of the call
> when doing the ALTER TABLE instead of polluting all these helper
> functions. Remove the alter_table function parameter.

In this case we have to create a format string and create an object
tree, since SET is part of the format string even if we remove
alter_table, we might have to pass SET as format string in that case
or we might have to duplicate these deparse_XXX functions for alter
table case. I preferred the existing approach unless there is an
easier way.

> 125b.
> IMO it would be tidier with a blank line before the returns.

Modified

> 125c.
> The function parameter *parent is unused.

Modified

> 126. deparse_RuleStmt
>
> + ruleStmt = new_objtree("CREATE RULE");
> +
> + append_string_object(ruleStmt, "%{or_replace}s",
> + node->replace ? "OR REPLACE" : "");
> +
> + append_string_object(ruleStmt, "%{identity}I",
> + node->rulename);
> +
> + append_string_object(ruleStmt, "AS ON %{event}s",
> + node->event == CMD_SELECT ? "SELECT" :
> + node->event == CMD_UPDATE ? "UPDATE" :
> + node->event == CMD_DELETE ? "DELETE" :
> + node->event == CMD_INSERT ? "INSERT" : "XXX");
> + append_object_object(ruleStmt, "TO %{table}D",
> + new_objtree_for_qualname_id(RelationRelationId,
> + rewrForm->ev_class));
> +
> + append_string_object(ruleStmt, "DO %{instead}s",
> + node->instead ? "INSTEAD" : "ALSO");
>
> I suspect all of this can be combined to be a single VA() function call.

Modified

> 127.
>
> + append_string_object(ruleStmt, "AS ON %{event}s",
> + node->event == CMD_SELECT ? "SELECT" :
> + node->event == CMD_UPDATE ? "UPDATE" :
> + node->event == CMD_DELETE ? "DELETE" :
> + node->event == CMD_INSERT ? "INSERT" : "XXX");
>
> The bogus "XXX" looks a bit dodgy. Probably it would be better to
> assign this 'event_str' separately and Assert/Error if node->event is
> not supported.

Modified

> 128.
>
> + 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);
>
> This doesn't look right to me...
>
> 128a.
> Using VA() with 0 args

Modified

> 128b.
> Using null object to fudge substitution to "%{clause}s, is avoidable IMO

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

> 128c.
> Shouldn't there be a "%{where_clause}s" on the ruleStmt format?

Modified

> 129. deparse_CreateTransformStmt
>
> + createTransform = new_objtree("CREATE");
> +
> + append_string_object(createTransform, "%{or_replace}s",
> + node->replace ? "OR REPLACE" : "");
> + append_object_object(createTransform, "TRANSFORM FOR %{typename}D",
> + new_objtree_for_qualname_id(TypeRelationId,
> + trfForm->trftype));
> + append_string_object(createTransform, "LANGUAGE %{language}I",
> + NameStr(langForm->lanname));
>
> This can all be combined into a single VA() function.

Modified

> 130.
> + /* deparse the transform_element_list */
> + if (trfForm->trffromsql != InvalidOid)
>
> 130a.
> Uppercase comment

Modified

> 130b.
> Use OidIsValid macro.

Modified

> 131.
>
> + /*
> + * Verbose syntax
> + *
> + * CREATE %{or_replace}s TRANSFORM FOR %{typename}D LANGUAGE
> + * %{language}I ( FROM SQL WITH FUNCTION %{signature}s, TO SQL WITH
> + * FUNCTION %{signature_tof}s )
> + *
> + * OR
> + *
> + * CREATE %{or_replace}s TRANSFORM FOR %{typename}D LANGUAGE
> + * %{language}I ( TO SQL WITH FUNCTION %{signature_tof}s )
> + */
> +
>
> According to the PG DOCS [3] *either* part of FROM/TO SQL WITH
> FUNCTION are optional. So a "FROM SQL" without a "TO SQL" is also
> allowed. So the comment should say this too.

Verbose syntax has been mentioned 3 times, I felt it is not required
to mention again and again. I have retained it at the beginning and
remained the others.

> 132.
>
> There are multiple other places in this code where should use OidIsValid macro.
>
> e.g.
> + if (trfForm->trftosql != InvalidOid)
>
> e.g.
> + /* Append a ',' if trffromsql is present, else append '(' */
> + append_string_object(createTransform, "%{comma}s",
> + trfForm->trffromsql != InvalidOid ? "," : "(");

Modified

> 133.
> These strange substitutions could've just use the
> append_format_string() function I think.
>
> 133a
> + /* Append a ',' if trffromsql is present, else append '(' */
> + append_string_object(createTransform, "%{comma}s",
> + trfForm->trffromsql != InvalidOid ? "," : "(");
>
> SUGGESTION
> append_format_string(createTransform, OidIsValid( trfForm->trffromsql)
> "," : "(");

Modified

> 133b.
> + append_string_object(createTransform, "%{close_bracket}s", ")");
>
> SUGGESTION
> append_format_string(createTransform, ")");

Modified

> 134.
> + sign = new_objtree("");
> +
> + append_object_object(sign, "%{identity}D",
> + new_objtree_for_qualname(procForm->pronamespace,
> + NameStr(procForm->proname)));
> + append_array_object(sign, "(%{arguments:, }s)", params);
> +
> + append_object_object(createTransform, "TO SQL WITH FUNCTION
> %{signature_tof}s", sign);
>
> 134a.
> IIUC it's a bit clunky to parse out this whole fmt looking for a '{'
> to extract the name "signature_tof" (maybe it works but there is a lot
> of ineficiency hidden under the covers I think), when with some small
> refactoring this could be done using a VA() function passing in the
> known name.

Modified

> 134b.
> Looks like 'sign' is either a typo or very misleading name. Isn't that
> supposed to be the ObjTree for the "signature_tof"?

Changed it to signature

> 135. append_literal_or_null
>
> +static void
> +append_literal_or_null(ObjTree *mainobj, char *elemname, char *value)
>
> In other functions 'mainobj' would have been called 'parent'. I think
> parent is a better name.

Modified

> 136.
>
> + top = new_objtree_VA("", 0);
>
> Don't use VA() for 0 args.

It was already fixed

> 137.
>
> + top = new_objtree_VA("", 0);
> + part = new_objtree_VA("NULL", 1,
> + "present", ObjTypeBool,
> + !value);
> + append_object_object(top, "%{null}s", part);
> + part = new_objtree_VA("", 1,
> + "present", ObjTypeBool,
> + !!value);
> + if (value)
> + append_string_object(part, "%{value}L", value);
> + append_object_object(top, "%{literal}s", part);
>
> 137a.
> Suggest to put each VA arg name/value on the same line.
> e.g.
> + part = new_objtree_VA("NULL", 1,
> + "present", ObjTypeBool, !value);

Modified

> 137b.
> The '!!' is cute but seems uncommon technique in PG sources. Maybe
> better just say value != NULL

Modified

> 137c.
> Suggest adding a blank line to separate the logic of the 2 parts.
> (e.g. above the 2nd part = new_objtree_VA).

Modified

> 138. deparse_CommentOnConstraintSmt
>
> + comment = new_objtree("COMMENT ON CONSTRAINT");
> +
> + append_string_object(comment, "%{identity}s",
> pstrdup(NameStr(constrForm->conname)));
> + append_format_string(comment, "ON");
> +
> + if (node->objtype == OBJECT_DOMCONSTRAINT)
> + append_format_string(comment, "DOMAIN");
> +
> + append_string_object(comment, "%{parentobj}s",
> + getObjectIdentity(&addr, false));
>
> This can mostly be done as a single VA() call I think.

Modified

> 139. deparse_CommentStmt
>
> +
> +static ObjTree *
> +deparse_CommentStmt(ObjectAddress address, Node *parsetree)
>
> Missing function comment.

Modified

> 140.
>
> + comment = new_objtree("COMMENT ON");
> + append_string_object(comment, "%{objtype}s", (char *)
> stringify_objtype(node->objtype));
>
> A single VA() function call can do this.

Modified

> 141. deparse_CreateAmStmt
>
> +
> +static ObjTree *
> +deparse_CreateAmStmt(Oid objectId, Node *parsetree)
>
> Missing function comment.

Modified

> 142.
>
> + CreateAm = new_objtree("CREATE ACCESS METHOD");
> + append_string_object(CreateAm, "%{identity}I",
> + NameStr(amForm->amname));
> +
> + switch (amForm->amtype)
> + {
> + case 'i':
> + amtype = "INDEX";
> + break;
> + case 't':
> + amtype = "TABLE";
> + break;
> + default:
> + elog(ERROR, "invalid type %c for access method", amForm->amtype);
> + }
> + append_string_object(CreateAm, "TYPE %{am_type}s", amtype);
> +
> + /* Add the HANDLER clause */
> + append_object_object(CreateAm, "HANDLER %{handler}D",
> + new_objtree_for_qualname_id(ProcedureRelationId,
> + amForm->amhandler));
>
> This entire thing can be done as a single VA() function call.
>
> SUGGESTION
>
> switch (amForm->amtype)
> {
> case 'i':
> amtype = "INDEX";
> break;
> case 't':
> amtype = "TABLE";
> break;
> default:
> elog(ERROR, "invalid type %c for access method", amForm->amtype);
> }
>
> createAm = new_objtree_VA("CREATE ACCESS METHOD %{identity}I TYPE
> %{am_type}s HANDLER %{handler}D", 3,
> "identity", ObjTypeString, NameStr(amForm->amname),
> "am_type", ObjTypeString, amtype,
> "handler", ObjTypeObject,
> new_objtree_for_qualname_id(ProcedureRelationId, amForm->amhandler));

Modified

> 143. deparse_simple_command
>
> + switch (nodeTag(parsetree))
> + {
> + case T_CreateSchemaStmt:
> + command = deparse_CreateSchemaStmt(objectId, parsetree);
> + break;
> +
> + case T_AlterDomainStmt:
> + command = deparse_AlterDomainStmt(objectId, parsetree,
> + cmd->d.simple.secondaryObject);
> + break;
> +
> + case T_CreateStmt:
> + command = deparse_CreateStmt(objectId, parsetree);
> + break;
> +
> + case T_RefreshMatViewStmt:
> + command = deparse_RefreshMatViewStmt(objectId, parsetree);
> + break;
> +
> + case T_CreateTrigStmt:
> + command = deparse_CreateTrigStmt(objectId, parsetree);
> + break;
> +
> + case T_RuleStmt:
> + command = deparse_RuleStmt(objectId, parsetree);
> + break;
> +
> + case T_CreatePLangStmt:
> + command = deparse_CreateLangStmt(objectId, parsetree);
> + break;
> +
> + case T_CreateSeqStmt:
> + command = deparse_CreateSeqStmt(objectId, parsetree);
> + break;
> +
> + case T_CreateFdwStmt:
> + command = deparse_CreateFdwStmt(objectId, parsetree);
> + break;
> +
> + case T_CreateUserMappingStmt:
> + command = deparse_CreateUserMappingStmt(objectId, parsetree);
> + break;
> +
> + case T_AlterUserMappingStmt:
> + command = deparse_AlterUserMappingStmt(objectId, parsetree);
> + break;
> +
> + case T_AlterStatsStmt:
> + command = deparse_AlterStatsStmt(objectId, parsetree);
> + break;
> +
> + case T_AlterFdwStmt:
> + command = deparse_AlterFdwStmt(objectId, parsetree);
> + break;
> +
> + case T_AlterSeqStmt:
> + command = deparse_AlterSeqStmt(objectId, parsetree);
> + break;
> +
> + case T_DefineStmt:
> + command = deparse_DefineStmt(objectId, parsetree,
> + cmd->d.simple.secondaryObject);
> + break;
> +
> + case T_CreateConversionStmt:
> + command = deparse_CreateConversion(objectId, parsetree);
> + break;
> +
> + case T_CreateDomainStmt:
> + command = deparse_CreateDomain(objectId, parsetree);
> + break;
> +
> + case T_CreateExtensionStmt:
> + command = deparse_CreateExtensionStmt(objectId, parsetree);
> + break;
> +
> + case T_AlterExtensionStmt:
> + command = deparse_AlterExtensionStmt(objectId, parsetree);
> + break;
> +
> + case T_AlterExtensionContentsStmt:
> + command = deparse_AlterExtensionContentsStmt(objectId, parsetree,
> + cmd->d.simple.secondaryObject);
> + break;
> +
> + case T_CreateOpFamilyStmt:
> + command = deparse_CreateOpFamily(objectId, parsetree);
> + break;
> +
> + case T_CreatePolicyStmt:
> + command = deparse_CreatePolicyStmt(objectId, parsetree);
> + break;
> +
> + case T_IndexStmt:
> + command = deparse_IndexStmt(objectId, parsetree);
> + break;
> +
> + case T_CreateFunctionStmt:
> + command = deparse_CreateFunction(objectId, parsetree);
> + break;
> +
> + case T_AlterFunctionStmt:
> + command = deparse_AlterFunction(objectId, parsetree);
> + break;
> +
> + case T_AlterCollationStmt:
> + command = deparse_AlterCollation(objectId, parsetree);
> + break;
> +
> + case T_RenameStmt:
> + command = deparse_RenameStmt(cmd->d.simple.address, parsetree);
> + break;
> +
> + case T_AlterObjectDependsStmt:
> + command = deparse_AlterDependStmt(objectId, parsetree);
> + break;
> +
> + case T_AlterObjectSchemaStmt:
> + command = deparse_AlterObjectSchemaStmt(cmd->d.simple.address,
> + parsetree,
> + cmd->d.simple.secondaryObject);
> + break;
> +
> + case T_AlterOwnerStmt:
> + command = deparse_AlterOwnerStmt(cmd->d.simple.address, parsetree);
> + break;
> +
> + case T_AlterOperatorStmt:
> + command = deparse_AlterOperatorStmt(objectId, parsetree);
> + break;
> +
> + case T_AlterPolicyStmt:
> + command = deparse_AlterPolicyStmt(objectId, parsetree);
> + break;
> +
> + case T_AlterTypeStmt:
> + command = deparse_AlterTypeSetStmt(objectId, parsetree);
> + break;
> +
> + case T_CreateStatsStmt:
> + command = deparse_CreateStatisticsStmt(objectId, parsetree);
> + break;
> +
> + case T_CreateForeignServerStmt:
> + command = deparse_CreateForeignServerStmt(objectId, parsetree);
> + break;
> +
> + case T_AlterForeignServerStmt:
> + command = deparse_AlterForeignServerStmt(objectId, parsetree);
> + break;
> +
> + case T_CompositeTypeStmt:
> + command = deparse_CompositeTypeStmt(objectId, parsetree);
> + break;
> +
> + case T_CreateEnumStmt: /* CREATE TYPE AS ENUM */
> + command = deparse_CreateEnumStmt(objectId, parsetree);
> + break;
> +
> + case T_CreateRangeStmt: /* CREATE TYPE AS RANGE */
> + command = deparse_CreateRangeStmt(objectId, parsetree);
> + break;
> +
> + case T_AlterEnumStmt:
> + command = deparse_AlterEnumStmt(objectId, parsetree);
> + break;
> +
> + case T_CreateCastStmt:
> + command = deparse_CreateCastStmt(objectId, parsetree);
> + break;
> +
> + case T_AlterTSDictionaryStmt:
> + command = deparse_AlterTSDictionaryStmt(objectId, parsetree);
> + break;
> +
> + case T_CreateTransformStmt:
> + command = deparse_CreateTransformStmt(objectId, parsetree);
> + break;
> +
> + case T_ViewStmt: /* CREATE VIEW */
> + command = deparse_ViewStmt(objectId, parsetree);
> + break;
> +
> + case T_CreateTableAsStmt: /* CREATE MATERIALIZED VIEW */
> + command = deparse_CreateTableAsStmt_vanilla(objectId, parsetree);
> + break;
> +
> + case T_CommentStmt:
> + command = deparse_CommentStmt(cmd->d.simple.address, parsetree);
> + break;
> +
> + case T_CreateAmStmt:
> + command = deparse_CreateAmStmt(objectId, parsetree);
> + break;
>
> 143a.
> Suggestion to put all these cases in alphabetical order.

Modified

> 143b.
> Suggest removing the variable 'command' and for each case just return
> the deparse_XXX result -- doing this will eliminate the need for
> "break;" and so the function can be 50 lines shorter.

Modified

> 144. deparse_TableElements
>
> + if (tree != NULL)
> + {
> + ObjElem *column;
> +
> + column = new_object_object(tree);
> + elements = lappend(elements, column);
> + }
>
> Why do all this instead of just:
>
> if (tree != NULL)
> elements = lappend(elements, new_object_object(tree));

Modified

> 145. deparse_utility_command
>
> + if (tree)
> + {
> + Jsonb *jsonb;
> +
> + jsonb = objtree_to_jsonb(tree);
> + command = JsonbToCString(&str, &jsonb->root, JSONB_ESTIMATED_LEN);
> + }
> + else
> + command = NULL;
>
> 145a.
> Since 'tree' is always assigned the result of deparse_XXX I am
> wondering if tree == NULL is even possible here? If not then this
> if/else should be an Assert instead.

This is required as the tree can be NULL like in the below case:
/*
* Indexes for PRIMARY KEY and other constraints are output
* separately; return empty here.
*/

> 145b.
> Anyway, maybe assign default command = NULL in the declaration to
> reduce a couple of lines of unnecessary code.

Modified

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

Regards,
Vignesh