Fix handling of unlogged tables in FOR ALL TABLES publications

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Fix handling of unlogged tables in FOR ALL TABLES publications

Peter Eisentraut-6
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.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

0001-Fix-handling-of-unlogged-tables-in-FOR-ALL-TABLES-pu.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Amit Langote-2
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.

Thanks,
Amit


Reply | Threaded
Open this post in threaded view
|

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

Kyotaro HORIGUCHI-2
At Thu, 14 Mar 2019 11:30:12 +0900, Amit Langote <[hidden email]> wrote in <[hidden email]>

> 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 <[hidden email]>
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 <[hidden email]>
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

Reply | Threaded
Open this post in threaded view
|

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

Amit Langote-2
On 2019/03/14 15:03, Kyotaro HORIGUCHI wrote:

> At Thu, 14 Mar 2019 11:30:12 +0900, Amit Langote <[hidden email]> wrote in <[hidden email]>
>> 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


Reply | Threaded
Open this post in threaded view
|

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

Kyotaro HORIGUCHI-2
At Thu, 14 Mar 2019 15:31:03 +0900, Amit Langote <[hidden email]> wrote in <[hidden email]>
> On 2019/03/14 15:03, Kyotaro HORIGUCHI wrote:
> > 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.

Yeah, that's a reasnable alternative.

--
Kyotaro Horiguchi
NTT Open Source Software Center