Re: adding partitioned tables to publications

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