Re: Support logical replication of DDLs

On Wed, Feb 15, 2023 at 3:33 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> > >
> > > 9.
> > > +
> > > +/*
> > > + * Append the parenthesized arguments of the given pg_proc row into the output
> > > + * buffer. force_qualify indicates whether to schema-qualify type names
> > > + * regardless of visibility.
> > > + */
> > > +static void
> > > +format_procedure_args_internal(Form_pg_proc procform, StringInfo buf,
> > > + bool force_qualify)
> > > +{
> > > + int i;
> > > + char* (*func[2])(Oid) = {format_type_be, format_type_be_qualified};
> > > +
> > > + appendStringInfoChar(buf, '(');
> > > + for (i = 0; i < procform->pronargs; i++)
> > > + {
> > > + Oid thisargtype = procform->proargtypes.values[i];
> > > + char *argtype = NULL;
> > > +
> > > + if (i > 0)
> > > + appendStringInfoChar(buf, ',');
> > > +
> > > + argtype = func[force_qualify](thisargtype);
> > > + appendStringInfoString(buf, argtype);
> > > + pfree(argtype);
> > > + }
> > > + appendStringInfoChar(buf, ')');
> > > +}
> > >
> > > 9b.
> > > I understand why this function was put here beside the other static
> > > functions in "Support Routines" but IMO it really belongs nearby (i.e.
> > > directly above) the only caller (format_procedure_args). Keeping both
> > > those functional together will improve the readability of both, and
> > > will also remove the need to have the static forward declaration.
> > >
>
> There was no reply for 9b. Was it accidentally overlooked, or just
> chose not to do it?

Fixed this. Moved the function up and removed the forward declaration.

On Wed, Feb 15, 2023 at 3:00 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Sat, Feb 11, 2023 at 3:21 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > 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.
> >
>
> Was this a correct change to make here?
>
> IIUC this Assert is now going to intercept both cases including the
> expand_one_jsonb_element() which previously would have thrown a proper
> ERROR.
>
Fixed this. Added an error check in expand_jsonb_array() as well.

Changes are in patch 1 and patch 2
regards,
Ajin Cherian
Fujitsu Australia