Re: "page is not marked all-visible" warning in regression tests

On Thursday, June 07, 2012 04:27:32 PM Robert Haas wrote:
> On Thu, Jun 7, 2012 at 9:41 AM, Andres Freund <andres@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.

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

The visibilitymap_clear/PageClearAllVisible in heap_multi_insert should be 
moved into the critical section, shouldn't it?

Regards,

Andres

-- 
 Andres Freund	                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services