On Thu, Feb 18, 2016 at 11:52 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> It looks like this was done correctly to begin with, and I broke it in
> d287818eb514d431b1a68e1f3940cd958f82aa34. Not sure what I was thinking :-(
I think that you might not have simply changed the order in a totally
misguided way back in 2008, as you seem to imply. Consider what this
block does following your commit just now:
...
else if (P_IGNORE(opaque))
indexStat.empty_pages++; /* this is the "half dead" state */
else if (P_ISLEAF(opaque))
{
int max_avail;
max_avail = BLCKSZ - (BLCKSZ - ((PageHeader) page)->pd_special +
SizeOfPageHeaderData);
indexStat.max_avail += max_avail;
indexStat.free_space += PageGetFreeSpace(page);
indexStat.leaf_pages++;
/*
* If the next leaf is on an earlier block, it means a
* fragmentation.
*/
if (opaque->btpo_next != P_NONE && opaque->btpo_next < blkno)
indexStat.fragments++;
}
...
I think that the P_ISLEAF() instrumentation of free space and
fragments might still need to happen for deleted and/or half dead
pages. Having looked at the 2008 commit d287818eb514d431 myself, ISTM
that your intent might well have been to have that happen -- why else
would any reasonable person have changed the order at all?
The avg_leaf_density and leaf_fragmentation fields might now be argued
to misrepresent the true picture. This is not clearly a departure from
their documented behavior, if only because the descriptions are almost
the same as the names of the fields themselves. If you think that the
instrumentation of free space is the most useful possible behavior as
of today, which it might well be, then you might have clarified that
this behavior was your intent in today's commit, for example by
updating the descriptions of the fields avg_leaf_density and
leaf_fragmentation in the docs.
Just a thought.
--
Peter Geoghegan
