Re: Support logical replication of DDLs

On Fri, Oct 28, 2022 at 2:50 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Here are some review comments for patch v32-0001.
>
> This is a WIP - I have not yet looked at the largest file of this
> patch (src/backend/commands/ddl_deparse.c)
>
> ======
>
> Commit Message
>
> 1.
>
> The list of the supported statements should be in alphabetical order
> to make it easier to read
Updated the list in the commit message.

> 2.
>
> The "Notes" are obviously notes, so the text does not need to say
> "Note that..." etc again
>
> "(Note #1) Note that some..." -> "(Note #1) Some..."
>
> "(Note #2) Note that, for..." -> "(Note #2) For..."
>
> "(Note #4) Note that, for..." -> "(Note #4) For..."
Modified.

> 3.
>
> For "Note #3", use uppercase for the SQL keywords in the example.
Modified.

> 4.
>
> For "Note #4":
>
> "We created" -> "we created"
Modified.

> ======
>
> src/backend/catalog/aclchk.c
>
> 5. ExecuteGrantStmt
>
> @@ -385,7 +385,11 @@ ExecuteGrantStmt(GrantStmt *stmt)
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("grantor must be current user")));
> +
> + istmt.grantor_uid = grantor;
> }
> + else
> + istmt.grantor_uid = InvalidOid;
>
> This code can be simpler by just declaring the 'grantor' variable at
> function scope, then assigning the istmt.grantor_uid along with the
> other grantor assignments.
>
> SUGGESTION
> Oid grantor = InvalidOid;
> ...
> istmt.grantor_uid = grantor;
> istmt.is_grant = stmt->is_grant;
> istmt.objtype = stmt->objtype;
Modified.

> ======
>
> src/backend/commands/collationcmds.c
>
> 6. DefineCollation
>
> + /* make from existing collationid available to callers */
> + if (from_collid && OidIsValid(collid))
> + ObjectAddressSet(*from_collid,
> + CollationRelationId,
> + collid);
>
> 6a.
> Maybe the param can be made 'from_existing_colid', then the above code
> comment can be made more readable?
Modified.

> 6b.
> Seems some unnecessary wrapping here
Modified.

> 7. convSpecifier
>
> typedef enum
> {
> SpecTypename,
> SpecOperatorname,
> SpecDottedName,
> SpecString,
> SpecNumber,
> SpecStringLiteral,
> SpecIdentifier,
> SpecRole
> } convSpecifier;
>
> Inconsistent case. Some of these say "name" and some say "Name"
Modified.

> 8. Forward declarations
>
> char *ddl_deparse_json_to_string(char *jsonb);
>
> Is this needed here? I thought this was already declared extern in
> ddl_deparse.h.
It is needed. We get the following warning without it:
ddl_json.c:704:1: warning: no previous prototype for
‘ddl_deparse_json_to_string’ [-Wmissing-prototypes]
ddl_deparse_json_to_string(char *json_str)

> 9. find_string_in_jsonbcontainer
>
> The function comment says "If it's of a type other than jbvString, an
> error is raised.", but I do not see this check in the function code.
Modified.

> 10. expand_fmt_recursive
>
> /*
> * Recursive helper for pg_event_trigger_expand_command
> *
> * Find the "fmt" element in the given container, and expand it into the
> * provided StringInfo.
> */
>
>
> 10a.
> I am not sure if the mention of "pg_event_trigger_expand_command" is
> stale or is not relevant anymore, because that caller is not in this
> module.
Modified.

> 10b.
> The first sentence is missing a period.
Modified.

> 11.
>
> value = findJsonbValueFromContainer(container, JB_FOBJECT, &key);
>
> Should this be checking is value is NULL?
The null checking for value is done in the upcoming call of
expand_one_jsonb_element().

> 12. expand_jsonval_dottedname
>
> * Expand a json value as a dot-separated-name. The value must be of type
> * object and may contain elements "schemaname" (optional), "objname"
> * (mandatory), "attrname" (optional). Double quotes are added to each element
> * as necessary, and dot separators where needed.
>
> The comment says "The value must be of type object" but I don't see
> any check/assert for that in the code.
The value must be of type binary, updated comment and added
Assert(jsonval→type == jbvBinary);

> 13. expand_jsonval_typename
>
> In other code (e.g. expand_jsonval_dottedname) there are lots of
> pfree(str) so why not similar here?
>
> e.g. Shouldn’t the end of the function have like shown below:
> pfree(schema);
> pfree(typename);
> pfree(typmodstr);
Modified.

> 14. expand_jsonval_operator
>
> The function comment is missing a period.
Modified.

> 15. expand_jsonval_string
>
> /*
> * Expand a JSON value as a string. The value must be of type string or of
> * type object. In the latter case, it must contain a "fmt" element which will
> * be recursively expanded; also, if the object contains an element "present"
> * and it is set to false, the expansion is the empty string.
>
> 15a.
> Although the comment says "The value must be of type string or of type
> object" the code is checking for jbvString and jbvBinary (??)
Updated the comment to “The value must be of type string or of type
binary”

> 15b.
> else
> return false;
>
> Is that OK to just return false, or should this in fact be throwing an
> error if the wrong type?
The caller checks the type is either jbvString or jbvBinary. Added comment
“The caller is responsible to check jsonval is of type jbvString or jbvBinary”.

> 16. expand_jsonval_strlit
>
> /* Easy case: if there are no ' and no \, just use a single quote */
> if (strchr(str, '\'') == NULL &&
> strchr(str, '\\') == NULL)
>
> That could be simplified as:
>
> if ((strpbk(str, "\'\\") == NULL)
Modified.

> 17. expand_jsonval_number
>
> strdatum = DatumGetCString(DirectFunctionCall1(numeric_out,
>
> NumericGetDatum(jsonval->val.numeric)));
> appendStringInfoString(buf, strdatum);
>
> Shouldn't this function do pfree(strdatum) at the end?
Modified.

> 18. expand_jsonval_role
>
> /*
> * Expand a JSON value as a role name. If the is_public element is set to
> * true, PUBLIC is expanded (no quotes); otherwise, expand the given role name,
> * quoting as an identifier.
> */
>
>
> Maybe better to quote that element name -> 'If the "is_public" element
> is set to true...'
I think we need to quote the non-public roles just in case they
contain special characters.

> 19. expand_one_jsonb_element
>
> The enum jbvType definition says that jbvBinary is a combination of
> array/object, so I am not sure if that should be reflected in the
> errmsg text (multiple places in this function body) instead of only
> saying "JSON object".
Updated errmsg texts to “JSON struct”.

> 20. ddl_deparse_expand_command
>
> * % expand to a literal %.
>
>
> Remove the period from that line (because not of the other specifier
> descriptions have one).
Modified.

> ======
>
> src/backend/utils/adt/regproc.c
>
> 21. format_procedure_args_internal
>
> +static void
> +format_procedure_args_internal(Form_pg_proc procform, StringInfo buf,
> + bool force_qualify)
> +{
> + int i;
> + int nargs = procform->pronargs;
>
> The 'nargs' var is used one time only, so hardly seems worth having it.
Modified.

> 22.
>
> + appendStringInfoString(buf,
> + force_qualify ?
> + format_type_be_qualified(thisargtype) :
> + format_type_be(thisargtype));
>
> 22a.
> Should these function results be assigned to a char* ptr so that they
> can be pfree(ptr) AFTER being appended to the 'buf'?
Modified.

> 22b.
> It's not really nececessary to check the force_qualify at every
> iteration. More effient to asign a function pointer outside this loop
> and just call that here. IIRC something like this:
>
> char * (*func[2])(Oid) = { format_type_be, format_type_be_qualified };
>
> ...
>
> then
> appendStringInfoString(buf, func[force_qualify](thisargtype))
Modified.

> src/backend/utils/adt/ruleutils.c
>
> 23. pg_get_ruledef_detailed
>
> Instead of the multiple if/else it might be easier to just assignup-front:
> *whereClause = NULL;
> *actions = NIL;
>
> Then the if blocks can just overwrite them.
>
> Also, if you do that, then I expect probably the 'output' temp list
> var is not needed at all.
Modified.

> 24. pg_get_viewdef_internal
>
> /*
> * In the case that the CREATE VIEW command execution is still in progress,
> * we would need to search the system cache RULERELNAME to get the rewrite
> * rule of the view as oppose to querying pg_rewrite as in
> pg_get_viewdef_worker(),
> * the latter will return empty result.
> */
>
> 24a.
> I'm not quite sure of the context of this function call. Maybe the
> comment was supposed to be worded more like below?
>
> "Because this function is called when CREATE VIEW command execution is
> still in progress, we need to search..."
Improved comment.

> 24b.
> "as oppose" -> "as opposed"
Modified.

> 25. pg_get_triggerdef_worker
>
> if (!isnull)
> {
> Node *qual;
> char *qualstr;
>
> qual = stringToNode(TextDatumGetCString(value));
> qualstr = pg_get_trigger_whenclause(trigrec, qual, pretty);
>
> appendStringInfo(&buf, "WHEN (%s) ", qualstr);
> }
>
> After appending the qualstr to buf, should there be a pfree(qualstr)?
I think we should skip pfree(qualstr) here since the memory is allocated
by initStringInfo in pg_get_trigger_whenclause, to avoid double free when
the StringInfoData in pg_get_trigger_whenclause gets freed.

> 26. pg_get_trigger_whenclause
>
> Missing function comment.
Added comment.

> 27. print_function_sqlbody
>
> -static void
> +void
> print_function_sqlbody(StringInfo buf, HeapTuple proctup)
> {
>
> Missing function comment. Probably having a function comment is more
> important now that this is not static?
Added comment.

> src/include/tcop/ddl_deparse.h
>
> 28.
>
> +extern char *deparse_utility_command(CollectedCommand *cmd, bool verbose_mode);
> +extern char *ddl_deparse_json_to_string(char *jsonb);
> +extern char *deparse_drop_command(const char *objidentity, const char
> *objecttype,
> + DropBehavior behavior);
>
> Function naming seems inconsistent. ('ddl_deparse_XXX' versus 'deparse_XXX').
modified.

Regards,
Zheng