On Tue, Dec 17, 2019 at 10:09 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Mon, Dec 16, 2019 at 9:16 PM Jehan-Guillaume de Rorthais > <j...@dalibo.com> wrote: > > > > On Mon, 16 Dec 2019 13:27:43 +0100 > > Peter Eisentraut <peter.eisentr...@2ndquadrant.com> wrote: > > > > > On 2019-12-16 11:11, Amit Kapila wrote: > > > > I agree that this is a timing issue. I also don't see a way to write > > > > a reproducible test for this. However, I could reproduce it via > > > > debugger consistently by following the below steps. I have updated a > > > > few comments and commit messages in the attached patch. > > > > > > > > Peter E., Petr J or anyone else, do you have comments or objections on > > > > this patch? If none, then I am planning to commit (and backpatch) > > > > this patch in a few days time. > > > > > > The patch seems fine to me. Writing a test seems hard. Let's skip it. > > > > > Okay. > > > > The commit message has a duplicate "building"/"built" in the first > > > sentence. > > > > I think the sentence is quite long. I had to re-read it to get it. > > > > What about: > > > > This patch allows building the local relmap cache for a subscribed > > relation > > after processing pending invalidation messages and potential relcache > > updates. > > > > Attached patch with updated commit message based on suggestions. I am > planning to commit this tomorrow unless there are more comments. >
While testing the patch on back versions, I found that the patch does not apply on PG 11 & PG 10 branch. Attached patch has the changes for PG 11 & PG 10 branch. Only difference in the patch was that table_open needed to be changed to heap_open. I have verified the patch on back branches and found it to be working. For PG 12 & current the previous patch that Amit need to be used, I'm not reattaching here. Regards, Vignesh EnterpriseDB: http://www.enterprisedb.com
From 2c4123d514473a9203c3617655229286a84487e6 Mon Sep 17 00:00:00 2001 From: vignesh <vignesh@localhost.localdomain> Date: Tue, 17 Dec 2019 17:05:35 +0530 Subject: [PATCH] Fix subscriber invalid memory access on DDL. This patch allows building the local relmap cache for a subscribed relation after processing pending invalidation messages and potential relcache updates. Without this, the attributes in the local cache don't tally with the updated relcache entry leading to invalid memory access. Reported-by Jehan-Guillaume de Rorthais Author: Jehan-Guillaume de Rorthais and Vignesh C Reviewed-by: Amit Kapila Backpatch-through: 10 Discussion: https://postgr.es/m/20191025175929.7e90dbf5@firost --- src/backend/replication/logical/relation.c | 40 +++++++++++++++++++----------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c index cf6f0a7..e1bbabd 100644 --- a/src/backend/replication/logical/relation.c +++ b/src/backend/replication/logical/relation.c @@ -220,6 +220,8 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE lockmode) { LogicalRepRelMapEntry *entry; bool found; + Oid relid = InvalidOid; + LogicalRepRelation *remoterel; if (LogicalRepRelMap == NULL) logicalrep_relmap_init(); @@ -232,20 +234,17 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE lockmode) elog(ERROR, "no relation map entry for remote relation ID %u", remoteid); - /* Need to update the local cache? */ + remoterel = &entry->remoterel; + + /* + * When opening and locking a relation, pending invalidation messages are + * processed which can invalidate the relation. We need to update the + * local cache both when we are first time accessing the relation and when + * the relation is invalidated (aka entry->localreloid is set InvalidOid). + */ if (!OidIsValid(entry->localreloid)) { - Oid relid; - int i; - int found; - Bitmapset *idkey; - TupleDesc desc; - LogicalRepRelation *remoterel; - MemoryContext oldctx; - - remoterel = &entry->remoterel; - - /* Try to find and lock the relation by name. */ + /* Try to find and lock the relation by name */ relid = RangeVarGetRelid(makeRangeVar(remoterel->nspname, remoterel->relname, -1), lockmode, true); @@ -256,6 +255,21 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE lockmode) remoterel->nspname, remoterel->relname))); entry->localrel = heap_open(relid, NoLock); + } + else + { + relid = entry->localreloid; + entry->localrel = heap_open(entry->localreloid, lockmode); + } + + if (!OidIsValid(entry->localreloid)) + { + int found; + Bitmapset *idkey; + TupleDesc desc; + MemoryContext oldctx; + int i; + /* Check for supported relkind. */ CheckSubscriptionRelkind(entry->localrel->rd_rel->relkind, remoterel->nspname, remoterel->relname); @@ -350,8 +364,6 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE lockmode) entry->localreloid = relid; } - else - entry->localrel = heap_open(entry->localreloid, lockmode); if (entry->state != SUBREL_STATE_READY) entry->state = GetSubscriptionRelState(MySubscription->oid, -- 1.8.3.1
