Re: Support logical replication of DDLs

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"

~

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.

~

101.

+ /* add an OPTIONS clause, if any */

Uppercase comment.

------

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"

~

102b.
Unnecessary blank line.

~

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.

~

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

Uppercase comment

------

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.

~

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

------

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.

~

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

~

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

------

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

~

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.

------

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'
would be better here.

~

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

------

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".

~

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)

------

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.

------

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?

------

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.

~

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

~

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.

~

116.

relation_close(pg_policy, AccessShareLock);

}
break;

case OBJECT_ATTRIBUTE:

Spurious blank line before the }

~

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.

~

118.

+ relation_close(relation, AccessShareLock);
+
+ break;
+ case OBJECT_FUNCTION:

The misplaced blank line should be *after* the break; not before it.

~

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).

~

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'

~

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

------

121. deparse_AlterDependStmt

+deparse_AlterDependStmt(Oid objectId, Node *parsetree)
+{
+ AlterObjectDependsStmt *node = (AlterObjectDependsStmt *) parsetree;
+ ObjTree *alterDependeStmt = NULL;
+
+
+ if (node->objectType == OBJECT_INDEX)

Double blank lines?

~

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

~

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.

~

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.

------

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.

~

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

~

125c.
The function parameter *parent is unused.

------

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.

~

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.

~

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

~

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

~

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

------

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.

~

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

130a.
Uppercase comment

~

130b.
Use OidIsValid macro.

~

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.

~

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 ? "," : "(");

~

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)
"," : "(");

~

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

SUGGESTION
append_format_string(createTransform, ")");

~

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.

~

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

------

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.

~

136.

+ top = new_objtree_VA("", 0);

Don't use VA() for 0 args.

~

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

~

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

~

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

------

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.

------

139. deparse_CommentStmt

+
+static ObjTree *
+deparse_CommentStmt(ObjectAddress address, Node *parsetree)

Missing function comment.

~

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.

------

141. deparse_CreateAmStmt

+
+static ObjTree *
+deparse_CreateAmStmt(Oid objectId, Node *parsetree)

Missing function comment.

~

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

------

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.

~

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.

------

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

------

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.

~

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

------
[3] CREATE TRANSFORM -
https://www.postgresql.org/docs/current/sql-createtransform.html

Kind Regards,
Peter Smith.
Fujitsu Australia