Re: Support logical replication of DDLs - Mailing list pgsql-hackers

On Fri, Feb 10, 2023 at 8:23 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Thu, Feb 9, 2023 at 6:55 PM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> (v67)
>
> I have some questions about adding the infrastructure for DDL deparsing.
>
> Apart from the changes made by 0001 patch to add infrastructure for
> DDL deparsing, 0002 patch seems to add some variables that are not
> used in 0002 patch:
>
> @@ -2055,6 +2055,7 @@ typedef struct AlterTableStmt
>         List       *cmds;                       /* list of subcommands */
>         ObjectType      objtype;                /* type of object */
>         bool            missing_ok;             /* skip error if table
> missing */
> +       bool        table_like;         /* internally generated for
> TableLikeClause */
>  } AlterTableStmt;
>
> @@ -39,6 +40,7 @@ typedef struct CollectedATSubcmd
>  {
>         ObjectAddress address;          /* affected column,
> constraint, index, ... */
>         Node       *parsetree;
> +       char       *usingexpr;
>  } CollectedATSubcmd;
>
>  typedef struct CollectedCommand
> @@ -62,6 +64,7 @@ typedef struct CollectedCommand
>                 {
>                         Oid                     objectId;
>                         Oid                     classId;
> +                       bool            rewrite;
>                         List       *subcmds;
>                 }                       alterTable;
>
> These three variables are used in 0006 patch.
>

Hmm, then it should be better to move these to 0006 patch.

> Looking at 0006 patch (Support DDL replication), it seems to me that
> it includes not only DDL replication support but also changes for the
> event trigger. For instance, the patch adds
> EventTriggerAlterTypeStart() and EventTriggerAlterTypeEnd(). If these
> changes are required for DDL deparse, should we include them in 0001
> patch? Perhaps the same is true for
> EventTriggerCollectCreatePublication() and friends. IIUC the DDL
> deparse and DDL replication are independent features, so I think 0006
> patch should not include any changes for DDL deparse infrastructure.
>

AFAICS, these are required for DDL replication, so not sure moving
them would be of any help.

> Also, 0003 and 0006 patches introduce SCT_Create/AlterPublication and
> change DDL deparse so that it deparse CREATE/ALTER PUBLICATION in a
> different way from other simple commands. Is there any reason for
> that? I mean, since EventTriggerCollectCreatePublication() collects
> the information from the parse tree, I wonder if we could use
> SCT_Simple for them.
>

Right, I think we should try a bit harder to avoid adding new
CollectedCommandTypes. Is there a reason that we can't retrieve the
additional information required to form the command string from system
catalogs or parsetree?

-- 
With Regards,
Amit Kapila.