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

On Fri, Aug 5, 2022 at 4:03 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Hou-san, here are my review comments for the patch v15-0001:
>
> ======
>
> 1. Commit Message
>
> CREATE/ALTER/DROP TABLE (*)
>
> At first, I thought "(*)" looks like a SQL syntax element.
>
> SUGGESTION:
>
> CREATE/ALTER/DROP TABLE - - Note #1, Note #2
> ...
> Note #1 – blah blah
> Note #2 – yada yada
>
> ======
fixed

>
> 2. src/backend/commands/ddl_deparse.c - General
>
> 2.1
> Lots of the deparse_XXX function are in random places scattered around
> in this module. Since there are so many, I think it's better to have
> functions arrange alphabetically to make them easier to find. (And if
> there are several functions that logically "belong" together then
> those should be re-named so they will be group together
> alphabetically...
>
> Same applies to other functions – not just the deparse_XXX ones

fixed

>
> 2.2
> There are lots of 'tmp' (or 'tmp2') variables in this file. Sometimes
> 'tmp' is appropriate (or maybe 'tmpobj' would be  better) but in other
> cases it seems like there should be a better name than 'tmp'. Please
> search all these and replace where you can use a more meaningful name
> than tmp.
>

changed to tmpobj

> 2.3
> Pointer NULL comparisons are not done consistently all through the
> file. E.g. Sometimes you do tree->fmtinfo == NULL, but in others you
> do like if (!tree->fmtinfo). It's no big deal whichever way you want
> to use, but at least for the comparisons involving the same variables
> IMO should use the same style consistently.
>
> ~~~

fixed.

>
> 3. src/backend/commands/ddl_deparse.c - format_type_detailed
>
> 3.1
> + * - typename is set to the type name, without quotes
>
> But the param is called 'typname', not 'typename'
>
> 3.2
> + * - typmod is set to the typemod, if any, as a string with parens
>
> I think you mean:
> "typmod is set" -> "typemodstr is set"
>
> 3.3
> + if (IsTrueArrayType(typeform) &&
> + typeform->typstorage != TYPSTORAGE_PLAIN)
> + {
> + /* Switch our attention to the array element type */
> + ReleaseSysCache(tuple);
> + tuple = SearchSysCache1(TYPEOID, ObjectIdGetDatum(array_base_type));
> + if (!HeapTupleIsValid(tuple))
> + elog(ERROR, "cache lookup failed for type %u", type_oid);
> +
> + typeform = (Form_pg_type) GETSTRUCT(tuple);
> + type_oid = array_base_type;
> + *typarray = true;
> + }
> + else
> + *typarray = false;
>
> Maybe this if/else can be simplified
>

fixed.

> *typarray = IsTrueArrayType(typeform) && typeform->typstorage !=
> TYPSTORAGE_PLAIN;
> if (*typarray)
> {
> ...
> }
>
> 3.4
> + /* otherwise, WITH TZ is added by typmod. */
>
> Uppercase comment
>
> ~~~

fixed.

>
> 4. src/backend/commands/ddl_deparse.c - append_object_to_format_string
>
> + for (cp = sub_fmt; cp < end_ptr; cp++)
> + {
> + if (*cp == '{')
> + {
> + start_copy = true;
> + continue;
> + }
>
> What's this logic going to do if it encounters "{{" - it looks like it
> will just keep going but wouldn't that be a name error to have a "{"
> in it?

I think this logic expects single braces.

>
> ~~~
>
> 5. src/backend/commands/ddl_deparse.c - append_bool_object
>
> +append_bool_object(ObjTree *tree, char *sub_fmt, bool value)
> +{
> + ObjElem  *param;
> + char *object_name = sub_fmt;
> + bool   is_present_flag = false;
> +
> + Assert(sub_fmt);
> +
> + if (strcmp(sub_fmt, "present") == 0)
> + {
> + is_present_flag = true;
> + tree->present = value;
> + }
> +
> + if (!verbose && !tree->present)
> + return;
> +
> + if (!is_present_flag)
> + object_name = append_object_to_format_string(tree, sub_fmt);
> +
> + param = new_object(ObjTypeBool, object_name);
> + param->value.boolean = value;
> + append_premade_object(tree, param);
> +}
>
> It feels like there is some subtle trickery going on here with the
> conditions. Is there a simpler way to write this, or maybe it is just
> lacking some explanatory comments?
>
> ~~~

Added a comment.

>
> 6. src/backend/commands/ddl_deparse.c - append_array_object
>
> + if (!verbose)
> + {
> + ListCell *lc;
> +
> + /* Extract the ObjElems whose present flag is true */
> + foreach(lc, array)
> + {
> + ObjElem *elem = (ObjElem *) lfirst(lc);
> +
> + Assert(elem->objtype == ObjTypeObject);
> +
> + if (!elem->value.object->present)
> + array = foreach_delete_current(array, lc);
> + }
> +
> + if (list_length(array) == 0)
> + return;
> + }
>
> Maybe it is OK as-is. I'm just wondering if this list_length(array)
> check should be outside of the !verbose check?
>
> ~~~

fixed.

>
> 7. src/backend/commands/ddl_deparse.c - objtree_to_jsonb_element
>
> + case ObjTypeObject:
> + /* recursively add the object into the existing parse state */
>
> Uppercase comment
>
> ~~~
>
> 8. src/backend/commands/ddl_deparse.c - new_objtree_for_qualname_id
>
> 8.1
> + *
> + * Elements "schemaname" and "objname" are set.  If the object is a temporary
> + * object, the schema name is set to "pg_temp".
>
> I'm not sure if this is the right place to say this, since it is not
> really this function that sets that "pg_temp".
>
> 8.2
> + if (isnull)
> + elog(ERROR, "unexpected NULL namespace");
> + objname = heap_getattr(catobj, Anum_name, RelationGetDescr(catalog),
> +    &isnull);
>
> Missing blank line after the elog?
>
> ~~~
>
> 9. src/backend/commands/ddl_deparse.c - deparse_ColumnIdentity
>
> + /* Definition elemets */
>
> typo "elemets"
>
> ~~~
>
> 10. src/backend/commands/ddl_deparse.c - deparse_CreateTrigStmt
>
> 10.1
> + else
> + elog(ERROR, "unrecognized trigger timing value %d", node->timing);
>
> should that say "unrecognized trigger timing type" (e.g. type instead of value)
>
> 10.2
> + /*
> + * Decode the events that the trigger fires for.  The output is a list;
> + * in most cases it will just be a string with the even name, but when
> + * there's an UPDATE with a list of columns, we return a JSON object.
> + */
>
> "even name" -> "event name" ?
>
> 10.3
> + foreach(cell, node->columns)
> + {
> + char   *colname = strVal(lfirst(cell));
> +
> + cols = lappend(cols,
> +    new_string_object(colname));
> + }
>
> Unnecessary wrapping?
>
> 10.4
> + append_array_object(update, "%{columns:, }I", cols);
> +
> + events = lappend(events,
> + new_object_object(update));
>
> Unnecessary wrapping?
>
> 10.5
> + /* verify that the argument encoding is correct */
>
> Uppercase comment
>
> ~~~

fixed all the above comments.

>
> 11. src/backend/commands/ddl_deparse.c - deparse_ColumnDef
>
> + saw_notnull = false;
> + foreach(cell, coldef->constraints)
> + {
> + Constraint *constr = (Constraint *) lfirst(cell);
> +
> + if (constr->contype == CONSTR_NOTNULL)
> + saw_notnull = true;
> + }
>
> Some similar functions here would 'break' when it finds the
> saw_notnull. Why not break here?
>
> ~~~

I think the comment explains why this "not null" is different.

>
> 12. src/backend/commands/ddl_deparse.c - obtainConstraints
>
> 12.1
> + /* only one may be valid */
>
> Uppercase comment
>
> 12.2
> + /*
> + * scan pg_constraint to fetch all constraints linked to the given
> + * relation.
> + */
>
> Uppercase comment
>
> ~~~
>
> 13. src/backend/commands/ddl_deparse.c - deparse_InhRelations
>
> +/*
> + * Deparse the inherits relations.
> + *
> + * Given a table OID, return a schema qualified table list representing
> + * the parent tables.
> + */
>
> I am not sure - should that say "Deparse the INHERITS relations." (uppercase)
>
> ~~~
>
> 14. src/backend/commands/ddl_deparse.c - pg_get_indexdef_detailed
>
> 14.1
> + * There is a huge lot of code that's a dupe of pg_get_indexdef_worker, but
> + * control flow is different enough that it doesn't seem worth keeping them
> + * together.
> + */
>
> SUGGESTION
> A large amount of code is duplicated from pg_get_indexdef_worker, ...
>
> 14.2
> + idxrelrec = (Form_pg_class) GETSTRUCT(ht_idxrel);
> +
> +
> + /*
> + * Fetch the pg_am tuple of the index' access method
> + */
>
> Remove the extra blank line
>
> ~~~
>
> 15. src/backend/commands/ddl_deparse.c - deparse_IndexStmt
>
> + /*
> + * indexes for PRIMARY KEY and other constraints are output
> + * separately; return empty here.
> + */
>
> Uppercase comment
>
> ~~~
>
> 16. src/backend/commands/ddl_deparse.c - deparse_FunctionSet
>
> 16.1
> +static ObjTree *
> +deparse_FunctionSet(VariableSetKind kind, char *name, char *value)
>
> Missing function comment.
>
> 16.2
> + ObjTree    *r;
>
> Is 'r' the best name?

fixed all the above comments.

>
> 16.3
> + if (kind == VAR_RESET_ALL)
> + {
> + r = new_objtree("RESET ALL");
> + }
> + else if (value != NULL)
> + {
> + r = new_objtree_VA("SET %{set_name}I", 1,
> +    "set_name", ObjTypeString, name);
> +
> + /*
> + * Some GUC variable names are 'LIST' type and hence must not be
> + * quoted.
> + */
> + if (GetConfigOptionFlags(name, true) & GUC_LIST_QUOTE)
> + append_string_object(r, "TO %{set_value}s", value);
> + else
> + append_string_object(r, "TO %{set_value}L", value);
> + }
> + else
> + {
> + r = new_objtree("RESET");
> + append_string_object(r, "%{set_name}I", name);
> + }
>
> It seems a bit strange that the kind of new_objtree is judged
> sometimes by the *value. Why isn't this just always switching on the
> different VariableSetKind?

checking on VariableSetKind requires to check multiple conditions,
this is a simpler comparison.
>
> ~~~
>
> 17. src/backend/commands/ddl_deparse.c - deparse_CreateFunction
>
> 17.1
> +/*
> + * deparse_CreateFunctionStmt
> + * Deparse a CreateFunctionStmt (CREATE FUNCTION)
> + *
> + * Given a function OID and the parsetree that created it, return the JSON
> + * blob representing the creation command.
> + */
> +static ObjTree *
> +deparse_CreateFunction(Oid objectId, Node *parsetree)
>
> The name of the function and the name of the function in the comment
> don't match. Suggest removing it from the comment.
>
> 17.2
> + /* get the pg_proc tuple */
>
> Uppercase comment
>
> 17.3
> + /* get the corresponding pg_language tuple */
>
> Uppercase comment
>
> 17.4
> + /* optional wholesale suppression of "name" occurs here */
>
> Uppercase comment
>
> 17.5
> + append_string_object(createFunc, "%{volatility}s",
> + procForm->provolatile == PROVOLATILE_VOLATILE ?
> + "VOLATILE" :
> + procForm->provolatile == PROVOLATILE_STABLE ?
> + "STABLE" :
> + procForm->provolatile == PROVOLATILE_IMMUTABLE ?
> + "IMMUTABLE" : "INVALID VOLATILITY");
>
> Does "INVALID VOLATILITY" make sense? Is that a real thing or should
> this give ERROR?

fixed the above comments, changed the logic here.

>
> 17.6
> + foreach(cell, node->options)
> + {
> + DefElem *defel = (DefElem *) lfirst(cell);
> + ObjTree    *tmp = NULL;
>
> Why assign *tmp = NULL? I think it serves no purpose.
>
> ~~~
>
> 18. src/backend/commands/ddl_deparse.c - deparse_AlterFunction
>
> 18.1
> + * Deparse a AlterFunctionStmt (ALTER FUNCTION)
>
> "a" -> "an"
>
> 18.2
> + /* get the pg_proc tuple */
>
> Uppercase comment
>
> 18.3
> + alterFunc = new_objtree_VA("ALTER FUNCTION", 0);
> +
> + params = NIL;
>
> Why not just assign params = NIL at the declaration?
>
> ~~~
>
> 19. src/backend/commands/ddl_deparse.c - deparse_AlterOwnerStmt
>
> + * Deparse a AlterOwnerStmt (ALTER ... OWNER TO ...).
>
> "a" -> "an"
>
> ~~~
>
> 20. src/backend/commands/ddl_deparse.c - deparse_AlterOperatorStmt
>
> + * Deparse a AlterOperatorStmt (ALTER OPERATOR ... SET ...).
>
> "a" -> "an"
>
> ~~~
>
> 21. src/backend/commands/ddl_deparse.c - deparse_RenameStmt
>
> 21.1
> + * In a ALTER .. RENAME command, we don't have the original name of the
>
> "a" -> "an"
>
> 21.2
> + relation_close(relation, AccessShareLock);
> +
> + break;
> + case OBJECT_SCHEMA:
>
> Misplaced bank line; should be before after break;
>
> 21.3
> + break;
> + case OBJECT_TRIGGER:
>
> Put blank line after break;
>
> 21.4
> + break;
> + default:
>
> Put blank line after break;
>
> ~~~

fixed the above comments.
>
> 22. src/backend/commands/ddl_deparse.c - deparse_Seq_Cache
>
> +static inline ObjElem *
> +deparse_Seq_Cache(ObjTree *parent, Form_pg_sequence seqdata, bool alter_table)
>
> Is param 'alter_table’ correct here or should that be 'alter_sequence'
> (or just 'alter')?
>
> ~~
>
> 23. src/backend/commands/ddl_deparse.c - deparse_Seq_Cycle
>
> +static inline ObjElem *
> +deparse_Seq_Cycle(ObjTree *parent, Form_pg_sequence seqdata, bool alter_table)
>
> Is param 'alter_table’ correct here or should that be 'alter_sequence'
> (or just 'alter')?
>
> ~~~
>
> 24. src/backend/commands/ddl_deparse.c - deparse_Seq_IncrementBy
>
> +static inline ObjElem *
> +deparse_Seq_IncrementBy(ObjTree *parent, Form_pg_sequence seqdata,
> bool alter_table)
>
> Is param 'alter_table’ correct here or should that be 'alter_sequence'
> (or just 'alter')?
>
> ~~~
>
> 25. src/backend/commands/ddl_deparse.c - deparse_Seq_Minvalue
>
> +static inline ObjElem *
> +deparse_Seq_Minvalue(ObjTree *parent, Form_pg_sequence seqdata, bool
> alter_table)
>
> Is param 'alter_table’ correct here or should that be 'alter_sequence'
> (or just 'alter')?
>
> ~~~
>
> 26. src/backend/commands/ddl_deparse.c - deparse_Seq_Maxvalue
>
> +static inline ObjElem *
> +deparse_Seq_Maxvalue(ObjTree *parent, Form_pg_sequence seqdata, bool
> alter_table)
>
> Is param 'alter_table’ correct here or should that be 'alter_sequence'
> (or just 'alter')?
>
> ~~~
>
> 27. src/backend/commands/ddl_deparse.c - deparse_Seq_Startwith
>
> +static inline ObjElem *
> +deparse_Seq_Startwith(ObjTree *parent, Form_pg_sequence seqdata, bool
> alter_table)
>
> Is param 'alter_table’ correct here or should that be 'alter_sequence'
> (or just 'alter')?
>
> ~~~

This  actually refers to whether the original  command is from
alter_table or not.
>
> 28. src/backend/commands/ddl_deparse.c - deparse_CreateSchemaStmt
>
> +/*
> + * Deparse a CreateSchemaStmt.
> + *
> + * Given a schema OID and the parsetree that created it, return an ObjTree
> + * representing the creation command.
> + *
> + * Note we don't output the schema elements given in the creation command.
> + * They must be output separately. (In the current implementation,
> + * CreateSchemaCommand passes them back to ProcessUtility, which will lead to
> + * this file if appropriate.)
> + */
>
> "this file" ??
>
> ~~~
removed this.

>
> 29. src/backend/commands/ddl_deparse.c - deparse_Seq_OwnedBy
>
> 29.1
> +static ObjElem *
> +deparse_Seq_OwnedBy(ObjTree *parent, Oid sequenceId, bool alter_table)
>
> Missing function comment.
>
> 29.2
> + /* only consider AUTO dependencies on pg_class */
>
> Uppercase comment.
>
> 29.3
> + if (!ownedby)
> + /* XXX this shouldn't happen ... */
> + ownedby = new_objtree_VA("OWNED BY %{owner}D",
> + 3,
> + "clause", ObjTypeString, "owned",
> + "owner", ObjTypeNull,
> + "present", ObjTypeBool, false);
> + return new_object_object(ownedby);
>
> Put blank line before return;
>
> ~~~
>
> 30. src/backend/commands/ddl_deparse.c - deparse_CreateSeqStmt
>
> 30.1
> + /* definition elemets */
>
> Uppercase comment, and typo "elemets"
>
> 30.2
> + /* we purposefully do not emit OWNED BY here */
>
> Uppercase comment
>
> ~~~
>
> 31. src/backend/commands/ddl_deparse.c - deparse_AlterTableStmt
>
> 31.1
> + tmp = new_objtree_VA("ADD CONSTRAINT %{name}I",
> + 2,
> + "type", ObjTypeString, "add constraint using index",
> + "name", ObjTypeString, get_constraint_name(constrOid));
>
> I think it was customary for you to put the number of varags on the
> 1st line, not like this. There are several others like this in this
> function which should also be changed (where they fit OK on the first
> line).
>
> 31.2
> + append_array_object(alterTableStmt, "%{subcmds:, }s", subcmds);
> + return alterTableStmt;
>
> Maybe add blank line before the return;
>
> ~~~
>
> 32. src/backend/commands/ddl_deparse.c - deparse_AlterOpFamily
>
> 32.1
> + list = NIL;
> + foreach(cell, cmd->d.opfam.operators)
>
> Why not assign list = NIL with the declaration?
>
> 32.2
> + proargtypes = procForm->proargtypes.values;
> + arglist = NIL;
> + for (i = 0; i < procForm->pronargs; i++)
>
> Why not assign arglist = NIL with the declaration?
>
> 32.3
> + if (!stmt->isDrop)
> + append_format_string(alterOpFam, "ADD");
> + else
> + append_format_string(alterOpFam, "DROP");
>
> Maybe simpler to reverse these; IMO isDrop means "DROP" makes more sense.
>
> ~~~
>
> 33. src/backend/commands/ddl_deparse.c - deparse_DefineStmt_Operator
>
> + /* Add the definition clause */
> + list = NIL;
>
> Why not assign list = NIL with the declaration?
>
> ~~~

fixed all the above comments.

>
> 34. src/backend/commands/ddl_deparse.c - deparse_DefineStmt
>
> + default:
> + elog(ERROR, "unsupported object kind");
> + return NULL;
>
> What purpose does this return serve after the ERROR? If you have to do
> something to quieten the compiler, maybe it's better  to set defStmt =
> NULL at declaration.
>
> ======
>
> 35. src/backend/commands/ddl_json.c - <General>
>
> In the errmsg text some of the messages say JSON (uppercase) and some
> say json (lowercase). IMO they should all be consistent. Maybe say
> JSON, since that way seems dominant.
>
> ~~~
>
> 36. src/backend/commands/ddl_json.c - enum
>
> +typedef enum
> +{
> + tv_absent,
> + tv_true,
> + tv_false
> +} trivalue;
>
> It seems there is another enum elsewhere already called this, because
> you did not add it into the typedefs.list, yet it is already there. Is
> that OK? Maybe this should have a unique name for this module.
>
> ~~~

changed this to say json_trivalue

>
> 37. src/backend/commands/ddl_json.c - expand_fmt_recursive
>
> 37.1
> + is_array = false;
> +
> + ADVANCE_PARSE_POINTER(cp, end_ptr);
>
> Why not assign is_array = false in the declaration?
>
> 37.2
> + /*
> + * Validate that we got an array if the format string specified one.
> + * And finally print out the data
> + */
> + if (is_array)
> + expand_jsonb_array(buf, param, value, arraysep, specifier, start_ptr);
> + else
> + expand_one_jsonb_element(buf, param, value, specifier, start_ptr);
>
> "Print out" the data? This comment seems a bit over-complicated.
> Perhaps these sentences can be combined and re-worded a bit.
>
> SUGGESTION (maybe?)
> Expand the data (possibly an array) into the output StringInfo.
>
> ~~~
>
> 38. src/backend/commands/ddl_json.c - expand_jsonval_identifier
>
> +/*
> + * Expand a json value as an identifier.  The value must be of type string.
> + */
> +static void
> +expand_jsonval_identifier(StringInfo buf, JsonbValue *jsonval)
>
> Should that say "as a quoted identifier" ?
>
> ~~~
>
> 39. src/backend/commands/ddl_json.c - expand_jsonval_typename
>
> 39.1
> + switch (is_array)
> + {
> + default:
> + case tv_absent:
>
> It seems slightly unusual for the default case to not be the last
> switch case. Consider rearranging it.
>
>

fixed this.

> 39.2
> + if (schema == NULL)
> + appendStringInfo(buf, "%s%s%s",
> + quote_identifier(typename),
> + typmodstr ? typmodstr : "",
> + array_decor);
> +
> + /* Special typmod needs */
> + else if (schema[0] == '\0')
> + appendStringInfo(buf, "%s%s%s",
> + typename,
> + typmodstr ? typmodstr : "",
> + array_decor);
> + else
> + appendStringInfo(buf, "%s.%s%s%s",
> + quote_identifier(schema),
> + quote_identifier(typename),
> + typmodstr ? typmodstr : "",
> + array_decor);
>
> The last 2 parts:
>                          typmodstr ? typmodstr : "",
>                          array_decor);
>
> are common for all those above appendStringInfo, so you could reduce
> the code (if you want to) and just add the common parts at the end.
>
> e.g.
>
> if (schema == NULL)
> appendStringInfo(buf, "%s", quote_identifier(typename));
> else if (schema[0] == '\0')
> appendStringInfo(buf, "%s", typename); /* Special typmod needs */
> else
> appendStringInfo(buf, "%s.%s", quote_identifier(schema),
> quote_identifier(typename));
>
> appendStringInfo(buf, "%s%s", typmodstr ? typmodstr : "", array_decor);
>
>

fixed it accordingly.

> 39.3
> In other code (e.g. expand_jsonval_dottedname) you did lots of
> pfree(str) after using the strings, so why not similar here?
>
> ~~~
>
> 40. src/backend/commands/ddl_json.c - expand_jsonval_operator
>
> 40.1
> + /* schema might be NULL or empty */
>
> Uppercase comment
>
> 40.2
> Why no pfree(str) here similar to what there was in prior code (e.g.
> expand_jsonval_dottedname)?
>
> ~~~
>
> 41. src/backend/commands/ddl_json.c - expand_jsonval_string
>
> Comment says "The value must be of type string or of type object."
>
> Yeah, but what it is isn't? This code will just fall thru and return
> true. Is that the right behaviour? Should there be an Assert at least?
>
> ~~~
>
> 42. src/backend/commands/ddl_json.c - expand_jsonval_number
>
> Does this need some pfree after the string is copied to 'buf'?
>
> ~~~
>
> 43 src/backend/commands/ddl_json.c - expand_jsonval_role
>
> + rolename = find_string_in_jsonbcontainer(jsonval->val.binary.data,
> + "rolename", false, NULL);
> + appendStringInfoString(buf, quote_identifier(rolename));
>
> Does this need some pfree after the string is copied to 'buf'?
>
> ~~~
>
> 44. src/backend/commands/ddl_json.c - ddl_deparse_json_to_string
>
> + d = DirectFunctionCall1(jsonb_in,
> + PointerGetDatum(json_str));
>
> Seems unnecessary wrapping here.
>
> ~~~
>
> 45. src/backend/commands/ddl_json.c - fmtstr_error_callback
>
> 45.1
> +/*
> + * Error context callback for JSON format string expansion.
> + *
> + * Possible improvement: indicate which element we're expanding, if applicable.
> + */
>
> Should that "Possible improvement" comment have "XXX" prefix like most
> other possible improvement comments have?
>
> 45.2
> +fmtstr_error_callback(void *arg)
> +{
> + errcontext("while expanding format string \"%s\"", (char *) arg);
> +
> +}
>
> Remove the blank line.
>
> ======
>
> 46. src/backend/utils/adt/ruleutils.c - pg_get_trigger_whenclause
>
> +char *
> +pg_get_trigger_whenclause(Form_pg_trigger trigrec, Node *whenClause,
> bool pretty)
>
> Missing function comment
>
> ~~~
>
> 47. src/backend/utils/adt/ruleutils.c - print_function_sqlbody
>
> @@ -3513,7 +3526,7 @@ pg_get_function_arg_default(PG_FUNCTION_ARGS)
>   PG_RETURN_TEXT_P(string_to_text(str));
>  }
>
> -static void
> +void
>  print_function_sqlbody(StringInfo buf, HeapTuple proctup)
>  {
>
> Having a function comment is more important now that this is no longer static.
>
> ------
fixed these.

regards,
Ajin Cherian
Fujitsu Australia