From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | Andres Freund <andres(at)2ndquadrant(dot)com> |
Cc: | pgsql-hackers(at)postgresql(dot)org, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Subject: | Re: "page is not marked all-visible" warning in regression tests |
Date: | 2012-06-07 16:08:34 |
Message-ID: | CA+TgmoaS4adz1-iYdSfT+kE9VGR6sLgXcxJ4Qc4HmbGLkjx4BQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thu, Jun 7, 2012 at 11:04 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On Thursday, June 07, 2012 04:27:32 PM Robert Haas wrote:
>> On Thu, Jun 7, 2012 at 9:41 AM, Andres Freund <andres(at)2ndquadrant(dot)com>
> wrote:
>> >> Proposed patch attached. This adds some more comments in various
>> >> places, and implements your suggestion of retesting the visibility-map
>> >> bit when we detect a possible mismatch with the page-level bit.
>> >
>> > Thanks, will look at it in a bit.
> I wonder if
> /* mark page all-visible, if appropriate */
> if (all_visible && !PageIsAllVisible(page))
> {
> PageSetAllVisible(page);
> MarkBufferDirty(buf);
> visibilitymap_set(onerel, blkno, InvalidXLogRecPtr, vmbuffer,
> visibility_cutoff_xid);
> }
> shouldn't test
> if (all_visible &&
> (!PageIsAllVisible(page) || !all_visible_according_to_vm)
> instead.
Hmm, I think you're right.
> Commentwise I am not totally content with the emphasis on memory ordering
> because some of the stuff is more locking than memory ordering. Except that I
> think its a pretty clear improvement. I can reformulate the places where I
> find that relevant but I have the feeling that wouldn't help the legibility.
> Its the big comment in vacuumlazy.c, the comment in nodeIndexonly.c and the
> one in the header of visibilitymap_test. Should be s/memory-
> ordering/concurrency/ except in nodeIndexonlyscan.c
Hmm, I see your point.
> The visibilitymap_clear/PageClearAllVisible in heap_multi_insert should be
> moved into the critical section, shouldn't it?
Yes, it should. I was thinking maybe we could go the other way and
have heap_insert do it before starting the critical section, but
that's no good: clearing the visibility map bit is part of the
critical data change, and we can't do it and then forget to WAL-log
it.
Updated patch attached.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachment | Content-Type | Size |
---|---|---|
vm-test-cleanup-v2.patch | application/octet-stream | 6.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Heikki Linnakangas | 2012-06-07 16:12:56 | Re: XLog changes for 9.3 |
Previous Message | Andres Freund | 2012-06-07 16:02:56 | Re: XLog changes for 9.3 |