From: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
To: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
Cc: peter.eisentraut@2ndquadrant.com
Cc: pgsql-hackers@postgresql.org
Subject: Re: Fix handling of unlogged tables in FOR ALL TABLES publications
Date: Thu, 14 Mar 2019 15:31:03 +0900
Message-ID: <26bfa053-3fb2-ad1d-efbb-7c930b41c0fd@lab.ntt.co.jp> (raw)
In-Reply-To: <20190314.150328.172224727.horiguchi.kyotaro@lab.ntt.co.jp>
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>
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%40ma...
>>
>> 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: Langote_Amit_f8@lab.ntt.co.jp, horiguchi.kyotaro@lab.ntt.co.jp, peter.eisentraut@2ndquadrant.com
Subject: Re: Fix handling of unlogged tables in FOR ALL TABLES publications
In-Reply-To: <26bfa053-3fb2-ad1d-efbb-7c930b41c0fd@lab.ntt.co.jp>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox