Re: Support logical replication of DDLs

On Thu, 9 Feb 2023 at 03:47, Peter Smith <smithpb2250@gmail.com> wrote:
>
> 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.

Added an Assert for this.

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

I checked that we have specified operator names everywhere, so added
error handling for this case and modified the function header
accordingly.

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

This looks better, I have included this change.

The attached v68 version patch has the changes for the same.

Regards,
Vignesh