Subject: Re: adding partitioned tables to publications
To: Amit Langote <amitlangote09@gmail.com>
References: 
 <CA+HiwqH=Y85vRK3mOdjEkqFK+E=ST=eQiHdpj43L=_eJMOOznQ@mail.gmail.com>
 <b6d8519b-353f-1949-79b5-460a6712e20f@2ndquadrant.com>
 <CA+HiwqHScTtXD97yC_ZRLk7TXf+0HYH+UeDvACf2Q-LBH-2ctw@mail.gmail.com>
 <CA+FpmFfKmsNww_C5hWU-=wg9h3cyV7vZcCUKWrrhtLkteUtY4w@mail.gmail.com>
 <CA+HiwqE2ie6_T01fMzsf7c5=hCGoVdj2yTStCJ78zOmayMVWbw@mail.gmail.com>
 <825b31ea-ad9f-21d1-29b6-2a494513458d@2ndquadrant.com>
 <CA+HiwqEqm3xvnMHjstkrECrpVv9R7YgYYpRHHVrffSvA9Zrf9g@mail.gmail.com>
 <CA+HiwqGiFpkHxx0mc2zpdJLGzfqEaqsdqUZurNto9kbkwzcQ1w@mail.gmail.com>
 <CA+FpmFfve1TtLvj5CwpgEZrjNiFG-EWh4ywpbtrB7-UTkZo2gg@mail.gmail.com>
 <CA+HiwqHfV73E4aJxX6d79xSEZjp+stFKnD0+7OktPw2_tD=pYg@mail.gmail.com>
 <73a1877a-6f1d-9f2e-da51-bc2eb57e07d5@2ndquadrant.com>
 <CA+HiwqFjo=vCXDdkwprf6ipaVeegCpW79n-Ds8BK5cNQsSey2A@mail.gmail.com>
 <d760bb60-563c-dd01-9903-f9337d50e8a6@2ndquadrant.com>
 <CA+HiwqFBni-cXy5NBN0PdwxidXgcCEio0N_AOnOM0pR2dMWUyA@mail.gmail.com>
 <CA+HiwqGVbp0yh-7-FscZiYu1B_CJjaDZRTYkPTjnjpUgSzE6RA@mail.gmail.com>
Cc: Rafia Sabih <rafia.pghackers@gmail.com>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>
From: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Organization: 2ndQuadrant
Message-ID: <b8f257af-3fcd-65b9-088c-0248d82e4e99@2ndquadrant.com>
Date: Thu, 19 Mar 2020 15:17:57 +0100
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:52.0)
 Gecko/20100101 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: 
 <CA+HiwqGVbp0yh-7-FscZiYu1B_CJjaDZRTYkPTjnjpUgSzE6RA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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