On Thu, Mar 26, 2020 at 11:23 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Wed, Mar 25, 2020 at 9:29 PM Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
> > On 2020-03-23 06:02, Amit Langote wrote:
> > > Okay, added some tests.
> > >
> > > Attached updated patches.
> >
> > I have committed the worker.c refactoring patch.
> >
> > "Add subscription support to replicate into partitioned tables" still
> > has lacking test coverage. Your changes in relation.c are not exercised
> > at all because the partitioned table branch in apply_handle_update() is
> > never taken. This is critical and tricky code, so I would look for
> > significant testing.
>
> While trying some tests around the code you mentioned, I found what
> looks like a bug, which looking into now.
Turns out the code in apply_handle_tuple_routing() for the UPDATE
message was somewhat bogus, which fixed in the updated version. I
ended up with anothing refactoring patch, which attached as 0001.
It appears to me that the tests now seem enough to cover
apply_handle_tuple_routing(), although more could still be added.
> > The code looks okay to me. I would remove this code
> >
> > + memset(entry->attrmap->attnums, -1,
> > + entry->attrmap->maplen * sizeof(AttrNumber));
> >
> > because the entries are explicitly filled right after anyway, and
> > filling the bytes with -1 has an unclear effect. There is also
> > seemingly some fishiness in this code around whether attribute numbers
> > are zero- or one-based. Perhaps this could be documented briefly.
> > Maybe I'm misunderstanding something.
>
> Will check and fix as necessary.
Removed that memset. I have added a comment about one- vs. zero-based
indexes contained in the maps coming from two different modules, viz.
tuple routing and logical replication, resp.
--
Thank you,
Amit Langote
EnterpriseDB: http://www.enterprisedb.com