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

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?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 2704c5fbb65ebfee144a37c6b34eccdd853033a0 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 14 Mar 2019 15:02:20 +0900
Subject: [PATCH 1/2] step1

---
 src/backend/utils/cache/relcache.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index eba77491fd..a43fb040cb 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -5089,6 +5089,8 @@ GetRelationPublicationActions(Relation relation)
         return memcpy(pubactions, relation->rd_pubactions,
                       sizeof(PublicationActions));
 
+    if (is_publishable_relation(relation))
+    {
     /* Fetch the publication membership info. */
     puboids = GetRelationPublications(RelationGetRelid(relation));
     puboids = list_concat_unique_oid(puboids, GetAllTablesPublications());
@@ -5121,6 +5123,7 @@ GetRelationPublicationActions(Relation relation)
             pubactions->pubdelete && pubactions->pubtruncate)
             break;
     }
+    }
 
     if (relation->rd_pubactions)
     {
-- 
2.16.3

From b0946c5b97857cf476975306ce78355f9316e722 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 14 Mar 2019 15:02:54 +0900
Subject: [PATCH 2/2] step2: fix indentation

---
 src/backend/utils/cache/relcache.c | 50 +++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index a43fb040cb..f5dff5572e 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -5091,38 +5091,38 @@ GetRelationPublicationActions(Relation relation)
 
     if (is_publishable_relation(relation))
     {
-    /* Fetch the publication membership info. */
-    puboids = GetRelationPublications(RelationGetRelid(relation));
-    puboids = list_concat_unique_oid(puboids, GetAllTablesPublications());
+        /* Fetch the publication membership info. */
+        puboids = GetRelationPublications(RelationGetRelid(relation));
+        puboids = list_concat_unique_oid(puboids, GetAllTablesPublications());
 
-    foreach(lc, puboids)
-    {
-        Oid            pubid = lfirst_oid(lc);
-        HeapTuple    tup;
-        Form_pg_publication pubform;
+        foreach(lc, puboids)
+        {
+            Oid            pubid = lfirst_oid(lc);
+            HeapTuple    tup;
+            Form_pg_publication pubform;
 
-        tup = SearchSysCache1(PUBLICATIONOID, ObjectIdGetDatum(pubid));
+            tup = SearchSysCache1(PUBLICATIONOID, ObjectIdGetDatum(pubid));
 
-        if (!HeapTupleIsValid(tup))
-            elog(ERROR, "cache lookup failed for publication %u", pubid);
+            if (!HeapTupleIsValid(tup))
+                elog(ERROR, "cache lookup failed for publication %u", pubid);
 
-        pubform = (Form_pg_publication) GETSTRUCT(tup);
+            pubform = (Form_pg_publication) GETSTRUCT(tup);
 
-        pubactions->pubinsert |= pubform->pubinsert;
-        pubactions->pubupdate |= pubform->pubupdate;
-        pubactions->pubdelete |= pubform->pubdelete;
-        pubactions->pubtruncate |= pubform->pubtruncate;
+            pubactions->pubinsert |= pubform->pubinsert;
+            pubactions->pubupdate |= pubform->pubupdate;
+            pubactions->pubdelete |= pubform->pubdelete;
+            pubactions->pubtruncate |= pubform->pubtruncate;
 
-        ReleaseSysCache(tup);
+            ReleaseSysCache(tup);
 
-        /*
-         * If we know everything is replicated, there is no point to check for
-         * other publications.
-         */
-        if (pubactions->pubinsert && pubactions->pubupdate &&
-            pubactions->pubdelete && pubactions->pubtruncate)
-            break;
-    }
+            /*
+             * If we know everything is replicated, there is no point to check
+             * for other publications.
+             */
+            if (pubactions->pubinsert && pubactions->pubupdate &&
+                pubactions->pubdelete && pubactions->pubtruncate)
+                break;
+        }
     }
 
     if (relation->rd_pubactions)
-- 
2.16.3