From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
cc: Andreas Seltenreich <seltenreich@gmx.de>, pgsql-hackers@postgresql.org,
        Alvaro Herrera <alvherre@2ndquadrant.com>,
        Michael Paquier <michael.paquier@gmail.com>
Subject: Re: [sqlsmith] stuck spinlock in pg_stat_get_wal_receiver after OOM
In-reply-to: <665.1506980082@sss.pgh.pa.us>
References: <87h8vhtgj2.fsf@ansel.ydns.eu>
 <20171002210805.x4lxxmarluen6rfg@alap3.anarazel.de>
 <459.1506979825@sss.pgh.pa.us>
 <20171002213215.4e3iwt7oh5wdjqm2@alap3.anarazel.de>
 <665.1506980082@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Mon, 02 Oct 2017 17:34:42 -0400"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1408.1506981260.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Mon, 02 Oct 2017 17:54:20 -0400
Message-ID: <1409.1506981260@sss.pgh.pa.us>
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

I wrote:
> If this is the only problem then I'd agree we should stick to a spinlock
> (I assume the strings in question can't be very long).  I was thinking
> more about what to do if we find other violations that are harder to fix.

I took a quick look through walreceiver.c, and there are a couple of
obvious problems of the same ilk in WalReceiverMain, which is doing this:

	walrcv->lastMsgSendTime =3D walrcv->lastMsgReceiptTime =3D walrcv->latestW=
alEndTime =3D GetCurrentTimestamp();

(ie, a potential kernel call) inside a spinlock.  But there seems no
real problem with just collecting the timestamp before we enter that
critical section.

I also don't especially like the fact that just above there it reaches
elog(PANIC) with the lock still held, though at least that's a case that
should never happen.

Further down, it's doing a pfree() inside the spinlock, apparently
for no other reason than to save one "if (tmp_conninfo)".

I don't especially like the Asserts inside spinlocks, either.  Personally,
I think if those conditions are worth testing then they're worth testing
for real (in production).  Variables that are manipulated by multiple
processes are way more likely to assume unexpected states than local
variables.

I'm also rather befuddled by the fact that this code sets and clears
walrcv->latch outside the critical sections.  If that field is used
by any other process, surely that's completely unsafe.  If it isn't,
why is it being kept in shared memory?

			regards, tom lane


--=20
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers