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

On Mon, 6 Feb 2023 at 06:47, Peter Smith <smithpb2250@gmail.com> wrote:
>
> 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.

Modified

> 2. Copyright
>
> + * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
>
> "2022" --> "2023"
>
Modified

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

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

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

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

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

>
> 7.
> + ADVANCE_PARSE_POINTER(cp, end_ptr);
> + for (; cp < end_ptr;)
> + {
>
>
> Maybe a while loop is more appropriate?
>
Modified

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

Yes, this was already answered by Zheng, quoting as "The null checking
for value is done in the upcoming call of expand_one_jsonb_element()."
in [1]

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

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

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

Modified

> ~~~
>
> 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;
> }

I'm not sure if this change is required as this will introduce a new
variable and require it to be set, this variable should be set to
"expand = false" in else after else if also, instead I preferred the
existing code. I did not make any change for this unless you are
seeing some bigger optimization.

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

Modified

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

Modified

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

Modified

> 15b.
> Maybe use a better variable name.
>
> "result" --> "string_expanded"

Modified

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

Modified

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

Modified

> src/include/tcop/ddl_deparse.h
>
> 17.
> + * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
>
> "2022" --> "2023"

Modified

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

Modified

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

Modified

[1] - https://www.postgresql.org/message-id/CAAD30UJ2MmA7vM1H2b20L_SMHS0-76raROqZELs-GDGk3Pet5A%40mail.gmail.com

Regards,
Vignesh