Re: Support logical replication of DDLs

On Wed, 7 Dec 2022 at 17:50, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> I think this patch is split badly.
>
> You have:
>
> 0001 an enormous patch including some required infrastructure, plus the
> DDL deparsing bits themselves.
>
> 0002 another enormous (though not as much) patch, this time for
> DDL replication using the above.
>
> 0003 a bugfix for 0001, which includes changes in both the
> infrastructure and the deparsing bits.
>
> 0004 test stuff for 0002.
>
> 0005 Another bugfix for 0001
>
> 0006 Another bugfix for 0001
>
> As presented, I think it has very little chance of being reviewed
> usefully. A better way to go about this, I think, would be:
>
> 0001 - infrastructure bits to support the DDL deparsing parts (all these
> new functions in ruleutils.c, sequence.c, etc). That means, everything
> (?) that's currently in your 0001 except ddl_deparse.c and friends.
> Clearly there are several independent changes here; maybe it is possible
> to break it down even further. This patch or these patches should also
> include the parts of 0003, 0005, 0006 that require changes outside of
> ddl_deparse.c.
> I expect that this patch should be fairly small.
>
> 0002 - ddl_deparse.c and its very close friends. This should not have
> any impact on places such as ruleutils.c, sequence.c, etc. The parts of
> the bugfixes (0001, 0005, 0006) that touch this could should be merged
> here as well; there's no reason to have them as separate patches. Some
> test code should be here also, though it probably doesn't need to aim to
> be complete.
> This one is likely to be very large, but also self-contained.
>
> 0003 - ddlmessage.c and friends. I understand that DDL-messaging is
> supporting infrastructure for DDL replication; I think it should be its
> own patch. Probably include its own simple-ish test bits.
> Not a very large patch.
>
> 0004 - DDL replication proper, including 0004.
> Probably not a very large patch either, not sure.
>
>
> Some review comments, just skimming:
> - 0002 adds some functions to event_trigger.c, but that doesn't seem to
> be their place. Maybe some new file in src/backend/replication/logical
> would make more sense.
>
> - publication_deparse_ddl_command_end has a long strcmp() list; why?
> Maybe change things so that it compares some object type enum instead.
>
> - CreatePublication has a long list of command tags; is that good?
> Maybe it'd be better to annotate the list in cmdtaglist.h somehow.
>
> - The change in pg_dump's getPublications needs updated to 16.
>
> - Don't "git add" src/bin/pg_waldump/logicalddlmsgdesc.c, just update
> its Makefile and meson.build
>
> - I think psql's \dRp should not have the new column at the end.
> Maybe one of:
> + Name | Owner | DDL | All tables | Inserts | Updates | Deletes | Truncates | Via root
> + Name | Owner | All tables | DDL | Inserts | Updates | Deletes | Truncates | Via root
> + Name | Owner | All tables | Inserts | Updates | Deletes | Truncates | DDL | Via root
> (I would not add the "s" at the end of that column title, also).

Thanks for the comments, these comments will make the patch reviewing easier.
There are a couple of review comments [1] and [2] which are spread
across the code, it will be difficult to handle this after
restructuring of the patch as the comments are spread across the code
in the patch. So we will handle [1] and [2] first and then work on
restructuring work suggested by you.

[1] - https://www.postgresql.org/message-id/CAHut%2BPsERMFwO8oK3LFH_3CRG%2B512T%2Bay_viWzrgNetbH2MwxA%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/CAHut%2BPuxo_kq2toicNK_BQdeccK3REGW-Xv8tVauFvTNku6V-w%40mail.gmail.com

Regards,
Vignesh