Re: pgstattuple: fix free space calculation - Mailing list pgsql-hackers

Rafia Sabih <rafia.pghackers@gmail.com> writes:
> On Thu, 29 Aug 2024 at 16:53, Frédéric Yhuel <frederic.yhuel@dalibo.com>
> wrote:
>> So I think we should just use PageGetExactFreeSpace().
>> 
>> Here is a v3 patch. It's the same as v2, I only removed the last
>> paragraph in the commit message.

> Thanks for the new patch. LGTM.

I looked at this patch.  I agree with making the change.  However,
I don't agree with the CF entry's marking of "target version: stable"
(i.e., requesting back-patch).  I think this falls somewhere in the
gray area between a bug fix and a definitional change.  Also, people
are unlikely to be happy if they suddenly get new, not-comparable
numbers after a minor version update.  So I think we should just fix
it in HEAD.

As far as the patch itself goes, the one thing that is bothering me
is this comment change

         /*
-         * It's not safe to call PageGetHeapFreeSpace() on new pages, so we
+         * It's not safe to call PageGetExactFreeSpace() on new pages, so we
          * treat them as being free space for our purposes.
          */

which looks like it wasn't made with a great deal of thought.
Now it seems to me that the comment was already bogus when written:
there isn't anything uncertain about what will happen if you call
either of these functions on a "new" page.  PageIsNew checks for

        return ((PageHeader) page)->pd_upper == 0;

If pd_upper is 0, PageGet[Exact]FreeSpace is absolutely guaranteed
to return zero, even if pd_lower contains garbage.  And then
PageGetHeapFreeSpace will likewise return zero.  Perhaps there
could be trouble if we got into the line-pointer-checking part
of PageGetHeapFreeSpace, but we can't.  So this comment is wrong,
and is even more obviously wrong after the above change.  I thought
for a moment about removing the PageIsNew test altogether, but
then I decided that it probably *is* what we want and is just
mis-explained.  I think the comment should read more like

        /*
         * PageGetExactFreeSpace() will return zero for a "new" page,
         * but it's actually usable free space, so count it that way.
         */

Now alternatively you could argue that a "new" page isn't usable free
space yet and so we should count it as zero, just as we don't count
dead tuples as usable free space.  You need VACUUM to turn either of
those things into real free space.  But that'd be a bigger definitional
change, and I'm not sure we want it.  Thoughts?

Also, do we need any documentation change for this?  I looked through
https://www.postgresql.org/docs/devel/pgstattuple.html
and didn't see anything that was being very specific about what
"free space" means, so maybe it's fine as-is.

            regards, tom lane