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+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>
 <b8f257af-3fcd-65b9-088c-0248d82e4e99@2ndquadrant.com>
 <CA+HiwqFZbbJzAZgTjqbRarp_xeqyymuNmGg8Q5tDKTZbk3=c2A@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: <c0d69728-c3d8-ede8-ad31-941b2bbaff66@2ndquadrant.com>
Date: Wed, 25 Mar 2020 13:29:52 +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+HiwqFZbbJzAZgTjqbRarp_xeqyymuNmGg8Q5tDKTZbk3=c2A@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-23 06:02, Amit Langote wrote:
> Okay, added some tests.
> 
> Attached updated patches.

I have committed the worker.c refactoring patch.

"Add subscription support to replicate into partitioned tables" still 
has lacking test coverage.  Your changes in relation.c are not exercised 
at all because the partitioned table branch in apply_handle_update() is 
never taken.  This is critical and tricky code, so I would look for 
significant testing.

The code looks okay to me.  I would remove this code

+       memset(entry->attrmap->attnums, -1,
+              entry->attrmap->maplen * sizeof(AttrNumber));

because the entries are explicitly filled right after anyway, and 
filling the bytes with -1 has an unclear effect.  There is also 
seemingly some fishiness in this code around whether attribute numbers 
are zero- or one-based.  Perhaps this could be documented briefly. 
Maybe I'm misunderstanding something.

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