Обсуждение: lazy_scan_heap() forgets to mark buffer dirty when setting allfrozen?
Hi,
lazy_scan_heap() contains the following block:
/*
* If the all-visible page is turned out to be all-frozen but not
* marked, we should so mark it. Note that all_frozen is only valid
* if all_visible is true, so we must check both.
*/
else if (all_visible_according_to_vm && all_visible && all_frozen &&
!VM_ALL_FROZEN(onerel, blkno, &vmbuffer))
{
/*
* We can pass InvalidTransactionId as the cutoff XID here,
* because setting the all-frozen bit doesn't cause recovery
* conflicts.
*/
visibilitymap_set(onerel, blkno, buf, InvalidXLogRecPtr,
vmbuffer, InvalidTransactionId,
VISIBILITYMAP_ALL_FROZEN);
}
but I'm afraid that's not quite enough. As an earlier comment explains:
* NB: If the heap page is all-visible but the VM bit is not set,
* we don't need to dirty the heap page. However, if checksums
* are enabled, we do need to make sure that the heap page is
* dirtied before passing it to visibilitymap_set(), because it
* may be logged. Given that this situation should only happen in
* rare cases after a crash, it is not worth optimizing.
*/
PageSetAllVisible(page);
MarkBufferDirty(buf);
visibilitymap_set(onerel, blkno, buf, InvalidXLogRecPtr,
vmbuffer, visibility_cutoff_xid, flags);
don't we need to do that here too? visibilitymap_set() does:
/*
* If data checksums are enabled (or wal_log_hints=on), we
* need to protect the heap page from being torn.
*/
if (XLogHintBitIsNeeded())
{
Page heapPage = BufferGetPage(heapBuf);
/* caller is expected to set PD_ALL_VISIBLE first */
Assert(PageIsAllVisible(heapPage));
PageSetLSN(heapPage, recptr);
}
i.e. it actually modifies the page when checksums/wal hint bits are
enabled, setting a different LSN. Without it being dirtied that won't
persist. Which doesn't seem good?
visibilitymap_set()'s comment header doesn't explain this well. Nor is
* Call visibilitymap_pin first to pin the right one. This function doesn't do
* any I/O.
actually true, given it does XLogInsert(). I think that should've been
adjusted in 503c7305a1e3 ("Make the visibility map crash-safe.").
Greetings,
Andres Freund
On 2019-Apr-07, Andres Freund wrote:
> lazy_scan_heap() contains the following block:
>
> /*
> * If the all-visible page is turned out to be all-frozen but not
> * marked, we should so mark it. Note that all_frozen is only valid
> * if all_visible is true, so we must check both.
> */
> else if (all_visible_according_to_vm && all_visible && all_frozen &&
> !VM_ALL_FROZEN(onerel, blkno, &vmbuffer))
> {
> /*
> * We can pass InvalidTransactionId as the cutoff XID here,
> * because setting the all-frozen bit doesn't cause recovery
> * conflicts.
> */
> visibilitymap_set(onerel, blkno, buf, InvalidXLogRecPtr,
> vmbuffer, InvalidTransactionId,
> VISIBILITYMAP_ALL_FROZEN);
> }
>
> but I'm afraid that's not quite enough.
Apparently the initial commit a892234f830e had MarkBufferDirty, but it
was removed one week later by 77a1d1e79892.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi,
On 2019-04-08 00:48:20 -0400, Alvaro Herrera wrote:
> On 2019-Apr-07, Andres Freund wrote:
>
> > lazy_scan_heap() contains the following block:
> >
> > /*
> > * If the all-visible page is turned out to be all-frozen but not
> > * marked, we should so mark it. Note that all_frozen is only valid
> > * if all_visible is true, so we must check both.
> > */
> > else if (all_visible_according_to_vm && all_visible && all_frozen &&
> > !VM_ALL_FROZEN(onerel, blkno, &vmbuffer))
> > {
> > /*
> > * We can pass InvalidTransactionId as the cutoff XID here,
> > * because setting the all-frozen bit doesn't cause recovery
> > * conflicts.
> > */
> > visibilitymap_set(onerel, blkno, buf, InvalidXLogRecPtr,
> > vmbuffer, InvalidTransactionId,
> > VISIBILITYMAP_ALL_FROZEN);
> > }
> >
> > but I'm afraid that's not quite enough.
>
> Apparently the initial commit a892234f830e had MarkBufferDirty, but it
> was removed one week later by 77a1d1e79892.
Good catch. Kinda looks like it could have been an accidental removal?
Robert?
Greetings,
Andres Freund
On Mon, Apr 8, 2019 at 12:49 PM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> lazy_scan_heap() contains the following block:
>
> /*
> * If the all-visible page is turned out to be all-frozen but not
> * marked, we should so mark it. Note that all_frozen is only valid
> * if all_visible is true, so we must check both.
> */
> else if (all_visible_according_to_vm && all_visible && all_frozen &&
> !VM_ALL_FROZEN(onerel, blkno, &vmbuffer))
> {
> /*
> * We can pass InvalidTransactionId as the cutoff XID here,
> * because setting the all-frozen bit doesn't cause recovery
> * conflicts.
> */
> visibilitymap_set(onerel, blkno, buf, InvalidXLogRecPtr,
> vmbuffer, InvalidTransactionId,
> VISIBILITYMAP_ALL_FROZEN);
> }
>
> but I'm afraid that's not quite enough. As an earlier comment explains:
>
>
> * NB: If the heap page is all-visible but the VM bit is not set,
> * we don't need to dirty the heap page. However, if checksums
> * are enabled, we do need to make sure that the heap page is
> * dirtied before passing it to visibilitymap_set(), because it
> * may be logged. Given that this situation should only happen in
> * rare cases after a crash, it is not worth optimizing.
> */
> PageSetAllVisible(page);
> MarkBufferDirty(buf);
> visibilitymap_set(onerel, blkno, buf, InvalidXLogRecPtr,
> vmbuffer, visibility_cutoff_xid, flags);
>
> don't we need to do that here too?
Thank you for pointing out. I think that the same things are necessary
here. Otherwise does it lead the case that the visibility map page is
set while the heap page bit is cleared?
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
On Mon, Apr 8, 2019 at 12:55 AM Andres Freund <andres@anarazel.de> wrote: > > Apparently the initial commit a892234f830e had MarkBufferDirty, but it > > was removed one week later by 77a1d1e79892. > > Good catch. Kinda looks like it could have been an accidental removal? > Robert? So you're talking about this hunk? - /* Page is marked all-visible but should be all-frozen */ - PageSetAllFrozen(page); - MarkBufferDirty(buf); - I don't remember exactly, but I am pretty sure that I assumed from the way that hunk looked that the MarkBufferDirty() was only needed there because of the call to PageSetAllFrozen(). Perhaps I should've figured it out from the "NB: If the heap page is all-visible..." comment, but I unfortunately don't find that comment to be very clear -- it basically says we don't need to do it, and then immediately contradicts itself by saying we sometimes do need to do it "because it may be logged." But that's hardly an explanation, because why should the fact that the page is going to be logged require that it be dirtied? We could improve the comment, but before we go there... Why the heck does visibilitymap_set() require callers to do MarkBufferDirty() instead of doing it itself? Or at least, if it's got to work that way, can't it Assert() something? It seems crazy to me that it calls PageSetLSN() without calling MarkBufferDirty() or asserting that the buffer is dirty or having a header comment that says that the buffer must be dirty. Ugh. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi,
On 2019-04-08 10:59:32 -0400, Robert Haas wrote:
> On Mon, Apr 8, 2019 at 12:55 AM Andres Freund <andres@anarazel.de> wrote:
> > > Apparently the initial commit a892234f830e had MarkBufferDirty, but it
> > > was removed one week later by 77a1d1e79892.
> >
> > Good catch. Kinda looks like it could have been an accidental removal?
> > Robert?
>
> So you're talking about this hunk?
>
> - /* Page is marked all-visible but should be all-frozen */
> - PageSetAllFrozen(page);
> - MarkBufferDirty(buf);
> -
>
> I don't remember exactly, but I am pretty sure that I assumed from the
> way that hunk looked that the MarkBufferDirty() was only needed there
> because of the call to PageSetAllFrozen(). Perhaps I should've
> figured it out from the "NB: If the heap page is all-visible..."
> comment, but I unfortunately don't find that comment to be very clear
> -- it basically says we don't need to do it, and then immediately
> contradicts itself by saying we sometimes do need to do it "because it
> may be logged." But that's hardly an explanation, because why should
> the fact that the page is going to be logged require that it be
> dirtied? We could improve the comment, but before we go there...
> Why the heck does visibilitymap_set() require callers to do
> MarkBufferDirty() instead of doing it itself? Or at least, if it's
> got to work that way, can't it Assert() something? It seems crazy to
> me that it calls PageSetLSN() without calling MarkBufferDirty() or
> asserting that the buffer is dirty or having a header comment that
> says that the buffer must be dirty. Ugh.
I think the visibilitymap_set() has incrementally gotten worse, to the
point that it should just be redone. Initially, before you made it
crashsafe, it indeed didn't do any I/O (like the header claims), and
didn't touch the heap. To make it crashsafe it started to call back into
heap. Then to support checksums, most of its callers had to take be
adapted around marking buffers dirty. And then the all-frozen stuff
complicated it a bit further.
I don't quite know what the right answer is, but I think
visibilitymap_set (or whatever it's successor is), shouldn't do any WAL
logging of its own, and it shouldn't call into heap - we want to be able
to reuse the vm for things like zheap after all. It's also just an
extremely confusing interface, especially because say
visibilitymap_clear() doesn't do WAL logging.
I think we should have a heap_visibilitymap_set() that does the WAL
logging, which internally calls into visibilitymap.c.
Perhaps it could look something roughly like:
static void
heap_visibilitymap_set(...)
{
Assert(LWLockIsHeld(BufferDescriptorGetIOLock(heapBuf))
Assert(PageIsAllVisible(heapPage));
/* other checks */
START_CRIT_SECTION();
/* if change required, this locks VM buffer */
if (visibilitmap_start_set(rel, heapBlk, &vmBuf))
{
XLogRecPtr recptr;
MarkBufferDirty(heapBuf)
if (RelationNeedsWAL(rel))
{
/* inlined body of log_heap_visible */
recptr = XLogInsert(XLOG_HEAP2_VISIBLE);
/*
* If data checksums are enabled (or wal_log_hints=on), we
* need to protect the heap page from being torn.
*/
if (XLogHintBitIsNeeded())
{
Page heapPage = BufferGetPage(heapBuf);
/* caller is expected to set PD_ALL_VISIBLE first */
Assert(PageIsAllVisible(heapPage));
PageSetLSN(heapPage, recptr);
}
}
/* actually change bits, set page LSN, release vmbuf lock */
visibilitmap_finish_set(rel, heapBlk, vmbuf, recptr);
}
END_CRIT_SECTION();
}
The replay routines would then not use heap_visibilitymap_set (they
right now call visibilitymap_set with a valid XLogRecPtr), but instead
visibilitmap_redo_set() or such, which can check the LSNs etc.
Greetings,
Andres Freund