Re: Support logical replication of DDLs

On Fri, Feb 10, 2023 at 8:23 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Thu, Feb 9, 2023 at 6:55 PM Ajin Cherian <itsajin(at)gmail(dot)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.