On Fri, Nov 11, 2022 at 4:09 PM Peter Smith <smithpb2...@gmail.com> wrote:
>
> On Fri, Nov 11, 2022 at 3:47 PM Peter Smith <smithpb2...@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?
~
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].
------
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.
~
52b.
The VA(0 args) should be combined with the subsequent appends to use
fewer append_XXX calls.
~
53.
Is it necessary to say append_bool_object(stmt, "present", true); ? --
I'd assumed that is the default unless it explicitly says false.
~
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
~
55.
+ tmp = new_objtree_VA("STORAGE=", 1,
+ "clause", ObjTypeString, "storage");
Missing comment above this to say /* STORAGE */
~
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.
~
56b.
The { } blocks are unnecessary
------
57. deparse_DefineStmt_TSConfig
+
+static ObjTree *
+deparse_DefineStmt_TSConfig(Oid objectId, DefineStmt *define,
+ ObjectAddress copied)
Missing function comment.
~
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?
~
59.
+ list = NIL;
+ /* COPY */
Just assign NIL when declared.
~
60.
+ if (copied.objectId != InvalidOid)
Use OidIsValid macro.
------
61. deparse_DefineStmt_TSParser
+
+static ObjTree *
+deparse_DefineStmt_TSParser(Oid objectId, DefineStmt *define)
Missing function comment.
~
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?
~
63.
+ list = NIL;
Just assign NIL when declared
~
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 */
------
65. deparse_DefineStmt_TSDictionary
+static ObjTree *
+deparse_DefineStmt_TSDictionary(Oid objectId, DefineStmt *define)
Missing function comment.
~
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?
~
67.
+ list = NIL;
Just assign NIL when declared
~
68.
+ tmp = new_objtree_VA("", 0);
Don't need VA() function for 0 args.
------
69. deparse_DefineStmt_TSTemplate
+static ObjTree *
+deparse_DefineStmt_TSTemplate(Oid objectId, DefineStmt *define)
Missing function comment.
~
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.
~
71.
+ list = NIL;
Just assign NIL when declared
~
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.
------
73. deparse_AlterTSConfigurationStmt
+static ObjTree *
+deparse_AlterTSConfigurationStmt(CollectedCommand *cmd)
Missing function comment.
~
74.
+ /* determine the format string appropriate to each subcommand */
+ switch (node->kind)
Uppercase comment
~
75.
+ tmp = new_objtree_VA("IF EXISTS", 0);
Should not use a VA() function with 0 args.
~
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?
~
77.
+ /* add further subcommand-specific elements */
Uppercase comment
~
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
~
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
~
81.
+ tmp = new_objtree_VA("", 0);
Don't use the VA() function for 0 args.
------
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.
~
82b.
This can all be simplified to one line:
relset = new_objtree(is_reset ? "RESET" : "SET");
------
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".
~
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.
------
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?
~
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/
~
87.
+ /* COLLUMNS clause */
+ if (node->into->colNames == NIL)
+ tmp = new_objtree_VA("", 1,
+ "present", ObjTypeBool, false);
+ else
87a.
Typo "COLLUMNS"
~
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);
~
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.
~
89.
+ /* WITH clause */
+ tmp = new_objtree_VA("WITH", 0);
VA() function call is not needed when there are 0 args.
~
90.
+ /* TABLESPACE clause */
+ tmp = new_objtree_VA("TABLESPACE %{tablespace}I", 0);
VA() function call nor needed when there are 0 args.
~
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.
~
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.
~
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?
------
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.
~
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);
~
95.
+ tmpobj = new_objtree_VA("FROM", 0);
VA() function call is not needed for 0 args.
~
96.
+ tmpobj = new_objtree_VA("WHEN", 0);
VA() function call is not needed for 0 args.
~
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");
~
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.
------
[2] CREATE OPERATOR -
https://www.postgresql.org/docs/current/sql-createoperator.html
Kind Regards,
Peter Smith.
Fujitsu Australia
