pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedFix handling of unlogged tables in FOR ALL TABLES publications
7+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2019-04-18 bb385c4fb0 f993bacdea 05d151e138 Fix handling of temp and unlogged tables in FOR ALL TABLES publications
* Fix handling of unlogged tables in FOR ALL TABLES publications
@ 2019-03-13 12:03 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
0 siblings, 1 reply; 7+ messages in thread
From: Peter Eisentraut @ 2019-03-13 12:03 UTC (permalink / raw)
To: pgsql-hackers
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
Attachments:
0001-Fix-handling-of-unlogged-tables-in-FOR-ALL-TABLES-pu.patch (text/plain, 1.3K) download
From 95f78d4ef499410c5f5a0fbcd86b658a2cdabbd9 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Wed, 13 Mar 2019 13:00:09 +0100
Subject: [PATCH] Fix handling of unlogged tables in FOR ALL TABLES
publications
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.
---
src/backend/executor/execReplication.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c
index 95dfc4987d..3b79629a4a 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -566,6 +566,13 @@ CheckCmdReplicaIdentity(Relation rel, CmdType cmd)
{
PublicationActions *pubactions;
+ /*
+ * If not publishable, we don't care, since it's not going to be
+ * replicated. (pgoutput_change() will ignore it.)
+ */
+ if (!is_publishable_relation(rel))
+ return;
+
/* We only need to do checks for UPDATE and DELETE. */
if (cmd != CMD_UPDATE && cmd != CMD_DELETE)
return;
--
2.21.0
* Re: Fix handling of unlogged tables in FOR ALL TABLES publications
@ 2019-03-14 02:30 Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
parent: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
0 siblings, 1 reply; 7+ messages in thread
From: Amit Langote @ 2019-03-14 02:30 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; pgsql-hackers
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.
Thanks,
Amit
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix handling of unlogged tables in FOR ALL TABLES publications
@ 2019-03-14 06:03 Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
parent: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
0 siblings, 1 reply; 7+ messages in thread
From: Kyotaro HORIGUCHI @ 2019-03-14 06:03 UTC (permalink / raw)
To: Langote_Amit_f8@lab.ntt.co.jp; +Cc: peter.eisentraut@2ndquadrant.com; pgsql-hackers
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?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments: download all (2 files, .tar.gz)
0001-step1.patch (text/x-patch, 1.0K) download
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
0002-step2-fix-indentation.patch (text/x-patch, 2.6K) download
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
* Re: Fix handling of unlogged tables in FOR ALL TABLES publications
@ 2019-03-14 06:31 Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
parent: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
0 siblings, 1 reply; 7+ messages in thread
From: Amit Langote @ 2019-03-14 06:31 UTC (permalink / raw)
To: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>; +Cc: peter.eisentraut@2ndquadrant.com; pgsql-hackers
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
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix handling of unlogged tables in FOR ALL TABLES publications
@ 2019-03-14 07:27 Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
parent: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
0 siblings, 1 reply; 7+ messages in thread
From: Kyotaro HORIGUCHI @ 2019-03-14 07:27 UTC (permalink / raw)
To: Langote_Amit_f8@lab.ntt.co.jp; +Cc: peter.eisentraut@2ndquadrant.com; pgsql-hackers
At Thu, 14 Mar 2019 15:31:03 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in <26bfa053-3fb2-ad1d-efbb-7c930b41c0fd@lab.ntt.co.jp>
> 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
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix handling of unlogged tables in FOR ALL TABLES publications
@ 2019-03-25 08:04 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
parent: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
0 siblings, 1 reply; 7+ messages in thread
From: Peter Eisentraut @ 2019-03-25 08:04 UTC (permalink / raw)
To: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>; Langote_Amit_f8@lab.ntt.co.jp; +Cc: pgsql-hackers
So perhaps push the check down to GetRelationPublicationActions()
instead. That way we don't have to patch up two places and everything
"just works" even for possible other callers. See attached patch.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
v2-0001-Fix-handling-of-unlogged-tables-in-FOR-ALL-TABLES.patch (text/plain, 1.4K) download
From 55255a6e338ccb11b4a7a8d6f10aff7a805cd72b Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 25 Mar 2019 09:00:14 +0100
Subject: [PATCH v2] Fix handling of unlogged tables in FOR ALL TABLES
publications
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. To fix, have GetRelationPublicationActions()
return that such a table publishes no actions.
Discussion: https://www.postgresql.org/message-id/f3f151f7-c4dd-1646-b998-f60bd6217dd3@2ndquadrant.com
---
src/backend/utils/cache/relcache.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 84609e0725..e13719ee97 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -5133,6 +5133,13 @@ GetRelationPublicationActions(Relation relation)
MemoryContext oldcxt;
PublicationActions *pubactions = palloc0(sizeof(PublicationActions));
+ /*
+ * If not publishable, it publishes no actions. (pgoutput_change() will
+ * ignore it.)
+ */
+ if (!is_publishable_relation(relation))
+ return pubactions;
+
if (relation->rd_pubactions)
return memcpy(pubactions, relation->rd_pubactions,
sizeof(PublicationActions));
--
2.21.0
* Re: Fix handling of unlogged tables in FOR ALL TABLES publications
@ 2019-04-18 08:14 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
parent: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
0 siblings, 0 replies; 7+ messages in thread
From: Peter Eisentraut @ 2019-04-18 08:14 UTC (permalink / raw)
To: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>; Langote_Amit_f8@lab.ntt.co.jp; +Cc: pgsql-hackers
On 2019-03-25 09:04, Peter Eisentraut wrote:
> So perhaps push the check down to GetRelationPublicationActions()
> instead. That way we don't have to patch up two places and everything
> "just works" even for possible other callers. See attached patch.
This has been committed and backpatched to PG10.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2019-04-18 08:14 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-03-13 12:03 Fix handling of unlogged tables in FOR ALL TABLES publications Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
2019-03-14 02:30 ` Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2019-03-14 06:03 ` Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
2019-03-14 06:31 ` Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
2019-03-14 07:27 ` Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
2019-03-25 08:04 ` Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
2019-04-18 08:14 ` Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
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