From: | Andres Freund <andres(at)anarazel(dot)de> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | "Shulgin, Oleksandr" <oleksandr(dot)shulgin(at)zalando(dot)de>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, chris(dot)tessels(at)inergy(dot)nl, Pg Bugs <pgsql-bugs(at)postgresql(dot)org> |
Subject: | Re: BUG #13985: Segmentation fault on PREPARE TRANSACTION |
Date: | 2016-02-25 17:03:11 |
Message-ID: | 20160225170311.qi3h6zbzf4mscdtj@alap3.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-bugs |
On 2016-02-25 09:51:49 -0500, Tom Lane wrote:
> "Shulgin, Oleksandr" <oleksandr(dot)shulgin(at)zalando(dot)de> writes:
> > On Wed, Feb 24, 2016 at 10:52 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > At the very least ISTM that we have to make pgprocno volatile (or use a
> >> memory barrier - but we don't have sufficient support for those in the
> >> older branches), and move the PGPROC/PGXACT lookups after the == -1
> >> check.
>
> > Use of volatile doesn't change the resulting code dramatically for me.
>
> Marking pgprocno volatile is silly. What *is* missing is this:
>
> - ProcArrayStruct *arrayP = procArray;
> + volatile ProcArrayStruct *arrayP = procArray;
Well, that'll also force arrayP->numProcs to be loaded from memory every
loop iteration. Not sure if we really want that. Otherwise, yes, that's
pretty much what I'm suggesting, although I think a temporary volatile
cast when loading pgprocno is better here. The important part is to
force that pgprocno is loaded *once* *before* the checks.
What bothers me about this right now is that we currently write the
pgprocno array with:
memmove(&arrayP->pgprocnos[index + 1], &arrayP->pgprocnos[index],
(arrayP->numProcs - index) * sizeof(int));
arrayP->pgprocnos[index] = proc->pgprocno;
arrayP->numProcs++;
afaics there's absolutely no guarantee that memmov() will only use
aligned sizeof(int) writes. Sure, efficiency reasons make that a sane
choice, but a memmove() doing sizeof(char) wide moves would still be
correct.
Which afaics can mean we end up with completely bogus pgprocno
values. Consider e.g. 0x0000ff00 being moved where previously 0x000000ff
was set - we could temporarily end up with 0x0000ffff; which might be
above MaxBackends.
Regards,
Andres
From | Date | Subject | |
---|---|---|---|
Next Message | Andres Freund | 2016-02-25 17:06:38 | Re: BUG #13985: Segmentation fault on PREPARE TRANSACTION |
Previous Message | Andres Freund | 2016-02-25 17:02:59 | Re: BUG #13985: Segmentation fault on PREPARE TRANSACTION |