MIME-Version: 1.0
References: 
 <CAAD30UKg8rXeGM8Oy_MAmxKBL_K5DiHXdeNF=hUefcu1C_6VfQ@mail.gmail.com>
 <20221006171601.6um4ey5idm4h62vf@alvherre.pgsql>
 <CAAD30UJh+LcUND+zg_A5dbQ_Bi=m_n7qUfKrOwAx2v4jBKWvKQ@mail.gmail.com>
 <CAFPTHDbVujj6C3TdMCmoBXovpc4=5Ow3i5M1_HNhmnqdiA5qSA@mail.gmail.com>
 <CAAD30UKCCpMTZd4WSSxYu-qRAEFd+0kjXTgje+EXHdo1JTkz0g@mail.gmail.com>
 <CALDaNm3F9GvQ9bPrxobhqkUKP3HmrRZGCU3EX0xt3=Ef0-Reaw@mail.gmail.com>
 <CAFPTHDY2O42ouQFMeEbPt51CWQ=zyzYhgK6B9basyd5PLaOv0A@mail.gmail.com>
 <CAFPTHDZmS2PrOyJx8OAe+Nt-Fx4-GZetJatqCEJhDNXA2orwCg@mail.gmail.com>
 <CAFiTN-tWkJW-JEGANkK+O3KXUjv_Yb5Tb+r83ujbognHG_brTA@mail.gmail.com>
 <CALDaNm08gZq9a7xnsbaJMmHmi29_kbEuyShHHfxAKLXPh6btWQ@mail.gmail.com>
 <CAFPTHDbGRpXOF-j+oemm=xkzjrh-ZnqPV7SMS_+QBPYdUwhWxg@mail.gmail.com>
 <CAAD30UL=wV7pvdwJ6w5UQUc3urHs6W2vnomK2MKUyek3rzsKcA@mail.gmail.com>
 <CAHut+Psm+9q++y8b70QTeBeZiYcfDNtc_obxSwhxukp0MFhnRA@mail.gmail.com>
 <CAHut+PsO0dwWoB4B6L3Ucd6D8ckgdXY2Sd3JK7F_3wLsXU7ZAg@mail.gmail.com>
 <CAHut+Puxo_kq2toicNK_BQdeccK3REGW-Xv8tVauFvTNku6V-w@mail.gmail.com>
 <CAHut+Pu6H6hY0JJNNRCRmFpM_3817z=0xjm-_ibP+cL85pBOpQ@mail.gmail.com>
 <CALDaNm3q_2Qfo4XbFkBT8mPz_ALS2MyuR=isP7_8Pz27JnLHiw@mail.gmail.com>
 <CAGfChW5Qbb8WKznzVqp+cyTqkzLt_vEeSe14td14et6HjnH7qw@mail.gmail.com>
 <CAAD30UKwHmLo1YyjB1j_vCEth0+OS9hkLr9Nrrsw5uwEuOs=+g@mail.gmail.com>
 <CALDaNm2V6YL6H4P9ZT95Ua_RDJaeDTUf6V0UDfrz4_vxhM5pMg@mail.gmail.com>
 <CAAD30ULrZB-RNmuD3NMr1jGNUt15ZpPgFdFRX53HbcAB76hefw@mail.gmail.com>
 <CAAD30UL0EmSGk58eZxxYzFTQ5EqSpX6d+OcX5MuE0j0r2ZgyDA@mail.gmail.com>
 <CAAD30ULAVzCnNOsnHwZWoeWfaYr3yDtM-7TnkMJf9HTrW7HVCQ@mail.gmail.com>
 <CAAD30U+fqaaD6533-eiaWVHpUaBNBCEvqyXOT_ow1B--Aa_jOQ@mail.gmail.com>
 <CALDaNm0mnkHXnQ7zMDxY6=ZtWn8A1iKLKGhEtqRVC9c3gc8YCw@mail.gmail.com>
 <CAFPTHDY_9_xd8JEckB=Au2ALPnx+hfmn9ztutVw4aVwZxLhrNQ@mail.gmail.com>
 <CALDaNm3YEFNwFkkbzS1J8NiHKX6LWQ_h3CA+CPtkUqAZgD8q+Q@mail.gmail.com>
 <CAFPTHDY_4koZ135_Bub_=HPBxpG1u+XBEv8zspwXW7rAQps6eA@mail.gmail.com>
 <CALDaNm0dOwbPVrkqw9OLN=TVFdo5aroUNwdjG5SAcKQg93-g0w@mail.gmail.com>
 <CAHut+PtH9S+3-TGPa4_uipBP-4-Kksx-FrEkg9ZtK0FphVRA5A@mail.gmail.com>
 <CAFPTHDbZzCeYMPJn0iFuD_ggpY-0ZHfVBHgQ9VJ6v4dF59xang@mail.gmail.com>
 <CAHut+PusvDXTJa2drUPzePBBW0i9we8yns8Z4DU4b7JVs6Ew1Q@mail.gmail.com>
 <CAFPTHDa6aHhwZkQ+GjFNL1dSQbMfoEtN-6y9wseMywCLdKuUYQ@mail.gmail.com>
In-Reply-To: 
 <CAFPTHDa6aHhwZkQ+GjFNL1dSQbMfoEtN-6y9wseMywCLdKuUYQ@mail.gmail.com>
From: vignesh C <vignesh21@gmail.com>
Date: Mon, 6 Mar 2023 18:43:51 +0530
Message-ID: 
 <CALDaNm3u2eskS_eC19AFpg5Mx3DgyuSuyoJ4wHtgK1AAicSfzQ@mail.gmail.com>
Subject: Re: Support logical replication of DDLs
To: Ajin Cherian <itsajin@gmail.com>
Cc: Peter Smith <smithpb2250@gmail.com>, Zheng Li <zhengli10@gmail.com>,
	li jie <ggysxcq@gmail.com>, Dilip Kumar <dilipbalaut@gmail.com>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
 "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>,
	Amit Kapila <amit.kapila16@gmail.com>,
 Masahiko Sawada <sawada.mshk@gmail.com>,
	Japin Li <japinli@hotmail.com>, rajesh singarapu <rajesh.rs0541@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CALDaNm3u2eskS_eC19AFpg5Mx3DgyuSuyoJ4wHtgK1AAicSfzQ%40mail.gmail.com>
Precedence: bulk

On Mon, 6 Mar 2023 at 12:04, Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Wed, Feb 15, 2023 at 3:33=E2=80=AFPM Peter Smith <smithpb2250@gmail.co=
m> wrote:
> >
> > > >
> > > > 9.
> > > > +
> > > > +/*
> > > > + * Append the parenthesized arguments of the given pg_proc row int=
o 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 b=
uf,
> > > > +    bool force_qualify)
> > > > +{
> > > > + int i;
> > > > + char* (*func[2])(Oid) =3D {format_type_be, format_type_be_qualifi=
ed};
> > > > +
> > > > + appendStringInfoChar(buf, '(');
> > > > + for (i =3D 0; i < procform->pronargs; i++)
> > > > + {
> > > > + Oid thisargtype =3D procform->proargtypes.values[i];
> > > > + char    *argtype =3D NULL;
> > > > +
> > > > + if (i > 0)
> > > > + appendStringInfoChar(buf, ',');
> > > > +
> > > > + argtype =3D 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 bo=
th
> > > > 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=E2=80=AFPM Peter Smith <smithpb2250@gmail.co=
m> wrote:
> >
> > On Sat, Feb 11, 2023 at 3:21 AM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > On Thu, 9 Feb 2023 at 03:47, Peter Smith <smithpb2250@gmail.com> wrot=
e:
> > > >
> > > > 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@gmail.com> wro=
te:
> > > > >
> > > > > On Mon, 6 Feb 2023 at 06:47, Peter Smith <smithpb2250@gmail.com> =
wrote:
> > > > > >
> > > > ...
> > > > > >
> > > > > > 8.
> > > > > > + value =3D 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 an=
swered)
> > > > > >
> > > > >
> > > > > Yes, this was already answered by Zheng, quoting as "The null che=
cking
> > > > > for value is done in the upcoming call of expand_one_jsonb_elemen=
t()."
> > > > > in [1]
> > > >
> > > > Thanks for the info. I saw that Zheng-san only wrote it is handled =
in
> > > > the =E2=80=9Cupcoming call of expand_one_jsonb_element=E2=80=9D, bu=
t I don=E2=80=99t know if
> > > > that is sufficient. For example, if the execution heads down the ot=
her
> > > > path (expand_jsonb_array) with a NULL jsonarr then it going to cras=
h,
> > > > 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=3D0x0,
lockmode=3D0, flags=3D0, callback=3D0x0, callback_arg=3D0x0) at
namespace.c:255
#1  0x0000559018be09dc in deparse_ColumnDef (relation=3D0x7f3e917abba8,
dpcontext=3D0x55901a792668, composite=3Dfalse, coldef=3D0x55901a77d758,
is_alter=3Dfalse, exprs=3D0x0) at ddl_deparse.c:1657
#2  0x0000559018be2271 in deparse_TableElements
(relation=3D0x7f3e917abba8, tableElements=3D0x55901a77d708,
dpcontext=3D0x55901a792668, typed=3Dfalse, composite=3Dfalse) at
ddl_deparse.c:2460
#3  0x0000559018be2b89 in deparse_CreateStmt (objectId=3D16420,
parsetree=3D0x55901a77d5f8) at ddl_deparse.c:2722
#4  0x0000559018bf72c3 in deparse_simple_command (cmd=3D0x55901a77d590,
include_owner=3D0x7ffe4e611234) at ddl_deparse.c:10019
#5  0x0000559018bf7563 in deparse_utility_command (cmd=3D0x55901a77d590,
include_owner=3Dtrue, verbose_mode=3Dfalse) at ddl_deparse.c:10122
#6  0x0000559018eb650d in publication_deparse_ddl_command_end
(fcinfo=3D0x7ffe4e6113f0) 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 =3D myvarcharin,
    output =3D myvarcharout,
    alignment =3D integer,
    storage =3D main
);

-- want to check updating of a domain over the target type, too
CREATE DOMAIN myvarchardom AS myvarchar;

ALTER TYPE myvarchar SET (storage =3D extended);

3)  invalid type option send
ALTER TYPE myvarchar SET (
    send =3D myvarcharsend,
    receive =3D myvarcharrecv,
    typmod_in =3D varchartypmodin,
    typmod_out =3D varchartypmodout,
    -- these are bogus, but it's safe as long as we don't use the type:
    analyze =3D ts_typanalyze,
    subscript =3D 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) =3D {format_type_be, format_type_be_qualifi=
ed};
.git/rebase-apply/patch:491: indent with spaces.
    appendStringInfoChar(buf, '(');
.git/rebase-apply/patch:492: indent with spaces.
    for (i =3D 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