Re: PageIsAllVisible()'s trustworthiness in Hot Standby

On 2012-12-04 18:38:11 +0530, Pavan Deolasee wrote:
> I was looking at the following code in heapam.c:
>
>  261     /*
>  262      * If the all-visible flag indicates that all tuples on the page
> are
>  263      * visible to everyone, we can skip the per-tuple visibility
> tests. But
>  264      * not in hot standby mode. A tuple that's already visible to all
>  265      * transactions in the master might still be invisible to a
> read-only
>  266      * transaction in the standby.
>  267      */
>  268     all_visible = PageIsAllVisible(dp) &&
> !snapshot->takenDuringRecovery;
>
> Isn't the check for !snapshot->takenDuringRecovery redundant now in master
> or whenever since we added crash-safety for VM ? In fact, this comment made
> me think if we are really handling index-only scans correctly or not on the
> Hot Standby. But apparently we are by forcing conflicting transactions to
> abort before redoing VM bit set operation on the standby. The same
> mechanism should protect us against the above case. Now I concede that the
> entire magic around setting and clearing the page level all-visible bit and
> the VM bit and our ability to keep them in sync is something I don't fully
> understand, but I see that every operation that sets the page
> level PD_ALL_VISIBLE flag also sets the VM bit while holding the buffer
> lock and emits a WAL record. So AFAICS  the conflict resolution logic will
> take care of the above too.

Yes, that really seems strange. As you say, were already relying on the
VM to be correct. I think it was simply missed that we can rely on this
since the conflict handling on all-visible was added in
3424bff90f40532527b9cf4f2ad9eaff750682f7.

Greetings,

Andres Freund

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