Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits

On Fri, Apr 5, 2019 at 8:37 AM Andres Freund <andres@anarazel.de> wrote:

> Hi,
>
> On 2019-04-05 00:06:04 -0300, Alvaro Herrera wrote:
>
> >
> > Hmm, isn't there already a critical section in visibilitymap_set itself?
>
> There is, but the proposed code sets all visible on the page, and marks
> the buffer dirty, before calling visibilitymap_set.
>

How's it any different than what we're doing at vacuumlazy.c:1322? We set
the page-level bit, mark the buffer dirty and then call
visibilitymap_set(), all outside a critical section.

1300         /* mark page all-visible, if appropriate */
1301         if (all_visible && !all_visible_according_to_vm)
1302         {
1303             uint8       flags = VISIBILITYMAP_ALL_VISIBLE;
1304
1305             if (all_frozen)
1306                 flags |= VISIBILITYMAP_ALL_FROZEN;
1307
1308             /*
1309              * It should never be the case that the visibility map
page is set
1310              * while the page-level bit is clear, but the reverse is
allowed
1311              * (if checksums are not enabled).  Regardless, set the
both bits
1312              * so that we get back in sync.
1313              *
1314              * NB: If the heap page is all-visible but the VM bit is
not set,
1315              * we don't need to dirty the heap page.  However, if
checksums
1316              * are enabled, we do need to make sure that the heap page
is
1317              * dirtied before passing it to visibilitymap_set(),
because it
1318              * may be logged.  Given that this situation should only
happen in
1319              * rare cases after a crash, it is not worth optimizing.
1320              */
1321             PageSetAllVisible(page);
1322             MarkBufferDirty(buf);
1323             visibilitymap_set(onerel, blkno, buf, InvalidXLogRecPtr,
1324                               vmbuffer, visibility_cutoff_xid, flags);
1325         }

As the first para in that comment says, I thought it's ok for page-level
bit to be set but the visibility bit to be clear, but not the vice versa.
The proposed code does not introduce any  new behaviour AFAICS. But I might
be missing something.

Thanks,
Pavan

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