Hi,
On 2015-05-11 16:57:08 +0530, Abhijit Menon-Sen wrote:
> I've attached an updated patch for pgstatbloat, as well as a patch to
> replace two uses of BuildTupleFromCStrings() elsewhere in pgstattuple.
TBH, I'd rather not touch unrelated things right now. We're pretty badly
behind...
> I've made the changes you mentioned in your earlier mail, except that I
> have not changed the name pending further suggestions about what would
> be the best name.
I don't really care how it's named, as long as it makes clear that it's
not an exact measurement.
> > > I haven't checked, but I'm not sure that it's safe/meaningful to
> > > call PageGetHeapFreeSpace() on a new page.
> >
> > OK, I'll check and fix if necessary.
>
> You're right, PageGetHeapFreeSpace() isn't safe on a new page. I've
> added a guard to that call in the attached patch, but I'm not sure
> that's the right thing to do.
> Should I copy the orphaned new-page handling from lazy_scan_heap? What
> about for empty pages? Neither feels like a very good thing to do, but
> then neither does skipping the new page silently. Should I count the
> space it would have free if it were initialised, but leave the page
> alone for VACUUM to deal with? Or just leave it as it is?
I think there's absolutely no need for pgstattuple to do anything
here. It's not even desirable.
> + LockBuffer(buf, BUFFER_LOCK_SHARE);
> +
> + page = BufferGetPage(buf);
> +
> + if (!PageIsNew(page))
> + stat->free_space += PageGetHeapFreeSpace(page);
> +
> + if (PageIsNew(page) || PageIsEmpty(page))
> + {
> + UnlockReleaseBuffer(buf);
> + continue;
> + }
Shouldn't new pages be counted as being fully free or at least bloat?
Just disregarding them seems wrong.
Greetings,
Andres Freund