On 10 November 2012 21:24, Tom Lane <t...@sss.pgh.pa.us> wrote: > Simon Riggs <si...@2ndquadrant.com> writes: >> On 10 November 2012 18:16, Tom Lane <t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
