On 2016-07-13 23:06:07 -0700, Andres Freund wrote:
> > +           /* Clear only the all-frozen bit on visibility map if needed */
> > +           if (PageIsAllVisible(BufferGetPage(buffer)) &&
> > +                   VM_ALL_FROZEN(relation, block, &vmbuffer))
> > +           {
> > +                   visibilitymap_clear_extended(relation, block, vmbuffer,
> > +                                                                           
> >  VISIBILITYMAP_ALL_FROZEN);
> > +           }
> > +
> 
> FWIW, I don't think it's worth introducing visibilitymap_clear_extended.
> As this is a 9.6 only patch, i think it's better to change
> visibilitymap_clear's API.

Besides that easily fixed issue, the code also has the significant issue
that it's only performing the the visibilitymap processing in the
BLK_NEEDS_REDO case. But that's not ok, because both in the BLK_RESTORED
and the BLK_DONE cases the visibilitymap isn't guaranteed (or even
likely in the former case) to have been updated.

I think we have two choices how to deal with that: First, we can add a
new flags variable to xl_heap_lock similar to
xl_heap_insert/update/... and bump page magic, or we can squeeze the
information into infobits_set.  The latter seems fairly ugly, and
fragile to me; so unless somebody protests I'm going with the former. I
think due to padding the additional byte doesn't make any size
difference anyway.

Regards,

Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to