From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
cc: Robert Haas <robertmhaas@gmail.com>,
        "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>,
        Amit Kapila <amit.kapila16@gmail.com>
Subject: Re: Why are we PageInit'ing buffers in RelationAddExtraBlocks()?
In-reply-to: <20190115204428.kpqmk3ix6cenr7gi@alap3.anarazel.de>
References: <20181219083945.6khtgm36mivonhva@alap3.anarazel.de>
 <CA+TgmoZRwKbf6q7TXEpwMX_vdjfeGEKudR8mofnmTwZg6O9=kg@mail.gmail.com>
 <20181219223736.4k2lhqoj34aaxj3m@alap3.anarazel.de>
 <CA+TgmoaJ67VCefuhkd0fdJiWb64bqBtkbZazbVm6a0wimY0fRQ@mail.gmail.com>
 <13071.1545266373@sss.pgh.pa.us>
 <20181220005636.cowkudhiu666mhdi@alap3.anarazel.de>
 <20181220230411.zzwioucrg7d3ld2m@alap3.anarazel.de>
 <20190115204428.kpqmk3ix6cenr7gi@alap3.anarazel.de>
Comments: In-reply-to Andres Freund <andres@anarazel.de>
	message dated "Tue, 15 Jan 2019 12:44:28 -0800"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <11586.1547591721.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Tue, 15 Jan 2019 17:35:21 -0500
Message-ID: <11587.1547591721@sss.pgh.pa.us>
Precedence: bulk

Andres Freund <andres@anarazel.de> writes:
>> 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?

> Any comments on the approach in this patch?

I agree with the concept of postponing page init till we're actually
going to do something with the page.  However, the patch seems to need
some polish:

* There's a comment in RelationAddExtraBlocks, just above where you
changed, that

         * Extend by one page.  This should generally match the main-line
         * extension code in RelationGetBufferForTuple, except that we hol=
d
         * the relation extension lock throughout.

This seems to be falsified by this patch, in that one of the two paths
does PageInit and the other doesn't.

* s/unitialized pages/uninitialized pages/

* This bit in vacuumlazy seems unnecessarily confusing:

+            Size        freespace =3D 0;
...
+            if (GetRecordedFreeSpace(onerel, blkno) =3D=3D 0)
+                freespace =3D BufferGetPageSize(buf) - SizeOfPageHeaderDa=
ta;
+
+            if (freespace > 0)
+            {
+                RecordPageWithFreeSpace(onerel, blkno, freespace);

I'd write that as just

+            if (GetRecordedFreeSpace(onerel, blkno) =3D=3D 0)
+            {
+                Size    freespace =3D BufferGetPageSize(buf) - SizeOfPage=
HeaderData;
+                RecordPageWithFreeSpace(onerel, blkno, freespace);


I tend to agree that the DEBUG message isn't very necessary, or at least
could be lower than DEBUG1.

			regards, tom lane