MIME-Version: 1.0
References: 
 <CAAD30ULtoGp8L_GKbV15Wnm+X5r=SE7MOnYHuqBr396m26jJSA@mail.gmail.com>
 <202203162206.7spggyktx63e@alvherre.pgsql>
 <CAAD30UKRUusq8JyyHzAv71=ncN22OE8OkOOyAWvRHW3wXNjyyA@mail.gmail.com>
 <CAAD30UKTp87+kvGZYL3M2Suxq=WEvFUG24ZRT0yT9rqdkP=uMA@mail.gmail.com>
 <MEYP282MB1669863D5C31D7F6A1D996D8B6139@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM>
 <CAAD30UKc=GiGQzE8H7+Ofo18hwMOfK4qUm_KUyw6c09q4JvA5Q@mail.gmail.com>
 <MEYP282MB16691E383140844437FB0633B6139@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM>
 <CAAD30U+ZTBXLH0wWsW9+Zu2RECGKeaQNynLs7wKA0o86w8C-fw@mail.gmail.com>
 <MEYP282MB166926E46397CBFC113B4A7EB6189@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM>
 <CAA4eK1J4AekmEKgmfp6e-zZz4M02m7w7uxvC2tjqmjF-LDSGDA@mail.gmail.com>
 <CAAD30UKvv5=k6BY+JAF1fWzrYNbGcB0DEdNi1FMokULzOwSxcQ@mail.gmail.com>
 <CAAD30U+CRgUgkAg33KzNBKwCbsgiSc5z3NYvxNzEfS0Zg2S1WA@mail.gmail.com>
 <CAD21AoAv_wsBEK8jcqjBpatspiP=5E+qLokw9zCESBSvCAiRMg@mail.gmail.com>
 <CAAD30UK6T8bfW1JMaSSRDSynB6W05HjNrmvSp+tvXp-jdu9xFQ@mail.gmail.com>
 <CAA4eK1JQhz4y-1rYxwFxHYEAN-1JKeO0iT+Nip0N7jJUj_g7RA@mail.gmail.com>
 <CAD21AoCnGwx2F+Ph3dpoJVq0YR8ke3P59XCs439pW=BRfdzgTQ@mail.gmail.com>
 <OS0PR01MB571695EDF9EAB2422FBF2C1094DE9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAAD30ULe-cZTELQJbcAahsOFoUO-ftMxorwBTmj7uYK=_=mwxg@mail.gmail.com>
 <OS0PR01MB571684CBF660D05B63B4412C94AB9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAAD30U+oi6e6Vh_zAzhuXzkqUhagmLGD+_iyn2N9w_sNRKsoag@mail.gmail.com>
 <CAA4eK1JKK9LACPovjogS-LThQBscwkrxBy9RuA6aHFP=vTGjtg@mail.gmail.com>
 <CAAD30U+wDPDFzUoPkSg2WYMNCXWNc8wa7GYB1Tzh_2PNUBsEHA@mail.gmail.com>
 <OS0PR01MB5716DBA31AC57163D2CF465E94B09@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <OS0PR01MB5716B1526C2EDA66907E733B94B39@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1K88SMoBq=DRA4XU-F3FG6qyzCjGMMKsPpcRBPRcrELrw@mail.gmail.com>
 <CAA4eK1+3ac2qXZZYfdiobuOF17e60v-fiFMG7HfJx93WbEkFhQ@mail.gmail.com>
 <OS0PR01MB571676298B814F7F5690D92994BB9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
In-Reply-To: 
 <OS0PR01MB571676298B814F7F5690D92994BB9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Wed, 29 Jun 2022 16:18:12 +0530
Message-ID: 
 <CAA4eK1L_Duk83otuAwtvGo21QWpKk4ptBib60H8wkFBxcPfcBw@mail.gmail.com>
Subject: Re: Support logical replication of DDLs
To: "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>, Japin Li <japinli@hotmail.com>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
 Dilip Kumar <dilipbalaut@gmail.com>,
	rajesh singarapu <rajesh.rs0541@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Zheng Li <zhengli10@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAA4eK1L_Duk83otuAwtvGo21QWpKk4ptBib60H8wkFBxcPfcBw%40mail.gmail.com>
Precedence: bulk

On Wed, Jun 29, 2022 at 3:24 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Wednesday, June 29, 2022 11:07 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Jun 28, 2022 at 5:43 PM Amit Kapila <amit.kapila16@gmail.com>
> > wrote:
> > >
> > > 5.
> > > +static ObjTree *
> > > +deparse_CreateStmt(Oid objectId, Node *parsetree)
> > > {
> > > ...
> > > + tmp = new_objtree_VA("TABLESPACE %{tablespace}I", 0); if
> > > + (node->tablespacename) append_string_object(tmp, "tablespace",
> > > + node->tablespacename); else { append_null_object(tmp, "tablespace");
> > > + append_bool_object(tmp, "present", false); }
> > > + append_object_object(createStmt, "tablespace", tmp);
> > > ...
> > > }
> > >
> > > Why do we need to append the objects (tablespace, with clause, etc.)
> > > when they are not present in the actual CREATE TABLE statement? The
> > > reason to ask this is that this makes the string that we want to send
> > > downstream much longer than the actual statement given by the user on
> > > the publisher.
> > >
> >
> > After thinking some more on this, it seems the user may want to optionally
> > change some of these attributes, for example, on the subscriber, it may want to
> > associate the table with a different tablespace. I think to address that we can
> > append these additional attributes optionally, say via an additional parameter
> > (append_all_options/append_all_attributes or something like that) in exposed
> > APIs like deparse_utility_command().
>
> I agree and will research this part.
>

Okay, note that similar handling would be required at other places
like deparse_ColumnDef. Few other comments on
v9-0001-Functions-to-deparse-DDL-commands.

1.
+static ObjElem *new_bool_object(bool value);
+static ObjElem *new_string_object(char *value);
+static ObjElem *new_object_object(ObjTree *value);
+static ObjElem *new_array_object(List *array);
+static ObjElem *new_integer_object(int64 value);
+static ObjElem *new_float_object(float8 value);

Here, new_object_object() seems to be declared out-of-order (not in
sync with its order of definition). Similarly, see all other append_*
functions.

2. The function printTypmod in ddl_deparse.c appears to be the same as
the function with the same name in format_type.c. If so, isn't it
better to have a single copy of that function?

3. As I pointed out yesterday, there is some similarity between
format_type_extended and format_type_detailed. Can we try to extract
common functionality? If this is feasible, it is better to do this as
a separate patch. Also, this can obviate the need to expose
printTypmod from format_type.c. I am not really sure if this will be
better than the current one or not but it seems worth trying.

4.
new_objtree_VA()
{
...
switch (type)
+ {
+ case ObjTypeBool:
+ elem = new_bool_object(va_arg(args, int));
+ break;
+ case ObjTypeString:
+ elem = new_string_object(va_arg(args, char *));
+ break;
+ case ObjTypeObject:
+ elem = new_object_object(va_arg(args, ObjTree *));
+ break;
+ case ObjTypeArray:
+ elem = new_array_object(va_arg(args, List *));
+ break;
+ case ObjTypeInteger:
+ elem = new_integer_object(va_arg(args, int64));
+ break;
+ case ObjTypeFloat:
+ elem = new_float_object(va_arg(args, double));
+ break;
+ case ObjTypeNull:
+ /* Null params don't have a value (obviously) */
+ elem = new_null_object();
...

I feel here ObjType's should be handled in the same order as they are
defined in ObjType.

5. There appears to be common code among node_*_object() functions.
Can we just have one function instead new_object(ObjType, void *val)?
In the function based on type, we can typecast the value. Is there a
reason why that won't be better than current one?

6.
deparse_ColumnDef()
{
...
/* Composite types use a slightly simpler format string */
+ if (composite)
+ column = new_objtree_VA("%{name}I %{coltype}T %{collation}s",
+ 3,
+ "type", ObjTypeString, "column",
+ "name", ObjTypeString, coldef->colname,
+ "coltype", ObjTypeObject,
+ new_objtree_for_type(typid, typmod));
+ else
+ column = new_objtree_VA("%{name}I %{coltype}T %{default}s
%{not_null}s %{collation}s",
+ 3,
+ "type", ObjTypeString, "column",
+ "name", ObjTypeString, coldef->colname,
+ "coltype", ObjTypeObject,
+ new_objtree_for_type(typid, typmod));
...
}

To avoid using the same arguments, we can define fmtstr for composite
and non-composite types as the patch is doing in deparse_CreateStmt().

7. It is not clear from comments or otherwise what should be
considered for default format string in functions like
deparse_ColumnDef() or deparse_CreateStmt().

8.
+ * FIXME --- actually, what about default values?
+ */
+static ObjTree *
+deparse_ColumnDef_typed(Relation relation, List *dpcontext, ColumnDef *coldef)

I think we need to handle default values for this FIXME.

-- 
With Regards,
Amit Kapila.