Re: Fix handling of unlogged tables in FOR ALL TABLES publications

On 2019/03/14 15:03, Kyotaro HORIGUCHI wrote:
> At Thu, 14 Mar 2019 11:30:12 +0900, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote in <59e5a734-9e06-1035-385b-6267175819aa(at)lab(dot)ntt(dot)co(dot)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