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.
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?
In apply_handle_do_insert(), the argument localslot should probably be
remoteslot.
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.
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?
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.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
