On 06.06.2013 17:22, Robert Haas wrote:
> On Thu, May 30, 2013 at 2:29 AM, Andres Freund<andres@2ndquadrant.com> wrote:
>>> Yeah, I think it's fine. The patch also looks fine, although I think
>>> the comments could use a bit of tidying. I guess we need to
>>> back-patch this all the way back to 8.4? It will require some
>>> adjustments for the older branches.
>>
>> I think 9.2 is actually far enough and it should apply there. Before
>> that we only logged the unsetting of all_visible via
>> heap_(inset|update|delete)'s wal records not the setting as far as I can
>> tell. So I don't immediately see a danger< 9.2.
>
> OK. I have committed this. For 9.2, I had to backport
> log_newpage_buffer() and use XLByteEQ rather than ==.
I'm afraid this patch was a few bricks shy of a load. The
log_newpage_buffer() function asserts that:
> /* We should be in a critical section. */
> Assert(CritSectionCount > 0);
But the call in vacuumlazy.c is not inside a critical section. Also, the
comments in log_newpage_buffer() say that the caller should mark the
buffer dirty *before* calling log_newpage_buffer(), but in vacuumlazy.c,
it's marked dirty afterwards. I'm not sure what consequences that might
have, but at least it contradicts the comment.
(spotted this while working on a patch, and ran into the assertion on
crash recovery)
- Heikki