MIME-Version: 1.0
References: 
 <CAAD30ULCxqOJp0sffm_y9jNC4BVPYv7Q7_va_JE8qyfRXkfu+g@mail.gmail.com>
 <CAFPTHDaaewvYUznZD1YjUQnvHycgvgMKNK4w=V8Q-8MKTpVDrw@mail.gmail.com>
 <OS3PR01MB6275025C397CC22E446B1F589EBC9@OS3PR01MB6275.jpnprd01.prod.outlook.com>
 <OS0PR01MB5716765B2D38786DA3943E0294809@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAFPTHDbAJPccMcZnOraBy14hM6qBJxYqcRwK6iV6=gtL6VZQwQ@mail.gmail.com>
 <CALDaNm3NUO8ofK64N7HMtNmUP=52R8_jWzrekqAm7m7wqZjwaQ@mail.gmail.com>
 <CALDaNm3XUKfD+nD1AVvSuZyUY_zRk_eyz+Pt9t13N8WXViR6pw@mail.gmail.com>
 <3032112.1679865718@sss.pgh.pa.us>
 <CAA4eK1K3VXfTWXbLADcH81J==7ussvNdqLFHN68sEokDPueu7w@mail.gmail.com>
 <CAA4eK1++Y7a2SQq55DXT6neghZgj3j+pQ74=8zfT3k8Tkdj0ZA@mail.gmail.com>
 <OS0PR01MB571638B29E37EB9A48507DAE94889@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <OS0PR01MB571636F0F1307AB1A868B06A948E9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <OS0PR01MB57169AB2355A90C9FA87DE75948E9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CALDaNm2vBN8oMv-7G=DH5rR-u40JGbR9aP4B6nwr71qw17rPFA@mail.gmail.com>
 <OS0PR01MB571614EE7EEDBF7049DA4EAE94939@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAJpy0uCSCtWwb_LyjXQT65c-UksYhp-4U0QpimtEho_wxjzkog@mail.gmail.com>
 <OS0PR01MB57168840ABFAB9E9BB28A54B94969@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <OS0PR01MB571643602A8B7226B558DBAA94969@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1LyEqEq0XCeCsd9ZmTHyeshHbv=BiRTD=1mmk+YtxwpkQ@mail.gmail.com>
 <OS0PR01MB5716A9E1433DA1FD3EE38069949C9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
In-Reply-To: 
 <OS0PR01MB5716A9E1433DA1FD3EE38069949C9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
From: shveta malik <shveta.malik@gmail.com>
Date: Thu, 20 Apr 2023 09:11:34 +0530
Message-ID: 
 <CAJpy0uDb2mDJtLNFXzUY4911qRZOvj6Q8pu4xFh4BMYBeOSPow@mail.gmail.com>
Subject: Re: Support logical replication of DDLs
To: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, vignesh C <vignesh21@gmail.com>,
	Ajin Cherian <itsajin@gmail.com>,
 "Wei Wang (Fujitsu)" <wangw.fnst@fujitsu.com>,
	Runqi Tian <runqidev@gmail.com>, Peter Smith <smithpb2250@gmail.com>,
 Tom Lane <tgl@sss.pgh.pa.us>,
	li jie <ggysxcq@gmail.com>, Dilip Kumar <dilipbalaut@gmail.com>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
 Masahiko Sawada <sawada.mshk@gmail.com>,
	Japin Li <japinli@hotmail.com>, rajesh singarapu <rajesh.rs0541@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Zheng Li <zhengli10@gmail.com>,
	shveta malik <shveta.malik@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAJpy0uDb2mDJtLNFXzUY4911qRZOvj6Q8pu4xFh4BMYBeOSPow%40mail.gmail.com>
Precedence: bulk

On Mon, Apr 17, 2023 at 5:32=E2=80=AFPM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Monday, April 10, 2023 7:20 PM Amit Kapila <amit.kapila16@gmail.com> w=
rote:
> >
> > On Fri, Apr 7, 2023 at 8:52=E2=80=AFAM houzj.fnst@fujitsu.com
> > <houzj.fnst@fujitsu.com> wrote:
> > >
> > > Sorry, there was a miss when rebasing the patch which could cause the
> > > CFbot to fail and here is the correct patch set.
> > >
> >
> > I see the following note in the patch: "Note: For ATTACH/DETACH PARTITI=
ON,
> > we haven't added extra logic on the subscriber to handle the case where=
 the
> > table on the publisher is a PARTITIONED TABLE while the target table on=
 the
> > subscriber side is a NORMAL table. We will research this more and impro=
ve it
> > later." and wonder what should we do about this. I can think of the fol=
lowing
> > possibilities: (a) Convert a non-partitioned table to a partitioned one=
 and then
> > attach the partition; (b) Add the partition as a separate new table; (c=
) give an
> > error that table types mismatch. For Detach partition, I don't see much
> > possibility than giving an error that no such partition exists or somet=
hing like
> > that. Even for the Attach operation, I prefer (c) as the other options =
don't seem
> > logical to me and may add more complexity to this work.
> >
> > Thoughts?
>
> I also think option (c) makes sense and is same as the latest patch's beh=
avior.
>
> Attach the new version patch set which include the following changes:
>

Few comments for ddl_deparse.c in patch dated April17:

1) append_format_string()
I think we need to have 'Assert(sub_fmt)' here like we have it in all
other similar functions (append_bool_object, append_object_object,
...)

2) append_object_to_format_string()
here we have code piece :
        if (sub_fmt =3D=3D NULL || tree->fmtinfo =3D=3D NULL)
                return sub_fmt;
but sub_fmt will never be null when we reach this function as all its
callers assert for null sub_fmt. So that means when tree->fmtinfo is
null, we end up returning sub_fmt as it is, instead of extracting
object name from that. Is this intended?

3) We can remove extra spaces after full-stop in the comment below
/*
 * Deparse a ColumnDef node within a typed table creation. This is simpler
 * than the regular case, because we don't have to emit the type declaratio=
n,
 * collation, or default.  Here we only return something if the column is b=
eing
 * declared NOT NULL.
 ...
 deparse_ColumnDef_typed()


4) These functions are not being used, do we need to retain these in this p=
atch?
deparse_Type_Storage()
deparse_Type_Receive()
deparse_Type_Send()
deparse_Type_Typmod_In()
deparse_Type_Typmod_Out()
deparse_Type_Analyze()
deparse_Type_Subscript()

5) deparse_AlterRelation()
We have below variable initialized to false in the beginning
'bool            istype =3D false;'
And then we have many conditional codes using the above, eg: istype ?
"ATTRIBUTE" : "COLUMN".  We are not changing 'istype' anywhere and it
is hard-coded in the beginning. It means there are parts of code in
this function which will never be htt (written for 'istype=3Dtrue' case)
, so why do we need this variable and conditional code around it?


6) There are plenty of places where we use 'append_not_present'
without using 'append_null_object'.
Do we need to have 'append_null_object' along with
'append_not_present' at these places?


7) deparse_utility_command():
Rather than inject --> Rather than injecting

thanks
Shveta