On 2019/03/14 15:03, Kyotaro HORIGUCHI wrote: > At Thu, 14 Mar 2019 11:30:12 +0900, Amit Langote > <langote_amit...@lab.ntt.co.jp> wrote in > <59e5a734-9e06-1035-385b-626717581...@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
