On Fri, Apr 3, 2020 at 4:52 PM Petr Jelinek <petr@2ndquadrant.com> wrote:
> On 02/04/2020 14:23, Peter Eisentraut wrote:
> > On 2020-03-30 17:42, Amit Langote wrote:
> >> I have updated the comments in apply_handle_tuple_routing() (see 0002)
> >> to better explain what's going on with UPDATE handling. I also
> >> rearranged the tests a bit for clarity.
> >>
> >> Attached updated patches.
> > > Also, the coverage report reveals that in logicalrep_partmap_init(), the
> > patch is mistakenly initializing LogicalRepRelMapContext instead of
> > LogicalRepPartMapContext. (Hmm, how does it even work like that?)
> >
>
> It works because it's just a MemoryContext and it's long lived. I wonder
> if the fix here is to simply remove the LogicalRepPartMapContext...
Actually, there is no LogicalRepPartMapContext in the patches posted
so far, but I have decided to add it in the updated patch. One
advantage beside avoiding confusion is that it might help to tell
memory consumed by the partitions apart from that consumed by the
actual replication targets.
> > I think apart from some of these details, this patch is okay, but I
> > don't have deep experience in the partitioning code, I can just see that
> > it looks like other code elsewhere. Perhaps someone with more knowledge
> > can give this a look as well.
> >
>
> FWIW it looks okay to me as well from perspective of somebody who
> implemented something similar outside of core.
Thanks for giving it a look.
> > About patch 0003, I was talking to some people offline about the name of
> > the option. There was some confusion about using the term "schema". How
> > about naming it "publish_via_partition_root", which also matches the
> > name of the analogous option in pg_dump.
> >
>
> +1 (disclaimer: I was one of the people who discussed this offline)
Okay, I like that too.
I am checking test coverage at the moment and should have the patches
ready by sometime later today.
--
Thank you,
Amit Langote
EnterpriseDB: http://www.enterprisedb.com