Re: Support logical replication of DDLs

On Thu, Feb 9, 2023 at 8:55 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Fri, Feb 3, 2023 at 11:41 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > Here are some review comments for patch v63-0001.
> >

> > ======
> > src/backend/catalog/aclchk.c
> >
> > 3. ExecuteGrantStmt
> >
> > + /* Copy the grantor id needed for DDL deparsing of Grant */
> > + istmt.grantor_uid = grantor;
> > +
> >
> > SUGGESTION (comment)
> > Copy the grantor id to the parsetree, needed for DDL deparsing of Grant
> >
>
> didn't change this, as Alvaro said this was not a parsetree.

Perhaps there is more to do here? Sorry, I did not understand the
details of Alvaro's post [1], but I did not recognize the difference
between ExecuteGrantStmt and ExecSecLabelStmt so it was my impression
either one or both of these places are either wrongly commented, or
maybe are doing something that should not be done.

> > ======
> > src/backend/utils/adt/regproc.c
> >
> > 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, ')');
> > +}
> >
> > 9a.
> > Assign argtype = NULL looks redundant because it will always be
> > overwritten anyhow.
> >
>
> changed this.
>
> > ~
> >
> > 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?

------
[1] https://www.postgresql.org/message-id/20230213090752.27ftbb6byiw3qcbl%40alvherre.pgsql

Kind Regards,
Peter Smith.
Fujitsu Australia