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