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.

Modified

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

Modified, I have currently kept the testing of deparse as a separate
patch, we are planning to add more tests to it. We can later merge it
to 0002 if required or keep it as 0003 if it is too large.

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

Modified

> 0004 - DDL replication proper, including 0004.
> Probably not a very large patch either, not sure.

Modified

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

Modified

> - publication_deparse_ddl_command_end has a long strcmp() list; why?
> Maybe change things so that it compares some object type enum instead.

Modified wherever possible

> - 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.
Modified

> - Don't "git add" src/bin/pg_waldump/logicalddlmsgdesc.c, just update
> its Makefile and meson.build

Modified

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

Modified it to:
Name | Owner | All tables | DDL | Inserts | Updates | Deletes |
Truncates | Via root

These issues were addressed as part of v55 at [1], v56 at [2] and v58
at [3] posted.

[1] - https://www.postgresql.org/message-id/CALDaNm2V6YL6H4P9ZT95Ua_RDJaeDTUf6V0UDfrz4_vxhM5pMg%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/CALDaNm0uXh%3DRgGD%3DoB1p83GONb5%3DL2n3nbpiLGVaMd57TimdZA%40mail.gmail.com
[3] - https://www.postgresql.org/message-id/CAAD30ULrZB-RNmuD3NMr1jGNUt15ZpPgFdFRX53HbcAB76hefw%40mail.gmail.com

Regards,
Vignesh