Re: Support logical replication of DDLs - Mailing list pgsql-hackers
From | vignesh C |
---|---|
Subject | Re: Support logical replication of DDLs |
Date | |
Msg-id | CALDaNm0-TpyigxOu_=_7x0X4QM5pCXH7ZEkUDEPyJYy6XSz_qg@mail.gmail.com Whole thread Raw |
In response to | Re: Support logical replication of DDLs (Alvaro Herrera <alvherre@alvh.no-ip.org>) |
List | pgsql-hackers |
On Wed, 7 Dec 2022 at 17:50, Alvaro Herrera <alvherre@alvh.no-ip.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
pgsql-hackers by date: