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

Here are some comments for patch v63-0002.

This is a WIP because I have not yet looked at the large file - ddl_deparse.c.

======
Commit Message

1.
This patch provides JSON blobs representing DDL commands, which can
later be re-processed into plain strings by well-defined sprintf-like
expansion. These JSON objects are intended to allow for machine-editing of
the commands, by replacing certain nodes within the objects.

~

"This patch provides JSON blobs" --> "This patch constructs JSON blobs"

======
src/backend/commands/ddl_json.

2. Copyright

+ * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group

"2022" --> "2023"

~~~

3.
+/*
+ * Conversion specifier which determines how we expand the JSON element into
+ * string.
+ */
+typedef enum
+{
+ SpecTypeName,
+ SpecOperatorName,
+ SpecDottedName,
+ SpecString,
+ SpecNumber,
+ SpecStringLiteral,
+ SpecIdentifier,
+ SpecRole
+} convSpecifier;

~

3a.
SUGGESTION (comment)
Conversion specifier which determines how to expand the JSON element
into a string.

~

3b.
Are these enums in this strange order deliberately? If not, then maybe
alphabetical is better.

~~~

4. Forward declaration

+char *deparse_ddl_json_to_string(char *jsonb);

Why is this forward declared here? Isn't this already declared extern
in ddl_deparse.h?

~~~

5. expand_fmt_recursive

+/*
+ * Recursive helper for deparse_ddl_json_to_string.
+ *
+ * Find the "fmt" element in the given container, and expand it into the
+ * provided StringInfo.
+ */
+static void
+expand_fmt_recursive(JsonbContainer *container, StringInfo buf)

I noticed all the other expand_XXXX functions are passing the
StringInfo buf as the first parameter. For consistency, shouldn’t this
be the same?

~

6.
+ if (*cp != '%')
+ {
+ appendStringInfoCharMacro(buf, *cp);
+ continue;
+ }
+
+
+ ADVANCE_PARSE_POINTER(cp, end_ptr);
+
+ /* Easy case: %% outputs a single % */
+ if (*cp == '%')
+ {
+ appendStringInfoCharMacro(buf, *cp);
+ continue;
+ }

Double blank lines?

~

7.
+ ADVANCE_PARSE_POINTER(cp, end_ptr);
+ for (; cp < end_ptr;)
+ {


Maybe a while loop is more appropriate?

~

8.
+ value = findJsonbValueFromContainer(container, JB_FOBJECT, &key);

Should the code be checking or asserting value is not NULL?

(IIRC I asked this a long time ago - sorry if it was already answered)

~~~

9. expand_jsonval_dottedname

It might be simpler code to use a variable like:
JsonbContainer *data = jsonval->val.binary.data;

Instead of repeating jsonval->val.binary.data many times.

~~~

10. expand_jsonval_typename

It might be simpler code to use a variable like:
JsonbContainer *data = jsonval->val.binary.data;

Instead of repeating jsonval->val.binary.data many times.

~~~

11.
+/*
+ * Expand a JSON value as an operator name.
+ */
+static void
+expand_jsonval_operator(StringInfo buf, JsonbValue *jsonval)

Should this function comment be more like the comment for
expand_jsonval_dottedname by saying there can be an optional
"schemaname"?

~~~

12.
+static bool
+expand_jsonval_string(StringInfo buf, JsonbValue *jsonval)
+{
+ if (jsonval->type == jbvString)
+ {
+ appendBinaryStringInfo(buf, jsonval->val.string.val,
+    jsonval->val.string.len);
+ }
+ else if (jsonval->type == jbvBinary)
+ {
+ json_trivalue present;
+
+ present = find_bool_in_jsonbcontainer(jsonval->val.binary.data,
+   "present");
+
+ /*
+ * If "present" is set to false, this element expands to empty;
+ * otherwise (either true or absent), fall through to expand "fmt".
+ */
+ if (present == tv_false)
+ return false;
+
+ expand_fmt_recursive(jsonval->val.binary.data, buf);
+ }
+ else
+ return false;
+
+ return true;
+}

I felt this could be simpler if there is a new 'expanded' variable
because then you can have just a single return point instead of 3
returns;

If you choose to do this there is a minor tweak to the "fall through" comment.

SUGGESTION
expand_jsonval_string(StringInfo buf, JsonbValue *jsonval)
{
    bool expanded = true;

    if (jsonval->type == jbvString)
    {
        appendBinaryStringInfo(buf, jsonval->val.string.val,
                               jsonval->val.string.len);
    }
    else if (jsonval->type == jbvBinary)
    {
        json_trivalue present;

        present = find_bool_in_jsonbcontainer(jsonval->val.binary.data,
                                              "present");

        /*
         * If "present" is set to false, this element expands to empty;
         * otherwise (either true or absent), expand "fmt".
         */
        if (present == tv_false)
            expanded = false;
        else
            expand_fmt_recursive(jsonval->val.binary.data, buf);
    }

    return expanded;
}

~~~

13.
+/*
+ * Expand a JSON value as an integer quantity.
+ */
+static void
+expand_jsonval_number(StringInfo buf, JsonbValue *jsonval)
+{

Should this also be checking/asserting that the type is jbvNumeric?

~~~

14.
+/*
+ * 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.
+ */
+static void
+expand_jsonval_role(StringInfo buf, JsonbValue *jsonval)

Maybe more readable to quote that param?

BEFORE
If the is_public element is set...

AFTER
If the 'is_public' element is set...

~~~

15.
+ *
+ * Returns false if no actual expansion was made (due to the "present" flag
+ * being set to "false" in formatted string expansion).
+ */
+static bool
+expand_one_jsonb_element(StringInfo buf, char *param, JsonbValue *jsonval,
+ convSpecifier specifier, const char *fmt)
+{
+ bool result = true;
+ ErrorContextCallback sqlerrcontext;

~

15a.
Looking at the implementation, maybe that comment can be made more
clear. Something like below:

SUGGESTION
Returns true, except for the formatted string case if no actual
expansion was made (due to the "present" flag being set to "false").

~

15b.
Maybe use a better variable name.

"result" --> "string_expanded"

======
src/include/catalog/pg_proc.dat

16.
@@ -11891,4 +11891,10 @@
   prorettype => 'bytea', proargtypes => 'pg_brin_minmax_multi_summary',
   prosrc => 'brin_minmax_multi_summary_send' },

+{ oid => '4642', descr => 'deparse the DDL command into JSON format string',
+  proname => 'ddl_deparse_to_json', prorettype => 'text',
+  proargtypes => 'pg_ddl_command', prosrc => 'ddl_deparse_to_json' },
+{ oid => '4643', descr => 'expand JSON format DDL to a plain DDL command',
+  proname => 'ddl_deparse_expand_command', prorettype => 'text',
+  pr

16a.
"deparse the DDL command into JSON format string" ==> "deparse the DDL
command into a JSON format string"

~

16b.
"expand JSON format DDL to a plain DDL command" --> "expand JSON
format DDL to a plain text DDL command"

======
src/include/tcop/ddl_deparse.h

17.
+ * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group

"2022" --> "2023"

~~~

+extern char *deparse_utility_command(CollectedCommand *cmd, bool verbose_mode);
+extern char *deparse_ddl_json_to_string(char *jsonb);
+extern char *deparse_drop_command(const char *objidentity, const char
*objecttype,
+   DropBehavior behavior);
+
+
+#endif /* DDL_DEPARSE_H */

Double blank lines.

======
src/include/tcop/deparse_utility.h

18.
@@ -100,6 +103,12 @@ typedef struct CollectedCommand
  {
  ObjectType objtype;
  } defprivs;
+
+ struct
+ {
+ ObjectAddress address;
+ Node *real_create;
+ } ctas;
  } d;

All the other sub-structures have comments. IMO this one should have a
comment too.

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