Re: Support logical replication of DDLs

On Mon, Apr 17, 2023 at 5:32 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Monday, April 10, 2023 7:20 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Apr 7, 2023 at 8:52 AM 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 PARTITION,
> > 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 improve it
> > later." and wonder what should we do about this. I can think of the following
> > 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 something 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 behavior.
>
> 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 == NULL || tree->fmtinfo == 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 declaration,
 * collation, or default.  Here we only return something if the column is being
 * declared NOT NULL.
 ...
 deparse_ColumnDef_typed()


4) These functions are not being used, do we need to retain these in this patch?
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 = 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=true' 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