Subject: Re: adding partitioned tables to publications
To: Amit Langote <amitlangote09@gmail.com>
References: 
 <CA+HiwqH=Y85vRK3mOdjEkqFK+E=ST=eQiHdpj43L=_eJMOOznQ@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>
 <b8f257af-3fcd-65b9-088c-0248d82e4e99@2ndquadrant.com>
 <CA+HiwqFZbbJzAZgTjqbRarp_xeqyymuNmGg8Q5tDKTZbk3=c2A@mail.gmail.com>
 <c0d69728-c3d8-ede8-ad31-941b2bbaff66@2ndquadrant.com>
 <CA+HiwqE+6gNgr-BVYeCMT38naRi9cJaanJb7+FCNsX9XgMwHsQ@mail.gmail.com>
 <CA+HiwqFjYE5anArxvkjr37AQMd52L-LZtz9Ld2QrLQ3YfcYhTw@mail.gmail.com>
 <CA+HiwqE+z2=wLv3a41h7JopcnUZAiFPeaiQyxZeGQ62QV3vsPQ@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: <4f999141-97c8-c968-a726-0a1ec3e36e47@2ndquadrant.com>
Date: Thu, 2 Apr 2020 14:23:27 +0200
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+HiwqE+z2=wLv3a41h7JopcnUZAiFPeaiQyxZeGQ62QV3vsPQ@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-30 17:42, Amit Langote wrote:
> I have updated the comments in apply_handle_tuple_routing() (see 0002)
> to better explain what's going on with UPDATE handling.  I also
> rearranged the tests a bit for clarity.
> 
> Attached updated patches.

Test coverage for 0002 is still a bit lacking.  Please do a coverage 
build yourself and get at least one test case to exercise every branch 
in apply_handle_tuple_routing().  Right now, I don't see any coverage 
for updates without attribute remapping and updates that don't move to a 
new partition.

Also, the coverage report reveals that in logicalrep_partmap_init(), the 
patch is mistakenly initializing LogicalRepRelMapContext instead of 
LogicalRepPartMapContext.  (Hmm, how does it even work like that?)

I think apart from some of these details, this patch is okay, but I 
don't have deep experience in the partitioning code, I can just see that 
it looks like other code elsewhere.  Perhaps someone with more knowledge 
can give this a look as well.

About patch 0003, I was talking to some people offline about the name of 
the option.  There was some confusion about using the term "schema". 
How about naming it "publish_via_partition_root", which also matches the 
name of the analogous option in pg_dump.

Code coverage here could also be improved.  A lot of the new code in 
pgoutput.c is not tested.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services