Re: adding partitioned tables to publications

On Thu, Mar 19, 2020 at 11:18 PM Peter Eisentraut
<peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
> On 2020-03-18 08:33, Amit Langote wrote:
> > By the way, I have rebased the patches, although maybe you've got your
> > own copies; attached.
>
> Looking through 0002 and 0003 now.
>
> The structure looks generally good.

Thanks for the review.

> In 0002, the naming of apply_handle_insert() vs.
> apply_handle_do_insert() etc. seems a bit prone to confusion. How about
> something like apply_handle_insert_internal()? Also, should we put each
> of those internal functions next to their internal function instead of
> in a separate group like you have it?

Sure.

> In apply_handle_do_insert(), the argument localslot should probably be
> remoteslot.

You're right, fixed.

> In apply_handle_do_delete(), the ExecOpenIndices() call was moved to a
> different location relative to the rest of the code. That was probably
> not intended.

Fixed.

> In 0003, you have /* TODO, use inverse lookup hashtable? */. Is this
> something you plan to address in this cycle, or is that more for future
> generations?

Sorry, this is simply a copy-paste from logicalrep_relmap_invalidate_cb().

> 0003 could use some more tests. The one test that you adjusted just
> ensures the data goes somewhere instead of being rejected, but there are
> no tests that check whether it ends up in the right partition, whether
> cross-partition updates work etc.

Okay, added some tests.

Attached updated patches.

--
Thank you,
Amit