Re: Support logical replication of DDLs

On Thu, 9 Feb 2023 at 03:47, Peter Smith <smithpb2250(at)gmail(dot)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(at)gmail(dot)com> wrote:
> >
> > On Mon, 6 Feb 2023 at 06:47, Peter Smith <smithpb2250(at)gmail(dot)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