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

Hi Vignesh, thanks for addressing my v63-0002 review comments.

I confirmed most of the changes. Below is a quick follow-up for the
remaining ones.

On Mon, Feb 6, 2023 at 10:32 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Mon, 6 Feb 2023 at 06:47, Peter Smith <smithpb2250@gmail.com> wrote:
> >
...
> >
> > 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]

Thanks for the info. I saw that Zheng-san only wrote it is handled in
the “upcoming call of expand_one_jsonb_element”, but I don’t know if
that is sufficient. For example, if the execution heads down the other
path (expand_jsonb_array) with a NULL jsonarr then it going to crash,
isn't it? So I still think some change may be needed here.

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

Is it really OK for the “objname" to be optional here (Yes, I know the
code is currently implemented like it is OK, but I am doubtful)

That would everything can be optional and the buf result might be
nothing. It could also mean if the "schemaname" is provided but the
"objname" is not, then the buf will have a trailing ".".

It doesn't sound quite right to me.

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

Sorry, I messed up the previous code suggestion. It should have said:

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

    if (jsonval->type == jbvString)
    {
        appendBinaryStringInfo(buf, jsonval->val.string.val,
jsonval->val.string.len);
        expanded = true;
    }
    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)
        {
            expand_fmt_recursive(jsonval->val.binary.data, buf);
            expanded = true;
        }
    }
    return expanded;
}

~

But I have no special "optimization" in mind. Only, IMO the code is
easier to understand, because:
- 1 return is simpler than 3 returns
- 1 else is simpler than 2 else's

YMMV.

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