Re: Support logical replication of DDLs

On Thu, Apr 20, 2023 at 9:11 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
>
> 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,
> ...)
>

+1.

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

Which full-stop you are referring to here first or second? I see there
is a tab after the first one which should be changed to space.

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

I don't think we need to retain these. And, it seems they are already removed.

> 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?
>

I don't see any usage of istype. We should simply remove the use of
istype and use "COLUMN" directly.

>
> 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?
>

What is the difference if we add a null object or not before not_present?

--
With Regards,
Amit Kapila.