Re: Support logical replication of DDLs

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

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);

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
);

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');

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

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.

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

Regards,
Vignesh