Hi,
On 2018-12-19 16:56:36 -0800, Andres Freund wrote:
> On 2018-12-19 19:39:33 -0500, Tom Lane wrote:
> > Robert Haas <robertmh...@gmail.com> writes:
> > > On Wed, Dec 19, 2018 at 5:37 PM Andres Freund <and...@anarazel.de> wrote:
> > >> What's gained by the logic of emitting that warning in VACUUM after a
> > >> crash? I don't really see any robustness advantages in it.
> >
> > > I don't know. I am just normally reluctant to change things
> > > precipitously that are of long tenure.
> >
> > Me too, but I think Andres has a point here. Those warnings in VACUUM
> > are ancient, probably predating the introduction of WAL :-(. At the
> > time there was good reason to be suspicious of zeroed pages in tables.
> > Now, though, we have (what we think is) a bulletproof crash recovery
> > procedure in which possibly-zeroed pages are to be expected; so we're
> > just causing users unnecessary alarm by warning about them. I think
> > it's reasonable to, if not remove the messages entirely, at least
> > downgrade them to a low DEBUG level.
>
> Yea, I think that'd be reasonable.
>
> I think we ought to add them, as new/zero pages, to the FSM. If we did
> so both during vacuum and RelationAddExtraBlocks() we'd avoid the
> redundant writes, and avoid depending on heap layout in
> RelationAddExtraBlocks().
>
> I wonder if it'd make sense to only log a DEBUG if there's no
> corresponding FSM entry. That'd obviously not be bulltproof, but it'd
> reduce the spammyness considerably.
Here's a prototype patch implementing this change. I'm not sure the new
DEBUG message is really worth it?
Looking at the surrounding code made me wonder about the wisdom of
entering empty pages as all-visible and all-frozen into the VM. That'll
mean we'll never re-discover them on a primary, after promotion. There's
no mechanism to add such pages to the FSM on a standby (in contrast to
e.g. pages where tuples are modified), as vacuum will never visit that
page again. Now obviously it has the advantage of avoiding
re-processing the page in the next vacuum, but is that really an
important goal? If there's frequent vacuums, there got to be a fair
amount of modifications, which ought to lead to re-use of such pages at
a not too far away point?
Greetings,
Andres Freund
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index b8b5871559b..aa0bc4d8f9c 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -216,18 +216,15 @@ RelationAddExtraBlocks(Relation relation, BulkInsertState bistate)
BufferGetBlockNumber(buffer),
RelationGetRelationName(relation));
- PageInit(page, BufferGetPageSize(buffer), 0);
-
/*
- * We mark all the new buffers dirty, but do nothing to write them
- * out; they'll probably get used soon, and even if they are not, a
- * crash will leave an okay all-zeroes page on disk.
+ * Add the page to the FSM without initializing. If we were to
+ * initialize here the page would potentially get flushed out to disk
+ * before we add any useful content. There's no guarantee that that'd
+ * happen however, so we need to deal with unitialized pages anyway,
+ * thus let's avoid the potential for unnecessary writes.
*/
- MarkBufferDirty(buffer);
-
- /* we'll need this info below */
blockNum = BufferGetBlockNumber(buffer);
- freespace = PageGetHeapFreeSpace(page);
+ freespace = BufferGetPageSize(buffer) - SizeOfPageHeaderData;
UnlockReleaseBuffer(buffer);
@@ -479,6 +476,18 @@ loop:
* we're done.
*/
page = BufferGetPage(buffer);
+
+ /*
+ * Initialize page, it'll be used soon. We could avoid dirtying the
+ * buffer here, and rely on the caller to do so whenever it puts a
+ * tuple onto the page, but there seems not much benefit in doing so.
+ */
+ if (PageIsNew(page))
+ {
+ PageInit(page, BufferGetPageSize(buffer), 0);
+ MarkBufferDirty(buffer);
+ }
+
pageFreeSpace = PageGetHeapFreeSpace(page);
if (len + saveFreeSpace <= pageFreeSpace)
{
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index 8134c52253e..2d8d3569372 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -862,42 +862,42 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
if (PageIsNew(page))
{
/*
- * An all-zeroes page could be left over if a backend extends the
- * relation but crashes before initializing the page. Reclaim such
- * pages for use.
+ * All-zeroes pages can be left over if either a backend extends
+ * the relation by a single page, but crashes before initializing
+ * the page, or when bulk-extending the relation (which creates a
+ * number of empty pages at the tail end of the relation, but
+ * enters them into the FSM).
*
- * We have to be careful here because we could be looking at a
- * page that someone has just added to the relation and not yet
- * been able to initialize (see RelationGetBufferForTuple). To
- * protect against that, release the buffer lock, grab the
- * relation extension lock momentarily, and re-lock the buffer. If
- * the page is still uninitialized by then, it must be left over
- * from a crashed backend, and we can initialize it.
+ * Make sure these pages are in the FSM, to ensure they can be
+ * reused. Do that by testing if there's any space recorded for
+ * the page. If not, enter it. We log that case on a debug level.
*
- * We don't really need the relation lock when this is a new or
- * temp relation, but it's probably not worth the code space to
- * check that, since this surely isn't a critical path.
- *
- * Note: the comparable code in vacuum.c need not worry because
- * it's got exclusive lock on the whole relation.
+ * Note we do not enter the page into the visibilitymap. That has
+ * the downside that we repeatedly visit this page in subsequent
+ * vacuums, but otherwise we'll never not discover the space on a
+ * promoted standby. The harm of repeated checking ought to
+ * normally not be too bad - the space usually should be used at
+ * some point, otherwise there wouldn't be any regular vacuums.
+ */
+ Size freespace = 0;
+
+ empty_pages++;
+
+ /*
+ * Perform checking of FSM after releasing lock, the fsm is
+ * approximate, after all.
*/
- LockBuffer(buf, BUFFER_LOCK_UNLOCK);
- LockRelationForExtension(onerel, ExclusiveLock);
- UnlockRelationForExtension(onerel, ExclusiveLock);
- LockBufferForCleanup(buf);
- if (PageIsNew(page))
- {
- ereport(WARNING,
- (errmsg("relation \"%s\" page %u is uninitialized --- fixing",
- relname, blkno)));
- PageInit(page, BufferGetPageSize(buf), 0);
- empty_pages++;
- }
- freespace = PageGetHeapFreeSpace(page);
- MarkBufferDirty(buf);
UnlockReleaseBuffer(buf);
- RecordPageWithFreeSpace(onerel, blkno, freespace);
+ if (GetRecordedFreeSpace(onerel, blkno) == 0)
+ freespace = BufferGetPageSize(buf) - SizeOfPageHeaderData;
+
+ if (freespace > 0)
+ {
+ RecordPageWithFreeSpace(onerel, blkno, freespace);
+ elog(DEBUG1, "relation \"%s\" page %u is uninitialized and not in fsm, fixing",
+ relname, blkno);
+ }
continue;
}
@@ -2030,7 +2030,6 @@ count_nondeletable_pages(Relation onerel, LVRelStats *vacrelstats)
if (PageIsNew(page) || PageIsEmpty(page))
{
- /* PageIsNew probably shouldn't happen... */
UnlockReleaseBuffer(buf);
continue;
}
