Re: heapgetpage() and ->takenDuringRecovery

On Sun, Mar 2, 2014 at 8:39 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> While reading around which references to SnapshotData's members exist, I
> once more came about the following tidbit in heapgetpage():
>         /*
>          * If the all-visible flag indicates that all tuples on the page are
>          * visible to everyone, we can skip the per-tuple visibility tests.
>          *
>          * Note: In hot standby, a tuple that's already visible to all
>          * transactions in the master might still be invisible to a read-only
>          * transaction in the standby. We partly handle this problem by tracking
>          * the minimum xmin of visible tuples as the cut-off XID while marking a
>          * page all-visible on master and WAL log that along with the visibility
>          * map SET operation. In hot standby, we wait for (or abort) all
>          * transactions that can potentially may not see one or more tuples on the
>          * page. That's how index-only scans work fine in hot standby. A crucial
>          * difference between index-only scans and heap scans is that the
>          * index-only scan completely relies on the visibility map where as heap
>          * scan looks at the page-level PD_ALL_VISIBLE flag. We are not sure if
>          * the page-level flag can be trusted in the same way, because it might
>          * get propagated somehow without being explicitly WAL-logged, e.g. via a
>          * full page write. Until we can prove that beyond doubt, let's check each
>          * tuple for visibility the hard way.
>          */
>         all_visible = PageIsAllVisible(dp) && !snapshot->takenDuringRecovery;
>
> I don't think this is neccessary >= 9.2. The are two only "interestings" place
> where PD_ALL_VISIBLE is set:
> a) lazy_vacuum_page() where a xl_heap_clean is logged *before*
>    PD_ALL_VISIBLE/the vm is touched and that causes recovery
>    conflicts. The heap page is locked for cleanup at that point. As the
>    logging of xl_heap_clean sets the page's LSN there's no way the page
>    can appear on the standby too early.
> b) empty pages in lazy_scan_heap(). If they always were empty, there's
>    no need for conflicts. The only other way I can see to end up there
>    is a previous heap_page_prune() that repaired fragmentation. But that
>    logs a WAL record with conflict information.

I don't think there's any reason to believe that lazy_scan_heap() can
only hit pages that are empty or have just been defragged.  Suppose
that there's a tuple on the page which was recently inserted; the
inserting transaction has committed but there are some backends that
still have older snapshots.  The page won't be marked all-visible
because it isn't.  Now, eventually those older snapshots will go away,
and sometime after that the relation will get vacuumed again, and
we'll once again look the page.  But this time we notice that it is
all-visible, and mark it so.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company