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

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

~~~

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

~~~

3.

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

~~~

4.

For "Note #4":

"We created" -> "we created"

======

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;

======

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?

~

6b.
Seems some unnecessary wrapping here


======

src/backend/commands/ddl_deparse.c

WIP - I will try to post some review comments on this file next week

======

src/backend/commands/ddl_json.c

7. convSpecifier

typedef enum
{
    SpecTypename,
    SpecOperatorname,
    SpecDottedName,
    SpecString,
    SpecNumber,
    SpecStringLiteral,
    SpecIdentifier,
    SpecRole
} convSpecifier;

Inconsistent case. Some of these say "name" and some say "Name"

~~~

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.

~~~

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.

~~~

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.

~

10b.
The first sentence is missing a period.

~~~

11.

        value = findJsonbValueFromContainer(container, JB_FOBJECT, &key);

Should this be checking is value is NULL?

~~~

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.

~~~

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

~~~

14. expand_jsonval_operator

The function comment is missing a period.

~~~

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 (??)

~

15b.
    else
        return false;

Is that OK to just return false, or should this in fact be throwing an
error if the wrong type?

~~~

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)

~~~

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?

~~~

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

~~~

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

~~~

20. ddl_deparse_expand_command

 * %        expand to a literal %.


Remove the period from that line (because not of the other specifier
descriptions have one).

======

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.

~~~

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'?

~

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


======

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.

~~~

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

~

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

~~~

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

~~~

26. pg_get_trigger_whenclause

Missing function 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?

======

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

------
Kind Regards,
Peter Smith.
Fujitsu Australia