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.
> 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.
--
Thank you,
Amit Langote
EnterpriseDB: http://www.enterprisedb.com
