Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Sun, Jun 19, 2011 at 7:17 PM, Kevin Grittner
> <Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
>> Heikki Linnakangas wrote:
>>
>>> * The oldserxid code is broken for non-default BLCKSZ.
>>> o The warning will come either too early or too late
>>> o There is no safeguard against actually wrapping around the
>>> SLRU, just the warning
>>> o I'm suspicious of the OldSerXidPagePrecedesLogically() logic
>>> with 32k BLCKSZ. In that case, OLDSERXID_MAX_PAGE is a larger
>>> than necessary to cover the whole range of 2^32 transactions,
>>> so at high XIDs, say 2^32-1, doesn't it incorrectly think that
>>> low XIDs, say 10, are in the past, not the future?
>>
>> It took me a while to see these problems because somehow I had
>> forgotten that SLRU_PAGES_PER_SEGMENT was a literal of 32 rather
>> than being based on BLCKSZ. After I rediscovered that, your
>> concern was clear enough.
>>
>> I think the attached patch addresses the problem with the
>> OldSerXidPagePrecedesLogically() function, which strikes me as
>> the most serious issue.
>>
>> Based on the calculation from the attached patch, it would be
>> easy to adjust the warning threshold, but I'm wondering if we
>> should just rip it out instead. As I mentioned in a recent post
>> based on reviewing your concerns, with an 8KB BLCKSZ the SLRU
>> system will start thinking we're into wraparound at one billion
>> transactions, and refuse to truncate segment files until we get
>> down to less than that, but we won't actually overwrite anything
>> and cause SSI misbehaviors until we eat through two billion (2^31
>> really) transactions while holding open a single read-write
>> transaction. At that point I think PostgreSQL has other defenses
>> which come into play. With the attached patch I don't think we
>> can have any such problems with a *larger* BLCKSZ, so the only
>> point of the warning would be for a BLCKSZ of 4KB or less. Is it
>> worth carrying the warning code (with an appropriate adjustment
>> to the thresholds) or should we drop it?
>
> Is this still an open item?
Yes, although I'm not clear on whether people feel it is one which
needs to be fixed for 9.1 or left for 9.2.
On a build with a BLCKSZ less than 8KB we would not get a warning
before problems occurred, and we would have more serious problem
involving potentially incorrect behavior. Tom questioned whether
anyone actually builds with BLCKSZ less than 8KB, and I'm not
altogether sure that SLRUs dealing with transaction IDs would behave
correctly either.
On block sizes larger than 8KB it will the warning if you burn
through one billion transactions while holding one serializable read
write transaction open, even though there won't be a problem. With
the larger BLCKSZ values it may also generate log level messages
about SLRU wraparound when that's not really a problem.
The patch posted with the quoted message will prevent the
misbehavior on smaller block sizes and the bogus log messages on
larger block sizes. We'd need to change a couple more lines to get
the warning to trigger at the appropriate time for different block
sizes -- or we could just rip out the warning code. (I didn't post
a patch for that because there wasn't a clear consensus about
whether to fix it, rip it out, or leave it alone for 9.1.)
-Kevin