On Fri, Apr 5, 2019 at 8:37 AM Andres Freund <and...@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
