pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedpgsql: Tune GetSnapshotData() during Hot Standby by avoiding loop
12+ messages / 5 participants
[nested] [flat]
* pgsql: Tune GetSnapshotData() during Hot Standby by avoiding loop
@ 2010-04-18 18:06 Simon Riggs <sriggs@postgresql.org>
0 siblings, 1 reply; 12+ messages in thread
From: sriggs@postgresql.org @ 2010-04-18 18:06 UTC (permalink / raw)
To: pgsql-committers@postgresql.org
Log Message:
-----------
Tune GetSnapshotData() during Hot Standby by avoiding loop
through normal backends. Makes code clearer also, since we
avoid various Assert()s. Performance of snapshots taken
during recovery no longer depends upon number of read-only
backends.
Modified Files:
--------------
pgsql/src/backend/access/transam:
xlog.c (r1.397 -> r1.398)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/transam/xlog.c?r1=1.397&r2=1.3...)
pgsql/src/backend/storage/ipc:
procarray.c (r1.62 -> r1.63)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/storage/ipc/procarray.c?r1=1.62&r2=1....)
pgsql/src/include/access:
xlog.h (r1.106 -> r1.107)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/access/xlog.h?r1=1.106&r2=1.107)
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: pgsql: Tune GetSnapshotData() during Hot Standby by avoiding loop
@ 2010-04-19 07:36 Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
parent: Simon Riggs <sriggs@postgresql.org>
0 siblings, 1 reply; 12+ messages in thread
From: Heikki Linnakangas @ 2010-04-19 07:36 UTC (permalink / raw)
To: Simon Riggs <sriggs@postgresql.org>; +Cc: pgsql-committers@postgresql.org
Simon Riggs wrote:
> Log Message:
> -----------
> Tune GetSnapshotData() during Hot Standby by avoiding loop
> through normal backends. Makes code clearer also, since we
> avoid various Assert()s. Performance of snapshots taken
> during recovery no longer depends upon number of read-only
> backends.
I think there's a little race condition there.
snapshot->takenDuringRecovery is set before acquiring ProcArrayLock, so
it's possible that by the time we acquire the lock, we're no longer in
recovery. So this can happen:
1. Backend A starts to take a snapshot, while we're still in recovery.
takenDuringRecovery is assigned true.
2. Recovery ends, and a normal transaction X begins in backend B.
3. A skips scanning ProcArray because takenDuringRecovery is true.
The snapshot doesn't include X, so any changes done in that transaction
will not be visible to the snapshot while the transaction is still
running, but will be after it commits.
Of course, it's highly improbable for 2. to happen, but it's there.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [COMMITTERS] pgsql: Tune GetSnapshotData() during Hot Standby by avoiding loop
@ 2010-04-19 08:05 Simon Riggs <simon@2ndQuadrant.com>
parent: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
0 siblings, 1 reply; 12+ messages in thread
From: Simon Riggs @ 2010-04-19 08:05 UTC (permalink / raw)
To: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>; +Cc: pgsql-hackers
On Mon, 2010-04-19 at 10:36 +0300, Heikki Linnakangas wrote:
> Simon Riggs wrote:
> > Log Message:
> > -----------
> > Tune GetSnapshotData() during Hot Standby by avoiding loop
> > through normal backends. Makes code clearer also, since we
> > avoid various Assert()s. Performance of snapshots taken
> > during recovery no longer depends upon number of read-only
> > backends.
>
> I think there's a little race condition there.
> snapshot->takenDuringRecovery is set before acquiring ProcArrayLock, so
> it's possible that by the time we acquire the lock, we're no longer in
> recovery. So this can happen:
>
> 1. Backend A starts to take a snapshot, while we're still in recovery.
> takenDuringRecovery is assigned true.
> 2. Recovery ends, and a normal transaction X begins in backend B.
> 3. A skips scanning ProcArray because takenDuringRecovery is true.
>
> The snapshot doesn't include X, so any changes done in that transaction
> will not be visible to the snapshot while the transaction is still
> running, but will be after it commits.
>
> Of course, it's highly improbable for 2. to happen, but it's there.
The order of events is as you say, though I don't see the problem. The
new xids will be beyond xmax and would be filtered out even if we did
scan the procs, so they will be treated as running, which they are. Xmax
will not have advanced since that relies on completed transactions, not
started ones.
--
Simon Riggs www.2ndQuadrant.com
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [COMMITTERS] pgsql: Tune GetSnapshotData() during Hot Standby by avoiding loop
@ 2010-04-19 08:37 Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
parent: Simon Riggs <simon@2ndQuadrant.com>
0 siblings, 1 reply; 12+ messages in thread
From: Heikki Linnakangas @ 2010-04-19 08:37 UTC (permalink / raw)
To: Simon Riggs <simon@2ndQuadrant.com>; +Cc: pgsql-hackers
Simon Riggs wrote:
> On Mon, 2010-04-19 at 10:36 +0300, Heikki Linnakangas wrote:
>> Simon Riggs wrote:
>>> Log Message:
>>> -----------
>>> Tune GetSnapshotData() during Hot Standby by avoiding loop
>>> through normal backends. Makes code clearer also, since we
>>> avoid various Assert()s. Performance of snapshots taken
>>> during recovery no longer depends upon number of read-only
>>> backends.
>> I think there's a little race condition there.
>> snapshot->takenDuringRecovery is set before acquiring ProcArrayLock, so
>> it's possible that by the time we acquire the lock, we're no longer in
>> recovery. So this can happen:
>>
>> 1. Backend A starts to take a snapshot, while we're still in recovery.
>> takenDuringRecovery is assigned true.
>> 2. Recovery ends, and a normal transaction X begins in backend B.
>> 3. A skips scanning ProcArray because takenDuringRecovery is true.
>>
>> The snapshot doesn't include X, so any changes done in that transaction
>> will not be visible to the snapshot while the transaction is still
>> running, but will be after it commits.
>>
>> Of course, it's highly improbable for 2. to happen, but it's there.
>
> The order of events is as you say, though I don't see the problem. The
> new xids will be beyond xmax and would be filtered out even if we did
> scan the procs, so they will be treated as running, which they are. Xmax
> will not have advanced since that relies on completed transactions, not
> started ones.
Good point. However, it is theoretically possible that yet another
transaction starts and commits in that same window, bumping xmax.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [COMMITTERS] pgsql: Tune GetSnapshotData() during Hot Standby by avoiding loop
@ 2010-04-19 09:05 Simon Riggs <simon@2ndQuadrant.com>
parent: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
0 siblings, 1 reply; 12+ messages in thread
From: Simon Riggs @ 2010-04-19 09:05 UTC (permalink / raw)
To: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>; +Cc: pgsql-hackers
On Mon, 2010-04-19 at 11:37 +0300, Heikki Linnakangas wrote:
> Simon Riggs wrote:
> > On Mon, 2010-04-19 at 10:36 +0300, Heikki Linnakangas wrote:
> >> Simon Riggs wrote:
> >>> Log Message:
> >>> -----------
> >>> Tune GetSnapshotData() during Hot Standby by avoiding loop
> >>> through normal backends. Makes code clearer also, since we
> >>> avoid various Assert()s. Performance of snapshots taken
> >>> during recovery no longer depends upon number of read-only
> >>> backends.
> >> I think there's a little race condition there.
> >> snapshot->takenDuringRecovery is set before acquiring ProcArrayLock, so
> >> it's possible that by the time we acquire the lock, we're no longer in
> >> recovery. So this can happen:
> >>
> >> 1. Backend A starts to take a snapshot, while we're still in recovery.
> >> takenDuringRecovery is assigned true.
> >> 2. Recovery ends, and a normal transaction X begins in backend B.
> >> 3. A skips scanning ProcArray because takenDuringRecovery is true.
> >>
> >> The snapshot doesn't include X, so any changes done in that transaction
> >> will not be visible to the snapshot while the transaction is still
> >> running, but will be after it commits.
> >>
> >> Of course, it's highly improbable for 2. to happen, but it's there.
> >
> > The order of events is as you say, though I don't see the problem. The
> > new xids will be beyond xmax and would be filtered out even if we did
> > scan the procs, so they will be treated as running, which they are. Xmax
> > will not have advanced since that relies on completed transactions, not
> > started ones.
>
> Good point. However, it is theoretically possible that yet another
> transaction starts and commits in that same window, bumping xmax.
If the snapshotting backend (#1) froze at the exact point between one
CPU instruction and the next, during which time recovery ends, two new
sessions connect, two new write transactions start (#2 and #3), they
begin to write data and so assign xids, then write WAL, then the #3
writes commit record into WAL, flushes disk (maybe), updates clog and
then is granted procarraylock in exclusive mode before snapshotting
backend attempts to do so, while #2 keeps running.
Yes, it does seem theoretically possible, at that one exact point in
time, never to be repeated.
It doesn't seem to be something we should place highly on the list of
events we need protection from, does it?
--
Simon Riggs www.2ndQuadrant.com
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Re: [COMMITTERS] pgsql: Tune GetSnapshotData() during Hot Standby by avoiding loop
@ 2010-04-19 11:55 Robert Haas <robertmhaas@gmail.com>
parent: Simon Riggs <simon@2ndQuadrant.com>
0 siblings, 1 reply; 12+ messages in thread
From: Robert Haas @ 2010-04-19 11:55 UTC (permalink / raw)
To: Simon Riggs <simon@2ndquadrant.com>; +Cc: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>; pgsql-hackers
On Mon, Apr 19, 2010 at 5:05 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> It doesn't seem to be something we should place highly on the list of
> events we need protection from, does it?
Since when do we not protect against race-conditions just because
they're low likelihood?
...Robert
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Re: [COMMITTERS] pgsql: Tune GetSnapshotData() during Hot Standby by avoiding loop
@ 2010-04-19 14:24 Tom Lane <tgl@sss.pgh.pa.us>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 12+ messages in thread
From: Tom Lane @ 2010-04-19 14:24 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Simon Riggs <simon@2ndquadrant.com>; Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>; pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Apr 19, 2010 at 5:05 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> It doesn't seem to be something we should place highly on the list of
>> events we need protection from, does it?
> Since when do we not protect against race-conditions just because
> they're low likelihood?
Murphy's law says that the probability of any race condition happening
in the field is orders of magnitude higher than you think. This has
been proven true many times ...
regards, tom lane
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Re: [COMMITTERS] pgsql: Tune GetSnapshotData() during Hot Standby by avoiding loop
@ 2010-04-19 14:31 Simon Riggs <simon@2ndQuadrant.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 12+ messages in thread
From: Simon Riggs @ 2010-04-19 14:31 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Robert Haas <robertmhaas@gmail.com>; Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>; pgsql-hackers
On Mon, 2010-04-19 at 10:24 -0400, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Mon, Apr 19, 2010 at 5:05 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> >> It doesn't seem to be something we should place highly on the list of
> >> events we need protection from, does it?
>
> > Since when do we not protect against race-conditions just because
> > they're low likelihood?
>
> Murphy's law says that the probability of any race condition happening
> in the field is orders of magnitude higher than you think. This has
> been proven true many times ...
Choices are
1. Check RecoveryInProgress() once outside of lock, plus wild rumour of
Murphy
2. Check RecoveryInProgress() before and after holding lock
3. Check RecoveryInProgress() while holding lock
All of which perform better than
4. Revert patch
--
Simon Riggs www.2ndQuadrant.com
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Re: [COMMITTERS] pgsql: Tune GetSnapshotData() during Hot Standby by avoiding loop
@ 2010-04-19 14:44 Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
parent: Simon Riggs <simon@2ndQuadrant.com>
0 siblings, 1 reply; 12+ messages in thread
From: Heikki Linnakangas @ 2010-04-19 14:44 UTC (permalink / raw)
To: Simon Riggs <simon@2ndQuadrant.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
Simon Riggs wrote:
> On Mon, 2010-04-19 at 10:24 -0400, Tom Lane wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> On Mon, Apr 19, 2010 at 5:05 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>>>> It doesn't seem to be something we should place highly on the list of
>>>> events we need protection from, does it?
>>> Since when do we not protect against race-conditions just because
>>> they're low likelihood?
>> Murphy's law says that the probability of any race condition happening
>> in the field is orders of magnitude higher than you think. This has
>> been proven true many times ...
Right. And some future code changes elsewhere could make it more likely,
by the time we've forgotten all about this.
> Choices are
>
> 1. Check RecoveryInProgress() once outside of lock, plus wild rumour of
> Murphy
>
> 2. Check RecoveryInProgress() before and after holding lock
>
> 3. Check RecoveryInProgress() while holding lock
4. Check RecoveryInProgress() once outside of lock, and scan the
ProcArray anyway, just in case. That's what we did before this patch.
Document that takenDuringRecovery == true means that the snapshot was
most likely taken during recovery, but there is some race conditions
where takenDuringRecovery is true even though the snapshot was taken
just after recovery finished. AFAICS all of the other current uses of
takenDuringRecovery work fine with that.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Re: [COMMITTERS] pgsql: Tune GetSnapshotData() during Hot Standby by avoiding loop
@ 2010-04-19 14:47 Simon Riggs <simon@2ndQuadrant.com>
parent: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
0 siblings, 1 reply; 12+ messages in thread
From: Simon Riggs @ 2010-04-19 14:47 UTC (permalink / raw)
To: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Mon, 2010-04-19 at 17:44 +0300, Heikki Linnakangas wrote:
> > Choices are
> >
> > 1. Check RecoveryInProgress() once outside of lock, plus wild rumour of
> > Murphy
> >
> > 2. Check RecoveryInProgress() before and after holding lock
> >
> > 3. Check RecoveryInProgress() while holding lock
>
> 4. Check RecoveryInProgress() once outside of lock, and scan the
> ProcArray anyway, just in case. That's what we did before this patch.
> Document that takenDuringRecovery == true means that the snapshot was
> most likely taken during recovery, but there is some race conditions
> where takenDuringRecovery is true even though the snapshot was taken
> just after recovery finished. AFAICS all of the other current uses of
> takenDuringRecovery work fine with that.
Checking RecoveryInProgress() is much cheaper than scanning the whole
ProcArray, so (4) is definitely worse than 1-3.
--
Simon Riggs www.2ndQuadrant.com
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Re: [COMMITTERS] pgsql: Tune GetSnapshotData() during Hot Standby by avoiding loop
@ 2010-04-19 14:55 Tom Lane <tgl@sss.pgh.pa.us>
parent: Simon Riggs <simon@2ndQuadrant.com>
0 siblings, 1 reply; 12+ messages in thread
From: Tom Lane @ 2010-04-19 14:55 UTC (permalink / raw)
To: Simon Riggs <simon@2ndQuadrant.com>; +Cc: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
Simon Riggs <simon@2ndQuadrant.com> writes:
> On Mon, 2010-04-19 at 17:44 +0300, Heikki Linnakangas wrote:
>>> Choices are
>>> 1. Check RecoveryInProgress() once outside of lock, plus wild rumour of
>>> Murphy
>>> 2. Check RecoveryInProgress() before and after holding lock
>>> 3. Check RecoveryInProgress() while holding lock
>>
>> 4. Check RecoveryInProgress() once outside of lock, and scan the
>> ProcArray anyway, just in case. That's what we did before this patch.
>> Document that takenDuringRecovery == true means that the snapshot was
>> most likely taken during recovery, but there is some race conditions
>> where takenDuringRecovery is true even though the snapshot was taken
>> just after recovery finished. AFAICS all of the other current uses of
>> takenDuringRecovery work fine with that.
> Checking RecoveryInProgress() is much cheaper than scanning the whole
> ProcArray, so (4) is definitely worse than 1-3.
If the lock we're talking about is an LWLock, #3 is okay. If it's a
spinlock, not so much.
regards, tom lane
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Re: [COMMITTERS] pgsql: Tune GetSnapshotData() during Hot Standby by avoiding loop
@ 2010-04-19 18:17 Simon Riggs <simon@2ndQuadrant.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 12+ messages in thread
From: Simon Riggs @ 2010-04-19 18:17 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
On Mon, 2010-04-19 at 10:55 -0400, Tom Lane wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
> > On Mon, 2010-04-19 at 17:44 +0300, Heikki Linnakangas wrote:
> >>> Choices are
> >>> 1. Check RecoveryInProgress() once outside of lock, plus wild rumour of
> >>> Murphy
> >>> 2. Check RecoveryInProgress() before and after holding lock
> >>> 3. Check RecoveryInProgress() while holding lock
> >>
> >> 4. Check RecoveryInProgress() once outside of lock, and scan the
> >> ProcArray anyway, just in case. That's what we did before this patch.
> >> Document that takenDuringRecovery == true means that the snapshot was
> >> most likely taken during recovery, but there is some race conditions
> >> where takenDuringRecovery is true even though the snapshot was taken
> >> just after recovery finished. AFAICS all of the other current uses of
> >> takenDuringRecovery work fine with that.
>
> > Checking RecoveryInProgress() is much cheaper than scanning the whole
> > ProcArray, so (4) is definitely worse than 1-3.
>
> If the lock we're talking about is an LWLock, #3 is okay. If it's a
> spinlock, not so much.
Just committed the following patch to implement option #3.
We test RecoveryInProgress() after the LWLockAcquire rather than before.
--
Simon Riggs www.2ndQuadrant.com
Attachments:
switcheroo.patch (text/x-patch, 1.2K) download
*** a/src/backend/storage/ipc/procarray.c
--- b/src/backend/storage/ipc/procarray.c
***************
*** 1074,1081 **** GetSnapshotData(Snapshot snapshot)
errmsg("out of memory")));
}
- snapshot->takenDuringRecovery = RecoveryInProgress();
-
/*
* It is sufficient to get shared lock on ProcArrayLock, even if we are
* going to set MyProc->xmin.
--- 1074,1079 ----
***************
*** 1091,1098 **** GetSnapshotData(Snapshot snapshot)
globalxmin = xmin = xmax;
/*
! * If in recovery get any known assigned xids.
*/
if (!snapshot->takenDuringRecovery)
{
/*
--- 1089,1103 ----
globalxmin = xmin = xmax;
/*
! * If we're in recovery then snapshot data comes from a different place,
! * so decide which route we take before grab the lock. It is possible
! * for recovery to end before we finish taking snapshot, and for newly
! * assigned transaction ids to be added to the procarray. Xmax cannot
! * change while we hold ProcArrayLock, so those newly added transaction
! * ids would be filtered away, so we need not be concerned about them.
*/
+ snapshot->takenDuringRecovery = RecoveryInProgress();
+
if (!snapshot->takenDuringRecovery)
{
/*
end of thread, other threads:[~2010-04-19 18:17 UTC | newest]
Thread overview: 12+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2010-04-18 18:06 pgsql: Tune GetSnapshotData() during Hot Standby by avoiding loop Simon Riggs <sriggs@postgresql.org>
2010-04-19 07:36 ` Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
2010-04-19 08:05 ` Simon Riggs <simon@2ndQuadrant.com>
2010-04-19 08:37 ` Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
2010-04-19 09:05 ` Simon Riggs <simon@2ndQuadrant.com>
2010-04-19 11:55 ` Robert Haas <robertmhaas@gmail.com>
2010-04-19 14:24 ` Tom Lane <tgl@sss.pgh.pa.us>
2010-04-19 14:31 ` Simon Riggs <simon@2ndQuadrant.com>
2010-04-19 14:44 ` Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
2010-04-19 14:47 ` Simon Riggs <simon@2ndQuadrant.com>
2010-04-19 14:55 ` Tom Lane <tgl@sss.pgh.pa.us>
2010-04-19 18:17 ` Simon Riggs <simon@2ndQuadrant.com>
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