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.
