Subject: Re: Fix handling of unlogged tables in FOR ALL TABLES publications
References: <f3f151f7-c4dd-1646-b998-f60bd6217dd3@2ndquadrant.com>
 <59e5a734-9e06-1035-385b-6267175819aa@lab.ntt.co.jp>
 <20190314.150328.172224727.horiguchi.kyotaro@lab.ntt.co.jp>
From: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Message-ID: <26bfa053-3fb2-ad1d-efbb-7c930b41c0fd@lab.ntt.co.jp>
Date: Thu, 14 Mar 2019 15:31:03 +0900
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.5.3
MIME-Version: 1.0
In-Reply-To: <20190314.150328.172224727.horiguchi.kyotaro@lab.ntt.co.jp>
Content-Type: text/plain; charset=windows-1252
Content-Language: en-US
Content-Transfer-Encoding: 7bit
To: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
Cc: peter.eisentraut@2ndquadrant.com, pgsql-hackers@postgresql.org
Precedence: bulk

On 2019/03/14 15:03, Kyotaro HORIGUCHI wrote:
> At Thu, 14 Mar 2019 11:30:12 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in <59e5a734-9e06-1035-385b-6267175819aa@lab.ntt.co.jp>
>> On 2019/03/13 21:03, Peter Eisentraut wrote:
>>> If a FOR ALL TABLES publication exists, unlogged tables are ignored
>>> for publishing changes.  But CheckCmdReplicaIdentity() would still
>>> check in that case that such a table has a replica identity set before
>>> accepting updates.  That is useless, so check first whether the given
>>> table is publishable and skip the check if not.
>>>
>>> Example:
>>>
>>> CREATE PUBLICATION pub FOR ALL TABLES;
>>> CREATE UNLOGGED TABLE logical_replication_test AS SELECT 1 AS number;
>>> UPDATE logical_replication_test SET number = 2;
>>> ERROR:  cannot update table "logical_replication_test" because it does
>>> not have a replica identity and publishes updates
>>>
>>> Patch attached.
>>
>> An email on -bugs earlier this morning complains of the same problem but
>> for temporary tables.
>>
>> https://www.postgresql.org/message-id/CAHOFxGr%3DmqPZXbAuoR7Nbq-bU4HxqVWHbTTUy5%3DPKQut_F0%3DXA%40mail.gmail.com
>>
>> It seems your patch fixes their case too.
> 
> Is it the right thing that GetRelationPublicationsActions sets
> wrong rd_publicatons for the relations?

Actually, after applying Peter's patch, maybe we should add an
Assert(is_publishable_relation(relation)) at the top of
GetRelationPublicationActions(), also adding a line in the function header
comment that callers must ensure that.  There's only one caller at the
moment anyway, which Peter's patch is fixing to ensure that.

Thanks,
Amit