Received: from malur.postgresql.org ([217.196.149.56])
	by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256)
	(Exim 4.89)
	(envelope-from <pgsql-hackers-owner+archive@lists.postgresql.org>)
	id 1igsTg-0001hb-LD
	for pgsql-hackers@arkaria.postgresql.org; Mon, 16 Dec 2019 15:40:09 +0000
Received: from localhost ([127.0.0.1] helo=malur.postgresql.org)
	by malur.postgresql.org with esmtp (Exim 4.89)
	(envelope-from <pgsql-hackers-owner+archive@lists.postgresql.org>)
	id 1igsTf-00064z-H6
	for pgsql-hackers@arkaria.postgresql.org; Mon, 16 Dec 2019 15:40:07 +0000
Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29])
	by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256)
	(Exim 4.89)
	(envelope-from <jgdr@dalibo.com>)
	id 1igsTf-00064s-66
	for pgsql-hackers@lists.postgresql.org; Mon, 16 Dec 2019 15:40:07 +0000
Received: from mail.dalibo.com ([212.85.157.172])
	by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256)
	(Exim 4.92)
	(envelope-from <jgdr@dalibo.com>)
	id 1igsTc-0007wa-QO
	for pgsql-hackers@lists.postgresql.org; Mon, 16 Dec 2019 15:40:06 +0000
Received: from firost (unknown [88.190.0.6])
	by mail.dalibo.com (Postfix) with ESMTPSA id 43A9B202E5;
	Mon, 16 Dec 2019 16:40:01 +0100 (CET)
DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=dalibo.com; s=a;
	t=1576510801; bh=lvi+C/9ZMODdeiAPI+uhEJts9rUgMU00EMWAShAp9T0=;
	h=Date:From:To:Cc:Subject:In-Reply-To:References:From;
	b=b/XHfoRJIDRN/aqF22OzkhW7XthdPS7Lw/d53s/f6hrZlnBSig/FglLt34/Nq3EbU
	 y+RbIEnxocrUdMQporzz7XvqcjTvUP+fqb4zEUfpz8+ZgaYLpfe8USLvqUbYdUM5CF
	 vzlE/kb5PNIkpipidNUFLJktza0tPfniLthw8cSw=
Date: Mon, 16 Dec 2019 16:40:00 +0100
From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
To: vignesh C <vignesh21@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>
Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>, PostgreSQL Hackers
 <pgsql-hackers@lists.postgresql.org>
Subject: Re: segmentation fault when cassert enabled
Message-ID: <20191216164000.02f06490@firost>
In-Reply-To: 
 <CALDaNm0kVT3+ku99M6=Wy9X=3=-H8jp_Yt7WVuTraPuREZugxQ@mail.gmail.com>
References: <20191025175929.7e90dbf5@firost>
	<18714.1572020918@sss.pgh.pa.us>
	<20191105172918.3e32a446@firost>
	<c75077cd-a1a7-9a2d-5dc6-4d3d8acfaafa@2ndquadrant.com>
	<20191125155519.5338e3c9@firost>
	<CAA4eK1+9vQb34faKLJbY6KD62HZOJ5Jm9PzoZGiK_9J7cvyDdw@mail.gmail.com>
	<CALDaNm0kVT3+ku99M6=Wy9X=3=-H8jp_Yt7WVuTraPuREZugxQ@mail.gmail.com>
Organization: Dalibo
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
List-Id: <pgsql-hackers.lists.postgresql.org>
List-Help: <https://lists.postgresql.org/manage/>
List-Subscribe: <https://lists.postgresql.org/manage/>
List-Post: <mailto:pgsql-hackers@lists.postgresql.org>
List-Owner: <mailto:pgsql-hackers-owner@lists.postgresql.org>
List-Archive: <https://www.postgresql.org/list/pgsql-hackers>
Precedence: bulk

On Fri, 13 Dec 2019 12:10:07 +0530
vignesh C <vignesh21@gmail.com> wrote:

> On Fri, Dec 6, 2019 at 5:30 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Nov 25, 2019 at 8:25 PM Jehan-Guillaume de Rorthais
> > <jgdr@dalibo.com> wrote:  
> > >
> > > On Wed, 6 Nov 2019 14:34:38 +0100
> > > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
> > >  
> > > > On 2019-11-05 17:29, Jehan-Guillaume de Rorthais wrote:  
> > > > > My best bet so far is that logicalrep_relmap_invalidate_cb is not
> > > > > called after the DDL on the subscriber so the relmap cache is not
> > > > > invalidated. So we end up with slot->tts_tupleDescriptor->natts
> > > > > superior than rel->remoterel->natts in slot_store_cstrings, leading
> > > > > to the overflow on attrmap and the sigsev.  
> > > >
> > > > It looks like something like that is happening.  But it shouldn't.
> > > > Different table schemas on publisher and subscriber are well supported,
> > > > so this must be an edge case of some kind.  Please continue
> > > > investigating.  
> > >
> > > I've been able to find the origin of the crash, but it was a long journey.
> > >
> > > <debugger hard life>
> > >
> > >   I was unable to debug using gdb record because of this famous error:
> > >
> > >     Process record does not support instruction 0xc5 at address
> > > 0x1482758a4b30.
> > >
> > >     Program stopped.
> > >     __memset_avx2_unaligned_erms ()
> > >     at ../sysdeps/x86_64/multiarch/memset-vec-unaligned-erms.S:168
> > >     168 ../sysdeps/x86_64/multiarch/memset-vec-unaligned-erms.S: No such
> > >       file or directory.
> > >
> > >   Trying with rr, I had constant "stack depth limit exceeded", even with
> > >   unlimited one. Does it worth opening a discussion or a wiki page about
> > > these tools? Peter, it looks like you have some experience with rr, any
> > > feedback?
> > >
> > >   Finally, Julien Rouhaud spend some time with me after work hours,
> > > a,swering my questions about some parts of the code and pointed me to the
> > > excellent backtrace_functions GUC addition few days ago. This finally did
> > > the trick to find out what was happening. Many thanks Julien!
> > >
> > > </debugger hard life>
> > >
> > > Back to the bug itself. Consider a working logical replication with
> > > constant update/insert activity, eg. pgbench running against provider.
> > >
> > > Now, on the subscriber side, a session issue an "ALTER TABLE ADD
> > > COLUMN" on a subscribed table, eg. pgbench_branches. A cache invalidation
> > > message is then pending for this table.
> > >
> > > In the meantime, the logical replication worker receive an UPDATE to
> > > apply. It opens the local relation using "logicalrep_rel_open". It finds
> > > the related entry in LogicalRepRelMap is valid, so it does not update its
> > > attrmap and directly opens and locks the local relation:
> > >  
> >
> > - /* Try to find and lock the relation by name. */
> > + /* Try to find the relation by name */
> >   relid = RangeVarGetRelid(makeRangeVar(remoterel->nspname,\
> >     remoterel->relname, -1),
> > - lockmode, true);
> > + NoLock, true);
> >
> > I think we can't do this because it could lead to locking the wrong
> > reloid.  See RangeVarGetRelidExtended.  It ensures that after locking
> > the relation (which includes accepting invalidation messages), that
> > the reloid is correct.  I think changing the code in the way you are
> > suggesting can lead to locking incorrect reloid.

Sorry for the delay, I couldn't answer earlier.

To be honest, I was wondering about that. Since we keep in cache the relid and
use it as cache invalidation, I thought it might be fragile. But then - as far
as I could find - the only way to change the relid is to drop and create a new
table. I wasn't sure it could really cause a race condition there because of
the impact of such commands on logical replication.

But now, I realize I should have go all the way through and close this
potential bug as well. Thank you.

> I have made changes to fix the comment provided. The patch for the
> same is attached. Could not add a test case for this scenario is based
> on timing issue.

Thank you for this fix Vignesh!