On Thu, Jun 7, 2012 at 11:04 AM, Andres Freund <and...@2ndquadrant.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 <and...@2ndquadrant.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
vm-test-cleanup-v2.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers