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.
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.patchtext/plain; charset=UTF-8; name=0001-Fix-handling-of-unlogged-tables-in-FOR-ALL-TABLES-pu.patch; x-mac-creator=0; x-mac-type=0Download+7-1
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 updatesPatch attached.
An email on -bugs earlier this morning complains of the same problem but
for temporary tables.
/messages/by-id/CAHOFxGr=mqPZXbAuoR7Nbq-bU4HxqVWHbTTUy5=PKQut_F0=XA@mail.gmail.com
It seems your patch fixes their case too.
Thanks,
Amit
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 updatesPatch attached.
An email on -bugs earlier this morning complains of the same problem but
for temporary tables./messages/by-id/CAHOFxGr=mqPZXbAuoR7Nbq-bU4HxqVWHbTTUy5=PKQut_F0=XA@mail.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
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 updatesPatch attached.
An email on -bugs earlier this morning complains of the same problem but
for temporary tables./messages/by-id/CAHOFxGr=mqPZXbAuoR7Nbq-bU4HxqVWHbTTUy5=PKQut_F0=XA@mail.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
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
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.patchtext/plain; charset=UTF-8; name=v2-0001-Fix-handling-of-unlogged-tables-in-FOR-ALL-TABLES.patch; x-mac-creator=0; x-mac-type=0Download+7-1
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