Re: Support logical replication of DDLs

On Wed, 12 Oct 2022 at 11:38, Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Tue, Oct 11, 2022 at 7:00 PM Ajin Cherian <itsajin@gmail.com> wrote:
> >
>
> I was going through 0001 patch and I have a few comments/suggestions.
>
> 1.
>
> @@ -6001,7 +6001,7 @@ getObjectIdentityParts(const ObjectAddress *object,
>                  transformType = format_type_be_qualified(transform->trftype);
>                  transformLang = get_language_name(transform->trflang, false);
>
> -                appendStringInfo(&buffer, "for %s on language %s",
> +                appendStringInfo(&buffer, "for %s language %s",
>                                   transformType,
>                                   transformLang);
>
>
> How is this change related to this patch?

This change is required for ddl of transform commands, we have added a
note for the same in the patch:
Removed an undesirable 'on' from the identity string for TRANSFORM in
getObjectIdentityParts. This is needed to make deparse of DROP
TRANSFORM command work since 'on' is not present in the current DROP
TRANSFORM syntax. For example, the correct syntax is
drop transform trf for int language sql;
instead of
drop transform trf for int on language sql;

> 2.
> +typedef struct ObjTree
> +{
> +    slist_head    params;
> +    int            numParams;
> +    StringInfo    fmtinfo;
> +    bool        present;
> +} ObjTree;
> +
> +typedef struct ObjElem
> +{
> +    char       *name;
> +    ObjType        objtype;
> +
> +    union
> +    {
> +        bool        boolean;
> +        char       *string;
> +        int64        integer;
> +        float8        flt;
> +        ObjTree       *object;
> +        List       *array;
> +    } value;
> +    slist_node    node;
> +} ObjElem;
>
> It would be good to explain these structure members from readability pov.

Modified

> 3.
>
> +
> +bool verbose = true;
> +
>
> I do not understand the usage of such global variables.  Even if it is
> required, add some comments to explain the purpose of it.

Modified

>
> 4.
> +/*
> + * Given a CollectedCommand, return a JSON representation of it.
> + *
> + * The command is expanded fully, so that there are no ambiguities even in the
> + * face of search_path changes.
> + */
> +Datum
> +ddl_deparse_to_json(PG_FUNCTION_ARGS)
> +{
>
> It will be nice to have a test case for this utility function.

We are discussing how to test in a separate thread at [1]. We will
implement accordingly once it is concluded. This comment will be
handled at that time.
[1] -
https://www.postgresql.org/message-id/flat/CAH8n8_jMTunxxtP4L-3tc%3DGNamg%3Dmg1X%3DtgHr9CqqjjzFLwQng%40mail.gmail.com

This patch also includes changes for replication of:
CREATE/ALTER/DROP STATISTICS
and pgindent fixes for the ddl replication code.
Thanks for the comments, the attached v30 patch has the changes for the same.

Regards,
Vignesh