On 10 November 2012 21:24, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
>> On 10 November 2012 18:16, Tom Lane <tgl@sss.pgh.pa.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
