From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
To: vignesh C <vignesh21@gmail.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: segmentation fault when cassert enabled
Date: Mon, 16 Dec 2019 16:40:00 +0100
Message-ID: <20191216164000.02f06490@firost> (raw)
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>
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!
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: jgdr@dalibo.com, vignesh21@gmail.com, amit.kapila16@gmail.com, peter.eisentraut@2ndquadrant.com, pgsql-hackers@lists.postgresql.org
Subject: Re: segmentation fault when cassert enabled
In-Reply-To: <20191216164000.02f06490@firost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
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