Re: Inadequate thought about buffer locking during hot standby replay

During normal running, operations such as btree page splits are
extremely careful about the order in which they acquire and release
buffer locks, if they're doing something that concurrently modifies
multiple pages.

During WAL replay, that all goes out the window. Even if an individual
WAL-record replay function does things in the right order for "standard"
cases, RestoreBkpBlocks has no idea what it's doing. So if one or more
of the referenced pages gets treated as a full-page image, we are left
with no guarantee whatsoever about what order the pages are restored in.
That never mattered when the code was originally designed, but it sure
matters during Hot Standby when other queries might be able to see the
intermediate states.

I can't prove that this is the cause of bug #7648, but it's fairly easy
to see that it could explain the symptom. You only need to assume that
the page-being-split had been handled as a full-page image, and that the
new right-hand page had gotten allocated by extending the relation.
Then there will be an interval just after RestoreBkpBlocks does its
thing where the updated left-hand sibling is in the index and is not
locked in any way, but its right-link points off the end of the index.
If a few indexscans come along before the replay process gets to
continue, you'd get exactly the reported errors.

I'm inclined to think that we need to fix this by getting rid of
RestoreBkpBlocks per se, and instead having the per-WAL-record restore
routines dictate when each full-page image is restored (and whether or
not to release the buffer lock immediately). That's not going to be a
small change unfortunately :-(

regards, tom lane

On 2012-11-09 18:24:25 -0500, Tom Lane wrote:
> I can't prove that this is the cause of bug #7648, but it's fairly easy
> to see that it could explain the symptom. You only need to assume that
> the page-being-split had been handled as a full-page image, and that the
> new right-hand page had gotten allocated by extending the relation.
> Then there will be an interval just after RestoreBkpBlocks does its
> thing where the updated left-hand sibling is in the index and is not
> locked in any way, but its right-link points off the end of the index.
> If a few indexscans come along before the replay process gets to
> continue, you'd get exactly the reported errors.

Sounds plausible.

> I'm inclined to think that we need to fix this by getting rid of
> RestoreBkpBlocks per se, and instead having the per-WAL-record restore
> routines dictate when each full-page image is restored (and whether or
> not to release the buffer lock immediately). That's not going to be a
> small change unfortunately :-(

I wonder if we couldn't instead "fix" it by ensuring the backup blocks
are in the right order in the backup blocks at the inserting
location. That would "just" need some care about the order of
XLogRecData blocks.
I am pretty unfamiliar with the nbtree locking but I seem to remember
that we should be fine if we always restore from left to right?

Greetings,

Andres Freund

On Fri, Nov 9, 2012 at 3:24 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> During normal running, operations such as btree page splits are
> extremely careful about the order in which they acquire and release
> buffer locks, if they're doing something that concurrently modifies
> multiple pages.
>
> During WAL replay, that all goes out the window. Even if an individual
> WAL-record replay function does things in the right order for "standard"
> cases, RestoreBkpBlocks has no idea what it's doing. So if one or more
> of the referenced pages gets treated as a full-page image, we are left
> with no guarantee whatsoever about what order the pages are restored in.
> That never mattered when the code was originally designed, but it sure
> matters during Hot Standby when other queries might be able to see the
> intermediate states.
>
> I can't prove that this is the cause of bug #7648,

(I was the reporter of 7648)

To lend slightly more circumstantial evidence in support of this, I
also happened to note that the relfile in question was the last
segment and it was about a quarter full, so the access attempt was
definitely at the extreme outermost edge of the index most generally.

--
fdr

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2012-11-09 18:24:25 -0500, Tom Lane wrote:
>> I'm inclined to think that we need to fix this by getting rid of
>> RestoreBkpBlocks per se, and instead having the per-WAL-record restore
>> routines dictate when each full-page image is restored (and whether or
>> not to release the buffer lock immediately). That's not going to be a
>> small change unfortunately :-(

> I wonder if we couldn't instead "fix" it by ensuring the backup blocks
> are in the right order in the backup blocks at the inserting
> location. That would "just" need some care about the order of
> XLogRecData blocks.

I don't think that's a good way to go. In the first place, if we did
that the fix would require incompatible changes in the contents of WAL
streams. In the second place, there are already severe constraints on
the positioning of backup blocks to ensure that WAL records can be
uniquely decoded (the section of access/transam/README about WAL coding
touches on this) --- I don't think it's a good plan to add still more
constraints there. And in the third place, the specific problem we're
positing here results from a failure to hold the buffer lock for a
full-page image until after we're done restoring a *non* full-page image
represented elsewhere in the same WAL record. In general, of the set
of pages touched by a WAL record, any arbitrary subset of them might be
converted to FPIs during XLogInsert; but the replay-time locking
requirements are going to be the same regardless of that. So AFAICS,
any design in which RestoreBkpBlocks acts independently of the
non-full-page-image updates is just broken.

regards, tom lane

On 9 November 2012 23:24, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> During normal running, operations such as btree page splits are
> extremely careful about the order in which they acquire and release
> buffer locks, if they're doing something that concurrently modifies
> multiple pages.
>
> During WAL replay, that all goes out the window. Even if an individual
> WAL-record replay function does things in the right order for "standard"
> cases, RestoreBkpBlocks has no idea what it's doing. So if one or more
> of the referenced pages gets treated as a full-page image, we are left
> with no guarantee whatsoever about what order the pages are restored in.
> That never mattered when the code was originally designed, but it sure
> matters during Hot Standby when other queries might be able to see the
> intermediate states.
>
> I can't prove that this is the cause of bug #7648, but it's fairly easy
> to see that it could explain the symptom. You only need to assume that
> the page-being-split had been handled as a full-page image, and that the
> new right-hand page had gotten allocated by extending the relation.
> Then there will be an interval just after RestoreBkpBlocks does its
> thing where the updated left-hand sibling is in the index and is not
> locked in any way, but its right-link points off the end of the index.
> If a few indexscans come along before the replay process gets to
> continue, you'd get exactly the reported errors.
>
> I'm inclined to think that we need to fix this by getting rid of
> RestoreBkpBlocks per se, and instead having the per-WAL-record restore
> routines dictate when each full-page image is restored (and whether or
> not to release the buffer lock immediately). That's not going to be a
> small change unfortunately :-(

No, but it looks like a clear bug scenario and a clear resolution also.

I'll start looking at it.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On 9 November 2012 23:24, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I'm inclined to think that we need to fix this by getting rid of
>> RestoreBkpBlocks per se, and instead having the per-WAL-record restore
>> routines dictate when each full-page image is restored (and whether or
>> not to release the buffer lock immediately). That's not going to be a
>> small change unfortunately :-(

> No, but it looks like a clear bug scenario and a clear resolution also.
> I'll start looking at it.

I'm on it already.

regards, tom lane

I wrote:
> I'm inclined to think that we need to fix this by getting rid of
> RestoreBkpBlocks per se, and instead having the per-WAL-record restore
> routines dictate when each full-page image is restored (and whether or
> not to release the buffer lock immediately). That's not going to be a
> small change unfortunately :-(

Here's a WIP patch that attacks it in this way. I've only gone as far as
fixing btree_xlog_split() for the moment, since the main point is to see
whether the coding change seems reasonable. At least in this function,
it seems that locking considerations are handled correctly as long as no
full-page image is used, so it's pretty straightforward to tweak it to
handle the case with full-page image(s) as well. I've not looked
through any other replay functions yet --- some of them may need more
invasive hacking. But so far this looks pretty good.

Note that this patch isn't correct yet even for btree_xlog_split(),
because I've not removed the RestoreBkpBlocks() call in btree_redo().
All the btree replay routines will have to get fixed before it's
testable at all.

One thing that seems a bit annoying is the use of zero-based backup
block indexes in RestoreBackupBlock, when most (not all) of the callers
are referencing macros with one-based names (XLR_BKP_BLOCK_1 etc).
That's a bug waiting to happen. We could address it by changing
RestoreBackupBlock to accept a one-based argument, but what I'm thinking
of doing instead is getting rid of the one-based convention entirely;
that is, remove XLR_BKP_BLOCK_1 through XLR_BKP_BLOCK_4 and instead have
all code use the XLR_SET_BKP_BLOCK() macro, which is zero-based. One
advantage of doing that is that it'll force inspection of all code
related to this.

Comments, opinions?

regards, tom lane

On 10 November 2012 18:16, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I wrote:
>> I'm inclined to think that we need to fix this by getting rid of
>> RestoreBkpBlocks per se, and instead having the per-WAL-record restore
>> routines dictate when each full-page image is restored (and whether or
>> not to release the buffer lock immediately). That's not going to be a
>> small change unfortunately :-(
>
> Here's a WIP patch that attacks it in this way. I've only gone as far as
> fixing btree_xlog_split() for the moment, since the main point is to see
> whether the coding change seems reasonable. At least in this function,
> it seems that locking considerations are handled correctly as long as no
> full-page image is used, so it's pretty straightforward to tweak it to
> handle the case with full-page image(s) as well. I've not looked
> through any other replay functions yet --- some of them may need more
> invasive hacking. But so far this looks pretty good.

Looks fine, but we need a top-level comment in btree code explaining
why/how it follows the very well explained rules in the README.

> Note that this patch isn't correct yet even for btree_xlog_split(),
> because I've not removed the RestoreBkpBlocks() call in btree_redo().
> All the btree replay routines will have to get fixed before it's
> testable at all.

I was just about to write you back you missed that...

> One thing that seems a bit annoying is the use of zero-based backup
> block indexes in RestoreBackupBlock, when most (not all) of the callers
> are referencing macros with one-based names (XLR_BKP_BLOCK_1 etc).
> That's a bug waiting to happen. We could address it by changing
> RestoreBackupBlock to accept a one-based argument, but what I'm thinking
> of doing instead is getting rid of the one-based convention entirely;
> that is, remove XLR_BKP_BLOCK_1 through XLR_BKP_BLOCK_4 and instead have
> all code use the XLR_SET_BKP_BLOCK() macro, which is zero-based. One
> advantage of doing that is that it'll force inspection of all code
> related to this.

I wouldn't do that in a back branch, but I can see why its a good idea.

Thanks for fixing.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On 10 November 2012 18:16, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Here's a WIP patch that attacks it in this way.

> Looks fine, but we need a top-level comment in btree code explaining
> why/how it follows the very well explained rules in the README.

Not sure what you'd want it to say exactly? Really these issues are per
WAL-replay function, not global. I've now been through all of the btree
functions, and AFAICS btree_xlog_split is the only one of them that
actually has a bug; the others can be a bit lax about the order in which
things get done.

>> One thing that seems a bit annoying is the use of zero-based backup
>> block indexes in RestoreBackupBlock, when most (not all) of the callers
>> are referencing macros with one-based names (XLR_BKP_BLOCK_1 etc).
>> That's a bug waiting to happen. We could address it by changing
>> RestoreBackupBlock to accept a one-based argument, but what I'm thinking
>> of doing instead is getting rid of the one-based convention entirely;
>> that is, remove XLR_BKP_BLOCK_1 through XLR_BKP_BLOCK_4 and instead have
>> all code use the XLR_SET_BKP_BLOCK() macro, which is zero-based. One
>> advantage of doing that is that it'll force inspection of all code
>> related to this.

> I wouldn't do that in a back branch, but I can see why its a good idea.

If any of this stuff were getting used by external modules, changing it
would be problematic ... but fortunately, it isn't, because we lack
support for plug-in rmgrs. So I'm not worried about back-patching the
change, and would prefer to keep the 9.x branches in sync.

Attached is an updated patch in which nbtxlog.c has been fully converted,
but I've not touched other rmgrs yet. I've tested this to the extent of
having a replication slave follow a master that's running the regression
tests, and it seems to work. It's difficult to prove much by testing
about concurrent behavior, of course.

I found that there's another benefit of managing backup-block replay
this way, which is that we can de-contort the handling of conflict
resolution by moving it into the per-record-type subroutines, instead of
needing an extra switch before the RestoreBkpBlocks call. So that gives
me more confidence that this is a good way to go.

regards, tom lane

While I'm looking at this: there seems to be a bug/inconsistency in
heap_xlog_freeze(). It uses a cleanup lock in the "normal" code path,
but it doesn't tell RestoreBkpBlocks to use a cleanup lock. Which
choice is correct?

regards, tom lane

On 10 November 2012 21:24, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
>> On 10 November 2012 18:16, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Here's a WIP patch that attacks it in this way.
>
>> Looks fine, but we need a top-level comment in btree code explaining
>> why/how it follows the very well explained rules in the README.
>
> Not sure what you'd want it to say exactly? Really these issues are per
> WAL-replay function, not global. I've now been through all of the btree
> functions, and AFAICS btree_xlog_split is the only one of them that
> actually has a bug; the others can be a bit lax about the order in which
> things get done.
>
>>> One thing that seems a bit annoying is the use of zero-based backup
>>> block indexes in RestoreBackupBlock, when most (not all) of the callers
>>> are referencing macros with one-based names (XLR_BKP_BLOCK_1 etc).
>>> That's a bug waiting to happen. We could address it by changing
>>> RestoreBackupBlock to accept a one-based argument, but what I'm thinking
>>> of doing instead is getting rid of the one-based convention entirely;
>>> that is, remove XLR_BKP_BLOCK_1 through XLR_BKP_BLOCK_4 and instead have
>>> all code use the XLR_SET_BKP_BLOCK() macro, which is zero-based. One
>>> advantage of doing that is that it'll force inspection of all code
>>> related to this.
>
>> I wouldn't do that in a back branch, but I can see why its a good idea.
>
> If any of this stuff were getting used by external modules, changing it
> would be problematic ... but fortunately, it isn't, because we lack
> support for plug-in rmgrs. So I'm not worried about back-patching the
> change, and would prefer to keep the 9.x branches in sync.
>
> Attached is an updated patch in which nbtxlog.c has been fully converted,
> but I've not touched other rmgrs yet. I've tested this to the extent of
> having a replication slave follow a master that's running the regression
> tests, and it seems to work. It's difficult to prove much by testing
> about concurrent behavior, of course.
>
> I found that there's another benefit of managing backup-block replay
> this way, which is that we can de-contort the handling of conflict
> resolution by moving it into the per-record-type subroutines, instead of
> needing an extra switch before the RestoreBkpBlocks call. So that gives
> me more confidence that this is a good way to go.

It's a good way to go, I'm just glad you're handling it for the back
branches ;-)

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Attached is a complete draft patch against HEAD for this issue. I found
a depressingly large number of pre-existing bugs:

Practically all WAL record types that touch multiple pages have some
bug of this type. In addition to btree_xlog_split, I found that
heap_xlog_update, ginRedoDeletePage, spgRedoAddLeaf, spgRedoMoveLeafs,
spgRedoAddNode, spgRedoSplitTuple, and spgRedoPickSplit fail to hold
locks as required to make their updates safe for concurrent queries.
(I'm not totally sure about ginRedoDeletePage, but the original action
definitely locks the pages simultaneously, and it's not clear that it's
safe not to.) Most of these are okay in cases without any full-page
images, but could fail if the wrong subset of the pages-to-be-touched
were processed by RestoreBkpBlocks. Some had bugs even without that :-(

Worse than that, GIST WAL replay seems to be a total disaster from this
standpoint, and I'm not convinced that it's even fixable without
incompatible WAL changes. There are several cases where index
insertion operations generate more than one WAL record while holding
exclusive lock on buffer(s). If the lock continuity is actually
necessary to prevent concurrent queries from seeing inconsistent index
states, then we have a big problem, because WAL replay isn't designed to
hold any buffer lock for more than one WAL action. This needs to be
looked at closer by somebody who's more up on the GIST code than I am.
The attached patch doesn't try very hard to make the GIST functions
safe, it just transposes them to the new API.

I also found that spgRedoAddNode could fail to update the parent
downlink at all, if the parent tuple is in the same page as either the
old or new split tuple --- it would get fooled by the LSN having been
advanced already. This is an index corruption bug not just a
transient-failure problem.

Also, ginHeapTupleFastInsert's "merge lists" case fails to mark the old
tail page as a candidate for a full-page image; in the worst case this
could result in torn-page corruption.

I already pointed out the inconsistency in heap_xlog_freeze about
whether a cleanup lock is needed. As is, this patch uses a cleanup
lock, but I suspect that a regular lock is sufficient --- comments?

Also, gistRedoPageDeleteRecord seems to be dead code? I don't see
anything that emits XLOG_GIST_PAGE_DELETE.

regards, tom lane

#application/octet-stream [restore-backup-blocks-individually-2.patch.gz] /home/tgl/pgsql/restore-backup-blocks-individually-2.patch.gz

[ this time *with* the patch ]

Attached is a complete draft patch against HEAD for this issue. I found
a depressingly large number of pre-existing bugs:

Practically all WAL record types that touch multiple pages have some
bug of this type. In addition to btree_xlog_split, I found that
heap_xlog_update, ginRedoDeletePage, spgRedoAddLeaf, spgRedoMoveLeafs,
spgRedoAddNode, spgRedoSplitTuple, and spgRedoPickSplit fail to hold
locks as required to make their updates safe for concurrent queries.
(I'm not totally sure about ginRedoDeletePage, but the original action
definitely locks the pages simultaneously, and it's not clear that it's
safe not to.) Most of these are okay in cases without any full-page
images, but could fail if the wrong subset of the pages-to-be-touched
were processed by RestoreBkpBlocks. Some had bugs even without that :-(

Worse than that, GIST WAL replay seems to be a total disaster from this
standpoint, and I'm not convinced that it's even fixable without
incompatible WAL changes. There are several cases where index
insertion operations generate more than one WAL record while holding
exclusive lock on buffer(s). If the lock continuity is actually
necessary to prevent concurrent queries from seeing inconsistent index
states, then we have a big problem, because WAL replay isn't designed to
hold any buffer lock for more than one WAL action. This needs to be
looked at closer by somebody who's more up on the GIST code than I am.
The attached patch doesn't try very hard to make the GIST functions
safe, it just transposes them to the new API.

I also found that spgRedoAddNode could fail to update the parent
downlink at all, if the parent tuple is in the same page as either the
old or new split tuple --- it would get fooled by the LSN having been
advanced already. This is an index corruption bug not just a
transient-failure problem.

Also, ginHeapTupleFastInsert's "merge lists" case fails to mark the old
tail page as a candidate for a full-page image; in the worst case this
could result in torn-page corruption.

I already pointed out the inconsistency in heap_xlog_freeze about
whether a cleanup lock is needed. As is, this patch uses a cleanup
lock, but I suspect that a regular lock is sufficient --- comments?

Also, gistRedoPageDeleteRecord seems to be dead code? I don't see
anything that emits XLOG_GIST_PAGE_DELETE.

regards, tom lane

On 2012-11-10 16:24:22 -0500, Tom Lane wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> >> One thing that seems a bit annoying is the use of zero-based backup
> >> block indexes in RestoreBackupBlock, when most (not all) of the callers
> >> are referencing macros with one-based names (XLR_BKP_BLOCK_1 etc).
> >> That's a bug waiting to happen. We could address it by changing
> >> RestoreBackupBlock to accept a one-based argument, but what I'm thinking
> >> of doing instead is getting rid of the one-based convention entirely;
> >> that is, remove XLR_BKP_BLOCK_1 through XLR_BKP_BLOCK_4 and instead have
> >> all code use the XLR_SET_BKP_BLOCK() macro, which is zero-based. One
> >> advantage of doing that is that it'll force inspection of all code
> >> related to this.
>
> > I wouldn't do that in a back branch, but I can see why its a good idea.
>
> If any of this stuff were getting used by external modules, changing it
> would be problematic ... but fortunately, it isn't, because we lack
> support for plug-in rmgrs. So I'm not worried about back-patching the
> change, and would prefer to keep the 9.x branches in sync.

XLR_BKP_BLOCK_* might be used by things like pg_lesslog and its surely
used by xlogdump. Not sure if either are worth that much attention, but
it seems worth noticing that such a change will break stuff.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 12.11.2012 01:25, Tom Lane wrote:
> Worse than that, GIST WAL replay seems to be a total disaster from this
> standpoint, and I'm not convinced that it's even fixable without
> incompatible WAL changes. There are several cases where index
> insertion operations generate more than one WAL record while holding
> exclusive lock on buffer(s). If the lock continuity is actually
> necessary to prevent concurrent queries from seeing inconsistent index
> states, then we have a big problem, because WAL replay isn't designed to
> hold any buffer lock for more than one WAL action.

Hmm. After the rewrite of that logic I did in 9.1 (commit 9de3aa65f0),
the GiST index is always in a consistent state. Before the downlink for
a newly split page has been inserted yet, its left sibling is flagged so
that a search knows to follow the right-link to find it. In normal
operation, the lock continuity is needed to prevent another backend from
seeing the incomplete split and trying to fix it, but in hot standby, we
never try to fix incomplete splits anyway.

So I think we're good on >= 9.1. The 9.0 code is broken, however. In
9.0, when a child page is split, the parent and new children are kept
locked until the downlinks are inserted/updated. If a concurrent scan
comes along and sees that incomplete state, it will miss tuples on the
new right siblings. We rely on a rm_cleanup operation at the end of WAL
replay to fix that situation, if the downlink insertion record is not
there. I don't see any easy way to fix that, unfortunately. Perhaps we
could backpatch the 9.1 rewrite, now that it's gotten some real-world
testing, but it was a big change so I don't feel very comfortable doing
that.

> Also, gistRedoPageDeleteRecord seems to be dead code? I don't see
> anything that emits XLOG_GIST_PAGE_DELETE.

Yep. It was used in VACUUM FULL, but has been dead since VACUUM FULL was
removed.

- Heikki

On Sun, Nov 11, 2012 at 6:24 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I already pointed out the inconsistency in heap_xlog_freeze about
> whether a cleanup lock is needed. As is, this patch uses a cleanup
> lock, but I suspect that a regular lock is sufficient --- comments?

Well, according to storage/buffer/README:

1. To scan a page for tuples, one must hold a pin and either shared or
exclusive content lock. To examine the commit status (XIDs and status bits)
of a tuple in a shared buffer, one must likewise hold a pin and either shared
or exclusive lock.

That does indeed make it sound like an x-lock is enough.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sun, Nov 11, 2012 at 6:24 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I already pointed out the inconsistency in heap_xlog_freeze about
>> whether a cleanup lock is needed. As is, this patch uses a cleanup
>> lock, but I suspect that a regular lock is sufficient --- comments?

> Well, according to storage/buffer/README:

> 1. To scan a page for tuples, one must hold a pin and either shared or
> exclusive content lock. To examine the commit status (XIDs and status bits)
> of a tuple in a shared buffer, one must likewise hold a pin and either shared
> or exclusive lock.

> That does indeed make it sound like an x-lock is enough.

Yeah. AFAICS, the only possible downside is that somebody might be
using the tuple (while holding just a buffer pin), and that its xmin
might change while that's happening. So for instance a nestloop join
might read out different xmin values for the same row while the join
proceeds. But that could happen anyway if a different plan had been
chosen (viz, this table on the inside not the outside of the nestloop).
The xmin update ought to be physically atomic, so you shouldn't be able
to get a garbage result, just the old value or the new.

The cleanup lock is needed for cases where we'd like to remove or move a
tuple, but that's not required here.

regards, tom lane

Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
> On 12.11.2012 01:25, Tom Lane wrote:
>> Worse than that, GIST WAL replay seems to be a total disaster from this
>> standpoint, and I'm not convinced that it's even fixable without
>> incompatible WAL changes.

> Hmm. After the rewrite of that logic I did in 9.1 (commit 9de3aa65f0),
> the GiST index is always in a consistent state. Before the downlink for
> a newly split page has been inserted yet, its left sibling is flagged so
> that a search knows to follow the right-link to find it. In normal
> operation, the lock continuity is needed to prevent another backend from
> seeing the incomplete split and trying to fix it, but in hot standby, we
> never try to fix incomplete splits anyway.

> So I think we're good on >= 9.1.

Okay. I'll update the patch to make sure that the individual WAL replay
functions hold all locks, but not worry about holding locks across
actions.

> The 9.0 code is broken, however. In
> 9.0, when a child page is split, the parent and new children are kept
> locked until the downlinks are inserted/updated. If a concurrent scan
> comes along and sees that incomplete state, it will miss tuples on the
> new right siblings. We rely on a rm_cleanup operation at the end of WAL
> replay to fix that situation, if the downlink insertion record is not
> there. I don't see any easy way to fix that, unfortunately. Perhaps we
> could backpatch the 9.1 rewrite, now that it's gotten some real-world
> testing, but it was a big change so I don't feel very comfortable doing
> that.

Me either. Given the lack of field complaints, I think we're better
advised to just leave it unfixed in 9.0. It'd not be a step forward
if we broke something trying to make this work.

>> Also, gistRedoPageDeleteRecord seems to be dead code? I don't see
>> anything that emits XLOG_GIST_PAGE_DELETE.

> Yep. It was used in VACUUM FULL, but has been dead since VACUUM FULL was
> removed.

Okay. I see we bumped XLOG_PAGE_MAGIC in 9.0, so there's no longer
any way that 9.0 or later versions could see this WAL record type.
I'll delete that replay function rather than bothering to fix it.

regards, tom lane

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2012-11-10 16:24:22 -0500, Tom Lane wrote:
>> If any of this stuff were getting used by external modules, changing it
>> would be problematic ... but fortunately, it isn't, because we lack
>> support for plug-in rmgrs. So I'm not worried about back-patching the
>> change, and would prefer to keep the 9.x branches in sync.

> XLR_BKP_BLOCK_* might be used by things like pg_lesslog and its surely
> used by xlogdump. Not sure if either are worth that much attention, but
> it seems worth noticing that such a change will break stuff.

Hm. Okay, how about we leave the old macros in place in the back
branches?

regards, tom lane

On 2012-11-12 10:19:09 -0500, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2012-11-10 16:24:22 -0500, Tom Lane wrote:
> >> If any of this stuff were getting used by external modules, changing it
> >> would be problematic ... but fortunately, it isn't, because we lack
> >> support for plug-in rmgrs. So I'm not worried about back-patching the
> >> change, and would prefer to keep the 9.x branches in sync.
>
> > XLR_BKP_BLOCK_* might be used by things like pg_lesslog and its surely
> > used by xlogdump. Not sure if either are worth that much attention, but
> > it seems worth noticing that such a change will break stuff.
>
> Hm. Okay, how about we leave the old macros in place in the back
> branches?

Sounds good to me. The RestoreBkpBlocks change seems unproblematic to
me. If anything its good that it has been renamed.

Thanks,

Andres

On 11 November 2012 23:24, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Practically all WAL record types that touch multiple pages have some
> bug of this type. In addition to btree_xlog_split, I found that
> heap_xlog_update, ginRedoDeletePage, spgRedoAddLeaf, spgRedoMoveLeafs,
> spgRedoAddNode, spgRedoSplitTuple, and spgRedoPickSplit fail to hold
> locks as required to make their updates safe for concurrent queries.
> (I'm not totally sure about ginRedoDeletePage, but the original action
> definitely locks the pages simultaneously, and it's not clear that it's
> safe not to.) Most of these are okay in cases without any full-page
> images, but could fail if the wrong subset of the pages-to-be-touched
> were processed by RestoreBkpBlocks. Some had bugs even without that :-(

Hmm, not good. Thanks for spotting.

Do these changes do anything to actions that occur across multiple
records? I assume not and think those are OK, agreed?

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 12 November 2012 14:51, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

>> The 9.0 code is broken, however. In
>> 9.0, when a child page is split, the parent and new children are kept
>> locked until the downlinks are inserted/updated. If a concurrent scan
>> comes along and sees that incomplete state, it will miss tuples on the
>> new right siblings. We rely on a rm_cleanup operation at the end of WAL
>> replay to fix that situation, if the downlink insertion record is not
>> there. I don't see any easy way to fix that, unfortunately. Perhaps we
>> could backpatch the 9.1 rewrite, now that it's gotten some real-world
>> testing, but it was a big change so I don't feel very comfortable doing
>> that.
>
> Me either. Given the lack of field complaints, I think we're better
> advised to just leave it unfixed in 9.0. It'd not be a step forward
> if we broke something trying to make this work.

Agreed. Most people running 9.0 with GIST indexes have already upgraded.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On 11 November 2012 23:24, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Practically all WAL record types that touch multiple pages have some
>> bug of this type. In addition to btree_xlog_split, I found that
>> heap_xlog_update, ginRedoDeletePage, spgRedoAddLeaf, spgRedoMoveLeafs,
>> spgRedoAddNode, spgRedoSplitTuple, and spgRedoPickSplit fail to hold
>> locks as required to make their updates safe for concurrent queries.
>> (I'm not totally sure about ginRedoDeletePage, but the original action
>> definitely locks the pages simultaneously, and it's not clear that it's
>> safe not to.) Most of these are okay in cases without any full-page
>> images, but could fail if the wrong subset of the pages-to-be-touched
>> were processed by RestoreBkpBlocks. Some had bugs even without that :-(

> Hmm, not good. Thanks for spotting.

> Do these changes do anything to actions that occur across multiple
> records? I assume not and think those are OK, agreed?

Right, we were and still are assuming that any state that exists between
WAL records is consistent and safe to expose to hot-standby queries.
The important thing here is that these WAL replay functions were failing
to ensure that their own actions appear atomic to onlookers. This is
basically hangover from pre-hot-standby coding conventions, when no such
concern existed.

regards, tom lane

Here's an updated patch that fixes the GIST replay functions as well as
the other minor issues that were mentioned. Barring objections, I'll
set about back-patching this as far as 9.0.

One thing that could use verification is my fix for
gistRedoPageSplitRecord. AFAICS, the first page listed in the WAL
record is always the "original" page, and the ones following it are
pages that were split off from it, and can (as yet) only be reached by
following right-links from the "original" page. As such, it should be
okay to release locks on the non-first pages as soon as we've written
them. We have to hold lock on the original page though to avoid letting
readers follow dangling right-links. Also, the update of
NSN/FOLLOW_RIGHT on the child page (if any) has to be done atomically
with all this, so that has to be done before releasing the original-page
lock as well. Does that sound right?

regards, tom lane

On 12.11.2012 22:53, Tom Lane wrote:
> Here's an updated patch that fixes the GIST replay functions as well as
> the other minor issues that were mentioned. Barring objections, I'll
> set about back-patching this as far as 9.0.

Ok. It won't help all that much on 9.0, though.

> One thing that could use verification is my fix for
> gistRedoPageSplitRecord. AFAICS, the first page listed in the WAL
> record is always the "original" page, and the ones following it are
> pages that were split off from it, and can (as yet) only be reached by
> following right-links from the "original" page. As such, it should be
> okay to release locks on the non-first pages as soon as we've written
> them. We have to hold lock on the original page though to avoid letting
> readers follow dangling right-links. Also, the update of
> NSN/FOLLOW_RIGHT on the child page (if any) has to be done atomically
> with all this, so that has to be done before releasing the original-page
> lock as well. Does that sound right?

Yep.

- Heikki

Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
> On 12.11.2012 22:53, Tom Lane wrote:
>> Here's an updated patch that fixes the GIST replay functions as well as
>> the other minor issues that were mentioned. Barring objections, I'll
>> set about back-patching this as far as 9.0.

> Ok. It won't help all that much on 9.0, though.

Well, it won't help GIST much, but the actually-reported-from-the-field
case is in btree, and it does fix that.

It occurs to me that if we're sufficiently scared of this case, we could
probably hack the planner (in 9.0 only) to refuse to use GIST indexes
in hot-standby queries. That cure might be worse than the disease though.

regards, tom lane

On Tue, Nov 13, 2012 at 9:03 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Ok. It won't help all that much on 9.0, though.
>
> Well, it won't help GIST much, but the actually-reported-from-the-field
> case is in btree, and it does fix that.
>
> It occurs to me that if we're sufficiently scared of this case, we could
> probably hack the planner (in 9.0 only) to refuse to use GIST indexes
> in hot-standby queries. That cure might be worse than the disease though.

if anything, it should be documented. if you do this kind of thing
people will stop installing bugfix releases.

merlin

On 14/11/12 04:32, Merlin Moncure wrote:
> On Tue, Nov 13, 2012 at 9:03 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Ok. It won't help all that much on 9.0, though.
>> Well, it won't help GIST much, but the actually-reported-from-the-field
>> case is in btree, and it does fix that.
>>
>> It occurs to me that if we're sufficiently scared of this case, we could
>> probably hack the planner (in 9.0 only) to refuse to use GIST indexes
>> in hot-standby queries. That cure might be worse than the disease though.
> if anything, it should be documented. if you do this kind of thing
> people will stop installing bugfix releases.
>
> merlin
>
>
How about displaying a warning, when people try to use the 'feature', as
well as document it?

Cheers,
Gavin

On Tue, Nov 13, 2012 at 10:32 AM, Merlin Moncure <mmoncure(at)gmail(dot)com> wrote:
> On Tue, Nov 13, 2012 at 9:03 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Ok. It won't help all that much on 9.0, though.
>>
>> Well, it won't help GIST much, but the actually-reported-from-the-field
>> case is in btree, and it does fix that.
>>
>> It occurs to me that if we're sufficiently scared of this case, we could
>> probably hack the planner (in 9.0 only) to refuse to use GIST indexes
>> in hot-standby queries. That cure might be worse than the disease though.
>
> if anything, it should be documented. if you do this kind of thing
> people will stop installing bugfix releases.

Agreed. I think doing that in a back-branch release would be
extremely user-hostile.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company