Re: Support logical replication of DDLs - Mailing list pgsql-hackers
From | li jie |
---|---|
Subject | Re: Support logical replication of DDLs |
Date | |
Msg-id | CAGfChW7H0L6JMU5pc0hGcMgWZiGyCUiZKgxhXO0J-4zBaMSuUw@mail.gmail.com Whole thread Raw |
In response to | Re: Support logical replication of DDLs (vignesh C <vignesh21@gmail.com>) |
Responses |
Re: Support logical replication of DDLs
|
List | pgsql-hackers |
It is worth considering whether temporary objects, such as tables, indexes, and sequences, should be replicated to the subscriber side. Like temp tables, different sessions create their own temp tables. If they are all transferred to the subscriber side, there will inevitably be errors, because there is only one subscription session. I think temporary objects should not be part of replication because they are visible within the session. replicate them over would not make them visible to the user and would not be meaningful. Here is a case: ``` create temp table t1(id int); \c create temp table t1(id int); ``` Thoughts? li jie vignesh C <vignesh21@gmail.com> 于2022年12月8日周四 13:07写道: > > 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. > > > > 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 > >
pgsql-hackers by date: