On Thu, Mar 19, 2020 at 11:18 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.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