Re: Support logical replication of DDLs

On Mon, 6 Mar 2023 at 18:43, vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Mon, 6 Mar 2023 at 12:04, Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> >
> > 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
>
> Few comments:
> 1) The following statement crashes:
> CREATE TABLE itest7b (a int);
> CREATE TABLE itest7c (a int GENERATED ALWAYS AS IDENTITY) INHERITS (itest7b);
> #0 0x0000559018aff927 in RangeVarGetRelidExtended (relation=0x0,
> lockmode=0, flags=0, callback=0x0, callback_arg=0x0) at
> namespace.c:255
> #1 0x0000559018be09dc in deparse_ColumnDef (relation=0x7f3e917abba8,
> dpcontext=0x55901a792668, composite=false, coldef=0x55901a77d758,
> is_alter=false, exprs=0x0) at ddl_deparse.c:1657
> #2 0x0000559018be2271 in deparse_TableElements
> (relation=0x7f3e917abba8, tableElements=0x55901a77d708,
> dpcontext=0x55901a792668, typed=false, composite=false) at
> ddl_deparse.c:2460
> #3 0x0000559018be2b89 in deparse_CreateStmt (objectId=16420,
> parsetree=0x55901a77d5f8) at ddl_deparse.c:2722
> #4 0x0000559018bf72c3 in deparse_simple_command (cmd=0x55901a77d590,
> include_owner=0x7ffe4e611234) at ddl_deparse.c:10019
> #5 0x0000559018bf7563 in deparse_utility_command (cmd=0x55901a77d590,
> include_owner=true, verbose_mode=false) at ddl_deparse.c:10122
> #6 0x0000559018eb650d in publication_deparse_ddl_command_end
> (fcinfo=0x7ffe4e6113f0) at ddltrigger.c:203

Fixed

> 2) invalid type storage error:
> CREATE TYPE myvarchar;
>
> CREATE FUNCTION myvarcharin(cstring, oid, integer) RETURNS myvarchar
> LANGUAGE internal IMMUTABLE PARALLEL SAFE STRICT AS 'varcharin';
>
> CREATE FUNCTION myvarcharout(myvarchar) RETURNS cstring
> LANGUAGE internal IMMUTABLE PARALLEL SAFE STRICT AS 'varcharout';
>
> CREATE FUNCTION myvarcharsend(myvarchar) RETURNS bytea
> LANGUAGE internal STABLE PARALLEL SAFE STRICT AS 'varcharsend';
>
> CREATE FUNCTION myvarcharrecv(internal, oid, integer) RETURNS myvarchar
> LANGUAGE internal STABLE PARALLEL SAFE STRICT AS 'varcharrecv';
>
> CREATE TYPE myvarchar (
> input = myvarcharin,
> output = myvarcharout,
> alignment = integer,
> storage = main
> );
>
> -- want to check updating of a domain over the target type, too
> CREATE DOMAIN myvarchardom AS myvarchar;
>
> ALTER TYPE myvarchar SET (storage = extended);

Fixed

> 3) invalid type option send
> ALTER TYPE myvarchar SET (
> send = myvarcharsend,
> receive = myvarcharrecv,
> typmod_in = varchartypmodin,
> typmod_out = varchartypmodout,
> -- these are bogus, but it's safe as long as we don't use the type:
> analyze = ts_typanalyze,
> subscript = raw_array_subscript_handler
> );

Fixed

> 4) There are some unsupported alter table subtype:
> CREATE FOREIGN DATA WRAPPER dummy;
> CREATE SERVER s0 FOREIGN DATA WRAPPER dummy;
> CREATE FOREIGN TABLE ft1 (
> c1 integer OPTIONS ("param 1" 'val1') NOT NULL,
> c2 text OPTIONS (param2 'val2', param3 'val3') CHECK (c2 <> ''),
> c3 date,
> CHECK (c3 BETWEEN '1994-01-01'::date AND '1994-01-31'::date)
> ) SERVER s0 OPTIONS (delimiter ',', quote '"', "be quoted" 'value');

Fixed

> 5) similarly in case of alter foreign table:
> ALTER FOREIGN TABLE ft1 ADD COLUMN c10 integer OPTIONS (p1 'v1');

Fixed

> 6) Few whitespace errors:
> Applying: Infrastructure to support DDL deparsing.
> .git/rebase-apply/patch:486: indent with spaces.
> bool force_qualify)
> .git/rebase-apply/patch:488: indent with spaces.
> int i;
> .git/rebase-apply/patch:489: indent with spaces.
> char* (*func[2])(Oid) = {format_type_be, format_type_be_qualified};
> .git/rebase-apply/patch:491: indent with spaces.
> appendStringInfoChar(buf, '(');
> .git/rebase-apply/patch:492: indent with spaces.
> for (i = 0; i < procform->pronargs; i++)
> warning: squelched 10 whitespace errors
> warning: 15 lines add whitespace errors.

These were already fixed

> 7) Alter foreign table rename not handled:
> ALTER FOREIGN TABLE foreign_schema.ft1 RENAME TO foreign_table_1;

Fixed

Attached v77 version patch has the fixes for the same.
Patches 0002 and 0003 were modified to fix the above issues.

Regards,
Vignesh