a fast bloat measurement tool (was Re: Measuring relation free space)

Attached patch adds a new function to the pageinspect extension for
measuring total free space, in either tables or indexes. It returns the
free space as a percentage, so higher numbers mean more bloat. After
trying a couple of ways to quantify it, I've found this particular
measure correlates well with the nastiest bloat issues I've ran into in
production recently. For example, an index that had swelled to over 5X
its original size due to autovacuum issues registered at 0.86 on this
scale. I could easily see people putting an alert at something like
0.40 and picking candidates to reindex based on it triggering. That
would be about a million times smarter than how I've been muddling
through this class of problems so far.

Code by Jaime Casanova, based on a prototype by me. Thanks to attendees
and sponsors of the PgWest conference for helping to fund some deeper
exploration of this idea.

Here's a test case showing it in action:

create extension pageinspect;
create table t (k serial,v integer);
insert into t(v) (select generate_series(1,100000));
create index t_idx on t(k);
delete from t where k<50000;
vacuum t;

gsmith=# select relation_free_space('t');
relation_free_space
---------------------
0.445466

gsmith=# select relation_free_space('t_idx');
relation_free_space
---------------------
0.550946

Some open questions in my mind:

-Given this is doing a full table scan, should it hook into a ring
buffer to keep from trashing the buffer cache? Or might it loop over
the relation in a different way all together? I was thinking about
eyeing the FSM instead at one point, didn't explore that yet. There's
certainly a few ways to approach this, we just aimed at the easiest way
to get a working starter implementation, and associated results to
compare others against.

-Should there be a non-superuser version of this? We'd certainly need
to get a less cache demolishing version before that would seem wise.

-There were related things in the pageinspect module, but a case could
be made for this being a core function instead. It's a bit more likely
to be used in production than the rest of that extension.

-What if anything related to TOAST should this handle?

We're also planning to do a sampling version of this, using the same
approach ANALYZE does. Grab a number of blocks, extrapolate from
there. It shouldn't take many samples before the accuracy is better
than how people are estimated this now. That work is just waiting on
some better thinking about how to handle the full relation version first.

And, yes, the explanation in the docs and code should be clear that it's
returning a percentage, which I just realized when writing this. At
least I remembered to document something; still ahead of the average new
patch...

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us

On Sun, Nov 6, 2011 at 04:08, Greg Smith <greg(at)2ndquadrant(dot)com> wrote:
> Attached patch adds a new function to the pageinspect extension for
> measuring total free space, in either tables or indexes.  It returns the
> free space as a percentage, so higher numbers mean more bloat.  After trying
> a couple of ways to quantify it, I've found this particular measure
> correlates well with the nastiest bloat issues I've ran into in production
> recently.  For example, an index that had swelled to over 5X its original
> size due to autovacuum issues registered at 0.86 on this scale.  I could
> easily see people putting an alert at something like 0.40 and picking
> candidates to reindex based on it triggering.  That would be about a million
> times smarter than how I've been muddling through this class of problems so
> far.
>
> Code by Jaime Casanova, based on a prototype by me.  Thanks to attendees and
> sponsors of the PgWest conference for helping to fund some deeper
> exploration of this idea.

Looks pretty useful.

One quick stylistic comment - we don't generally use "* 1.0" to turn
an int into a double - just use a cast.

> -Given this is doing a full table scan, should it hook into a ring buffer to
> keep from trashing the buffer cache?  Or might it loop over the relation in
> a different way all together?  I was thinking about eyeing the FSM instead
> at one point, didn't explore that yet.  There's certainly a few ways to
> approach this, we just aimed at the easiest way to get a working starter
> implementation, and associated results to compare others against.

Hooking into a ring buffer seems like an almost requirement before you
can run this on a larger production system, wouldn't it? I don't know
how hard that is code-wise, but it certainly seems worthwhile.

> -Should there be a non-superuser version of this?  We'd certainly need to
> get a less cache demolishing version before that would seem wise.

Not sure that's necessary - at least not for now. Many other
diagnostics functions are already superuser only...

> -There were related things in the pageinspect module, but a case could be
> made for this being a core function instead.  It's a bit more likely to be
> used in production than the rest of that extension.

A case can be made for a lot of things in contrib to be in core ;) I
say let's keep it in pageinspect, but then also have you finish off
that "split up the contrib" patch :-)

> -What if anything related to TOAST should this handle?

Similar data for TOAST relations would be intersting, no? But that's
easily done from userspace by just querying to the toast table
specifically, I assume?

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

--On 6. November 2011 01:08:11 -0200 Greg Smith <greg(at)2ndQuadrant(dot)com> wrote:

> Attached patch adds a new function to the pageinspect extension for measuring
> total free space, in either tables or indexes.

I wonder if that should be done in the pgstattuple module, which output some
similar numbers.

--
Thanks

Bernd

On 07/11/11 10:20, Bernd Helmle wrote:
>
>
> --On 6. November 2011 01:08:11 -0200 Greg Smith <greg(at)2ndQuadrant(dot)com>
> wrote:
>
>> Attached patch adds a new function to the pageinspect extension for
>> measuring
>> total free space, in either tables or indexes.
>
> I wonder if that should be done in the pgstattuple module, which
> output some similar numbers.
>

Not meaning to disparage Greg's effort in any way, but I was thinking
the same thing about pg_freespacemap. I have not checked what - if any
differences there are in output, but it would be interesting to compare
which of the various (3 at present) extensions with slightly overlapping
areas of functionality should perhaps be merged.

I am guessing (at this point very much guessing) that pg_freespace map
may return its data faster, as pageinspect is gonna have to grovel
through all the pages for itself (whereas pg_freespacemap relies on
using info from the ... free space map fork).

regards

Mark

On 11/06/2011 11:55 PM, Mark Kirkwood wrote:
> I am guessing (at this point very much guessing) that pg_freespace map
> may return its data faster, as pageinspect is gonna have to grovel
> through all the pages for itself (whereas pg_freespacemap relies on
> using info from the ... free space map fork).

I started with pageinspect because I wasn't sure if other methods would
be as accurate. For example, I wasn't sure until just before submission
that only the free space and size of the relation are needed to get a
useful measure here; at one point I was considering some other things
too. I've ruled them out as unnecessary as far as I can tell, but I
can't claim my tests are exhaustive. Some review confirmation that this
measure is useful for other people is one thing I was hoping for
feedback on, as one thing to consider in addition to the actual
implementation.

If this measurement is the only one needed, than as I said at the start
of the thread here it might easily be implemented to run just against
the free space map instead. I'm thinking of what's been sent so far as
a UI with matching output it should produce. If it's possible to get
the same numbers faster, exactly how to implement the function under the
hood is easy enough to change. Jaime already has a new version in
development that adds a ring buffer for example.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us

On Sun, Nov 6, 2011 at 5:38 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>
> Looks pretty useful.
>

thanks for the review, attached is a new version of it

> One quick stylistic comment - we don't generally use "* 1.0" to turn
> an int into a double - just use a cast.
>

ok

>
> Hooking into a ring buffer seems like an almost requirement before you
> can run this on a larger production system, wouldn't it? I don't know
> how  hard that is code-wise, but it certainly seems worthwhile.
>

seems it wasn't too difficult... i just have to indicate the right
buffer access strategy so it's using a ring buffer now

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

On Tue, Nov 8, 2011 at 1:07 PM, Greg Smith <greg(at)2ndquadrant(dot)com> wrote:
> On 11/06/2011 11:55 PM, Mark Kirkwood wrote:
>
> I am guessing (at this point very much guessing) that pg_freespace map may
> return its data faster, as pageinspect is gonna have to grovel through all
> the pages for itself (whereas pg_freespacemap relies on using info from the
> ... free space map fork).
>
> I started with pageinspect because I wasn't sure if other methods would be
> as accurate.  For example, I wasn't sure until just before submission that
> only the free space and size of the relation are needed to get a useful
> measure here; at one point I was considering some other things too.  I've
> ruled them out as unnecessary as far as I can tell, but I can't claim my
> tests are exhaustive.  Some review confirmation that this measure is useful
> for other people is one thing I was hoping for feedback on, as one thing to
> consider in addition to the actual implementation.
>
> If this measurement is the only one needed, than as I said at the start of
> the thread here it might easily be implemented to run just against the free
> space map instead.  I'm thinking of what's been sent so far as a UI with
> matching output it should produce.  If it's possible to get the same numbers
> faster, exactly how to implement the function under the hood is easy enough
> to change.  Jaime already has a new version in development that adds a ring
> buffer for example.

It's already easy to get "good enough" numbers based on user space
tools with very little overhead, so I think it's more important that
the server side tool be accurate rather than fast. Of course, if we
can get both, that's a bonus, but I'd rather not go that route at the
expense of accuracy. Just my .02.

Robert Treat
conjecture: xzilla.net
consulting: omniti.com

On 11/08/2011 05:07 PM, Robert Treat wrote:
> It's already easy to get "good enough" numbers based on user space
> tools with very little overhead, so I think it's more important that
> the server side tool be accurate rather than fast.

What user space method do you consider good enough here? I haven't
found any approximation that I was really happy with; wouldn't have
bothered with this otherwise.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us

Excerpts from Jaime Casanova's message of mar nov 08 18:12:25 -0300 2011:
> On Sun, Nov 6, 2011 at 5:38 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> >
> > Looks pretty useful.
>
> thanks for the review, attached is a new version of it

Note that AFAIK you shouldn't update the 1.0 extension script ... you
have to create a 1.1 version (or whatever), update the default version
in the control file, and create an 1.0--1.1 script to upgrade from the
original version to 1.1.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Tue, Nov 8, 2011 at 7:19 PM, Greg Smith <greg(at)2ndquadrant(dot)com> wrote:
> On 11/08/2011 05:07 PM, Robert Treat wrote:
>>
>> It's already easy to get "good enough" numbers based on user space
>> tools with very little overhead, so I think it's more important that
>> the server side tool be accurate rather than fast.
>
> What user space method do you consider good enough here?  I haven't found
> any approximation that I was really happy with; wouldn't have bothered with
> this otherwise.
>

check_postgres and the pg_bloat_report both use a method of comparing
on disk size vs estimated size based on table structure (or index
info). Run regularly, it's certainly possible to keep bloat under
control. That said, I'd still like to see something more accurate.

Robert Treat
conjecture: xzilla.net
consulting: omniti.com

On Wed, Nov 9, 2011 at 7:58 AM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
>
> Excerpts from Jaime Casanova's message of mar nov 08 18:12:25 -0300 2011:
>> On Sun, Nov 6, 2011 at 5:38 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>> >
>> > Looks pretty useful.
>>
>> thanks for the review, attached is a new version of it
>
> Note that AFAIK you shouldn't update the 1.0 extension script ... you
> have to create a 1.1 version (or whatever), update the default version
> in the control file, and create an 1.0--1.1 script to upgrade from the
> original version to 1.1.
>

good point... fixed that...
a question i have is: are we supposed to let the old script (1.0) around?

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

On Mon, Nov 14, 2011 at 2:02 PM, Jaime Casanova <jaime(at)2ndquadrant(dot)com> wrote:
> On Wed, Nov 9, 2011 at 7:58 AM, Alvaro Herrera
> <alvherre(at)commandprompt(dot)com> wrote:
>>
>> Excerpts from Jaime Casanova's message of mar nov 08 18:12:25 -0300 2011:
>>> On Sun, Nov 6, 2011 at 5:38 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>>> >
>>> > Looks pretty useful.
>>>
>>> thanks for the review, attached is a new version of it
>>
>> Note that AFAIK you shouldn't update the 1.0 extension script ... you
>> have to create a 1.1 version (or whatever), update the default version
>> in the control file, and create an 1.0--1.1 script to upgrade from the
>> original version to 1.1.
>>
>
> good point... fixed that...
> a question i have is: are we supposed to let the old script (1.0) around?

Since the syntax to install a non-default version is supported, I
would argue the old script should be kept.
CREATE extension pageinspect with version "1.0"

This patch applies and builds cleanly. It works either for "CREATE
EXTENSION" from scratch, or for updating from the prior version with
"ALTER EXTENSION..UPDATE".

It seems to be using the buffer ring strategy as advertised.

It reports space that is free exclusively for updates as being free.
In other words, it considers space free even if it is reserved against
inserts in deference to fillfactor. This is in contrast to
pg_freespace, which only reports space available for inserts as being
available. I think this is reasonable behavior, but it is subtle and
should perhaps be documented. (Is it common to use fill factors other
than the default in the first place? Do we assume that people using
fillfactor are sophisticated enough not to shot themselves in the
foot?)

As noted by Greg, the documentation calls it "total amount of free
free [sic] space" when that is not what is reported. However, it also
is not reporting a percentage, but rather a decimal fraction. The
reported value should be multiplied by 100, especially if the docs are
going to be changed to call it a percentage.

Unless I am missing something, all indexes are handled via a procedure
designed for BTree indices, "GetBTRelationFreeSpace". I don't know
that the ultimate behavior of this is wrong, but it seems unusual. If
I get some more time, I will try to explore what is actually going on
when called on other types of indexes.

I have no insight into how to handle toast tables, or non-superusers.
I had thought that toast tables had names of their own which could be
used, but I could not figure out how to do that.

Even if there are other ways to get approximately the same
information, this functionality seems to be a natural thing to have in
the pageinspect extension.

Cheers,

Jeff

On 11/25/2011 04:42 PM, Jeff Janes wrote:
> It reports space that is free exclusively for updates as being free.
> In other words, it considers space free even if it is reserved against
> inserts in deference to fillfactor. This is in contrast to
> pg_freespace, which only reports space available for inserts as being
> available. I think this is reasonable behavior, but it is subtle and
> should perhaps be documented.

Ah, that's right, this is why I first wandered this specific path.
Ignoring fillfactor seems to have even more downsides as I see it.
Certainly deserves a doc improvement, as well as fixing the description
of the value so it's clearly a ratio rather than a true percentage.

> (Is it common to use fill factors other
> than the default in the first place? Do we assume that people using
> fillfactor are sophisticated enough not to shot themselves in the
> foot?)

It's not common, and I think anyone who sets fillfactor themselves would
understand the downside. The bigger risk are people who inherit designs
from others that use this feature, but the new person doesn't understand
it. If using this feature calls attention to a problem there that
prompts an investigation, I'd see that as a good thing, rather than a
foot shot.

> Unless I am missing something, all indexes are handled via a procedure
> designed for BTree indices, "GetBTRelationFreeSpace". I don't know
> that the ultimate behavior of this is wrong, but it seems unusual. If
> I get some more time, I will try to explore what is actually going on
> when called on other types of indexes.

This I think I'll punt back toward Jaime, as well as asking "did you
have a plan for TOAST here?"

On Mon, Nov 28, 2011 at 5:40 AM, Greg Smith <greg(at)2ndquadrant(dot)com> wrote:
>
>> Unless I am missing something, all indexes are handled via a procedure
>> designed for BTree indices, "GetBTRelationFreeSpace".  I don't know
>> that the ultimate behavior of this is wrong, but it seems unusual.  If
>> I get some more time, I will try to explore what is actually going on
>> when called on other types of indexes.
>
>
> This I think I'll punt back toward Jaime, as well as asking "did you have a
> plan for TOAST here?"
>

for indexes. it seems pageinspect only deals with btree indexes and i
neglected to put a similar limitation on this function... now, because
the free space is calculated using PageGetFreeSpace() for indexes it
should be doing the right thing for all kind of indexes, i only put
the function there because i was trying to avoid to create a new file.
But if the function is right for all kind of indexes that's maybe
enough to create a new file and rename the helper function so is
obvious that it can manage all kind of indexes

for toast tables. a simple test here seems to show that is as easy as
to add toast tables in the supported objects and treat them as normal
pages...

or there is something i'm missing?

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

On 11/28/2011 05:40 AM, Greg Smith wrote:
> Ignoring fillfactor seems to have even more downsides as I see it.
> Certainly deserves a doc improvement, as well as fixing the
> description of the value so it's clearly a ratio rather than a true
> percentage.

So: I'm very clear on what to do here now:

-Make the computation be in units that match it documetnation
-Take a look at other index types, as well as TOAST, at least to get the
easy ones right.
-Fully confirm the extension upgrade logic works as hoped

That's the must do stuff. Then there's two more features to consider
and do something with if sensible:

-Double check whether there is really a useful role in using
pg_freespace here. I don't think the numbers will be as good, but maybe
we don't care.
-Once the above is all sorted, add a second UI that samples some pages
and extrapolates, ANALYZE-style, rather than hitting everything.

This ones leaves as returned with feedback, feeling pretty good it will
be whipped into good shape for the last 9.2 CommitFest.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us

On Sun, Nov 06, 2011 at 10:20:49PM +0100, Bernd Helmle wrote:
> --On 6. November 2011 01:08:11 -0200 Greg Smith <greg(at)2ndQuadrant(dot)com> wrote:
>
>> Attached patch adds a new function to the pageinspect extension for measuring
>> total free space, in either tables or indexes.
>
> I wonder if that should be done in the pgstattuple module, which output
> some similar numbers.

Indeed, pgstattuple already claims to show precisely the same measure. Its
reckoning is right in line for heaps, but the proposed pageinspect function
finds more free space in indexes:

[local] test=# SELECT t.free_percent, relation_free_space('pg_proc'), i.free_percent, relation_free_space('pg_proc_proname_args_nsp_index') FROM pgstattuple('pg_proc') t, pgstattuple('pg_proc_proname_args_nsp_index') i;
free_percent | relation_free_space | free_percent | relation_free_space
--------------+---------------------+--------------+---------------------
2.53 | 0.0253346 | 8.61 | 0.128041
(1 row)

Is one of those index figures simply wrong, or do they measure two senses of
free space, both of which are interesting to DBAs?

Thanks,
nm

On 12/15/2011 04:11 PM, Noah Misch wrote:
> Is one of those index figures simply wrong, or do they measure two senses of
> free space, both of which are interesting to DBAs?
>

I think the bigger one--the one I was aiming to measure--also includes
fill-factor space. It should be possible to isolate whether that's true
by running the function against a fresh index, or by trying tests with a
table where there's no useful fill. I need to add some of those to the
test example suite.

While in theory both measures of free space might be interesting to
DBAs, I'd prefer to have the one that reflects the lost space to
fill-factor if I'm checking an index. But as Robert Treat was pointing
out, even the very rough estimates being made with existing user-space
tools not using the contrib module features are helpful enough for a lot
of users. So long as it's easy and accuracy is good enough to find
bloated indexes, either implementation is probably good enough.

Shaking out the alternate implementation ideas was really my goal for
this CF here. The major goal of the next revision is to present the
options with a measure of their respective accuracy and runtime. If I
have to give up just a of bit of accuracy and make it much faster,
that's probably what most people want as an option. When Jaime and I
come back with an update, it really needs to have benchmarks and
accuracy numbers for each option. That may be complicated a bit
depending on how much of the table or index is cached, so isolating that
out will be a pain.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us

On Fri, Dec 16, 2011 at 02:02:03AM -0500, Greg Smith wrote:
> On 12/15/2011 04:11 PM, Noah Misch wrote:
>> Is one of those index figures simply wrong, or do they measure two senses of
>> free space, both of which are interesting to DBAs?
>
> I think the bigger one--the one I was aiming to measure--also includes
> fill-factor space. It should be possible to isolate whether that's true
> by running the function against a fresh index, or by trying tests with a
> table where there's no useful fill. I need to add some of those to the
> test example suite.

No, both measures include fillfactor space. From a brief look at the code, the
proposed function counts space in non-leaf pages, while pgstattuple does not.
Also, the proposed function counts half-dead pages like live pages, while
pgstattuple counts them like dead pages.

One could perhaps justify those choices either way, but they seem too esoteric
for DBA exposure. I recommend choosing a policy on each and making both
pgstattuple() and any new code respect that policy.

> Shaking out the alternate implementation ideas was really my goal for
> this CF here. The major goal of the next revision is to present the
> options with a measure of their respective accuracy and runtime. If I
> have to give up just a of bit of accuracy and make it much faster,
> that's probably what most people want as an option. When Jaime and I
> come back with an update, it really needs to have benchmarks and
> accuracy numbers for each option. That may be complicated a bit
> depending on how much of the table or index is cached, so isolating that
> out will be a pain.

The previous submission seemed to boil down to a speedier version of "SELECT
free_percent FROM pgstattuple('foo')". (Some of the other statistics aren't
cheap.) Considering that, the code does belong in the pgstattuple module.

The sampling approach you have mentioned sounds promising, especially for
indexes. For heap bloat, it may be hard to improve on pg_freespacemap-based and
check_postgres-style estimates with anything less than a full heap scan.

Thanks,
nm

On Thu, Dec 15, 2011 at 4:11 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Sun, Nov 06, 2011 at 10:20:49PM +0100, Bernd Helmle wrote:
>> --On 6. November 2011 01:08:11 -0200 Greg Smith <greg(at)2ndQuadrant(dot)com> wrote:
>>
>>> Attached patch adds a new function to the pageinspect extension for measuring
>>> total free space, in either tables or indexes.
>>
>> I wonder if that should be done in the pgstattuple module, which output
>> some similar numbers.
>
> Indeed, pgstattuple already claims to show precisely the same measure.  Its
> reckoning is right in line for heaps, but the proposed pageinspect function
> finds more free space in indexes:
>
> [local] test=# SELECT t.free_percent, relation_free_space('pg_proc'), i.free_percent, relation_free_space('pg_proc_proname_args_nsp_index') FROM pgstattuple('pg_proc') t, pgstattuple('pg_proc_proname_args_nsp_index') i;
>  free_percent | relation_free_space | free_percent | relation_free_space
> --------------+---------------------+--------------+---------------------
>         2.53 |           0.0253346 |         8.61 |            0.128041
> (1 row)
>
> Is one of those index figures simply wrong, or do they measure two senses of
> free space, both of which are interesting to DBAs?
>

i created a test env using pgbench -s 20 -F 90, i then create a new
table (that keep tracks actions that happens the the pgbench tables,
so insert only) and changed a few fillfactors:
"""
relname | reltuples | reloptions
-------------------------------------+---- -------+------------------
audit_log | 804977 |
pgbench_accounts | 1529890 | {fillfactor=90}
pgbench_accounts_pkey | 1529890 | {fillfactor=50}
pgbench_branches | 20 | {fillfactor=100}
pgbench_branches_pkey | 20 |
pgbench_history | 94062 |
pgbench_tellers | 200 | {fillfactor=100}
pgbench_tellers_pkey | 200 |
(8 rows)
"""

and after running "pgbench -n -c 4 -j 2 -T 300" a few times, i used
attached free_space.sql to see what pg_freespacemap, pgstattuple and
relation_free_space had to say about these tables. the result is
attached in result_free_space.out

my first conclusion is that pg_freespacemap is unreliable when indexes
are involved (and looking at the documentation of that module confirms
that), also the fact that FSM is not designed for accuracy make me
think is not an option.

pgstattuple and relation_free_space are very close in all the numbers
except for 2 indexes pgbench_branches_pkey and pgbench_tellers_pkey;
after a VACUUM FULL and a REINDEX (and the difference persistence) i
checked pgbench_tellers_pkey contents (it has only one page besides
the metapage) and the numbers that i look at where:

page size: 8192
free size: 4148

which in good romance means 50% of free space... so, answering Noah's
question: if that difference has some meaning i can't see it but
looking at the evidence the measure relation_free_space() is giving is
the good one

so, tomorrow (or ...looking at the clock... later today) i will update
the relation_free_space() patch to accept toast tables and other kind
of indexes and add it to the commitfest unless someone says that my
math is wrong and somehow there is a more accurate way of getting the
free space (which is entirely possible)

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

On Sat, Jan 14, 2012 at 04:41:57AM -0500, Jaime Casanova wrote:
> pgstattuple and relation_free_space are very close in all the numbers
> except for 2 indexes pgbench_branches_pkey and pgbench_tellers_pkey;
> after a VACUUM FULL and a REINDEX (and the difference persistence) i
> checked pgbench_tellers_pkey contents (it has only one page besides
> the metapage) and the numbers that i look at where:
>
> page size: 8192
> free size: 4148
>
> which in good romance means 50% of free space... so, answering Noah's
> question: if that difference has some meaning i can't see it but
> looking at the evidence the measure relation_free_space() is giving is
> the good one
>
> so, tomorrow (or ...looking at the clock... later today) i will update
> the relation_free_space() patch to accept toast tables and other kind
> of indexes and add it to the commitfest unless someone says that my
> math is wrong and somehow there is a more accurate way of getting the
> free space (which is entirely possible)

Did you see this followup[1]? To summarize:

- pgstattuple() and relation_free_space() should emit the same number, even if
that means improving pgstattuple() at the same time.
- relation_free_space() belongs in the pgstattuple extension, because its role
is cheaper access to a single pgstattuple() metric.

Thanks,
nm

[1] http://archives.postgresql.org/message-id/20111218165625.GB6393@tornado.leadboat.com

On Sat, Jan 14, 2012 at 6:26 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
>
> - pgstattuple() and relation_free_space() should emit the same number, even if
>  that means improving pgstattuple() at the same time.

yes, i just wanted to understand which one was more accurate and
why... and give the opportunity for anyone to point my error if any

> - relation_free_space() belongs in the pgstattuple extension, because its role
>  is cheaper access to a single pgstattuple() metric.
>

oh! right! so, what about just fixing the free_percent that
pgstattuple is providing

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

On Sat, Jan 14, 2012 at 02:40:46PM -0500, Jaime Casanova wrote:
> On Sat, Jan 14, 2012 at 6:26 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> >
> > - pgstattuple() and relation_free_space() should emit the same number, even if
> > ?that means improving pgstattuple() at the same time.
>
> yes, i just wanted to understand which one was more accurate and
> why... and give the opportunity for anyone to point my error if any

pgstattuple()'s decision to treat half-dead pages like deleted pages is
better. That transient state can only end in the page's deletion.

I don't know about counting non-leaf pages, but I personally wouldn't revisit
pgstattuple()'s decision there. In the indexes I've briefly surveyed, the
ratio of leaf pages to non-leaf pages is 100:1 or better. No amount of bloat
in that 1% will matter. Feel free to make the argument if you think
otherwise, though; I've only taken a brief look at the topic.

> > - relation_free_space() belongs in the pgstattuple extension, because its role
> > ?is cheaper access to a single pgstattuple() metric.
>
> oh! right! so, what about just fixing the free_percent that
> pgstattuple is providing

If pgstattuple() meets your needs, you might indeed no longer need any patch.

On Mon, Jan 16, 2012 at 5:09 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
>
> pgstattuple()'s decision to treat half-dead pages like deleted pages is
> better.  That transient state can only end in the page's deletion.
>

the only page in that index has 200 records (all live 0 dead) using
half the page size (which is a leaf page and is not half dead, btw).
so, how do you justify that pgstattuple say we have just 25% of free
space?

postgres=# SELECT * from bt_page_stats('pgbench_tellers_pkey', 1);
-[ RECORD 1 ]-+-----
blkno | 1
type | l
live_items | 200
dead_items | 0
avg_item_size | 16
page_size | 8192
free_size | 4148
btpo_prev | 0
btpo_next | 0
btpo | 0
btpo_flags | 3

> I don't know about counting non-leaf pages

ignoring all non-leaf pages still gives a considerable difference
between pgstattuple and relation_free_space()

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

On Wed, Jan 18, 2012 at 09:46:20AM -0500, Jaime Casanova wrote:
> On Mon, Jan 16, 2012 at 5:09 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> >
> > pgstattuple()'s decision to treat half-dead pages like deleted pages is
> > better. ?That transient state can only end in the page's deletion.
> >
>
> the only page in that index has 200 records (all live 0 dead) using
> half the page size (which is a leaf page and is not half dead, btw).
> so, how do you justify that pgstattuple say we have just 25% of free
> space?
>
> postgres=# SELECT * from bt_page_stats('pgbench_tellers_pkey', 1);
> -[ RECORD 1 ]-+-----
> blkno | 1
> type | l
> live_items | 200
> dead_items | 0
> avg_item_size | 16
> page_size | 8192
> free_size | 4148
> btpo_prev | 0
> btpo_next | 0
> btpo | 0
> btpo_flags | 3
>
> > I don't know about counting non-leaf pages
>
> ignoring all non-leaf pages still gives a considerable difference
> between pgstattuple and relation_free_space()

pgstattuple() counts the single B-tree meta page as always-full, while
relation_free_space() skips it for all purposes. For tiny indexes, that can
shift the percentage dramatically.

On Wed, Jan 18, 2012 at 7:01 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Wed, Jan 18, 2012 at 09:46:20AM -0500, Jaime Casanova wrote:
>>
>> ignoring all non-leaf pages still gives a considerable difference
>> between pgstattuple and relation_free_space()
>
> pgstattuple() counts the single B-tree meta page as always-full, while
> relation_free_space() skips it for all purposes.  For tiny indexes, that can
> shift the percentage dramatically.
>

ok, i will reformulate the question. why is fine ignoring non-leaf
pages but is not fine to ignore the meta page?

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

On Fri, Jan 20, 2012 at 07:03:22PM -0500, Jaime Casanova wrote:
> On Wed, Jan 18, 2012 at 7:01 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> > On Wed, Jan 18, 2012 at 09:46:20AM -0500, Jaime Casanova wrote:
> >>
> >> ignoring all non-leaf pages still gives a considerable difference
> >> between pgstattuple and relation_free_space()
> >
> > pgstattuple() counts the single B-tree meta page as always-full, while
> > relation_free_space() skips it for all purposes. ?For tiny indexes, that can
> > shift the percentage dramatically.
> >
>
> ok, i will reformulate the question. why is fine ignoring non-leaf
> pages but is not fine to ignore the meta page?

pgstattuple() figures the free_percent by adding up all space available to
hold tuples and dividing that by the simple size of the relation. Non-leaf
pages and the meta page get identical treatment: both never hold tuples, so
they do not contribute to the free space.

Excerpts from Noah Misch's message of vie ene 20 22:33:30 -0300 2012:
> On Fri, Jan 20, 2012 at 07:03:22PM -0500, Jaime Casanova wrote:
> > On Wed, Jan 18, 2012 at 7:01 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> > > On Wed, Jan 18, 2012 at 09:46:20AM -0500, Jaime Casanova wrote:
> > >>
> > >> ignoring all non-leaf pages still gives a considerable difference
> > >> between pgstattuple and relation_free_space()
> > >
> > > pgstattuple() counts the single B-tree meta page as always-full, while
> > > relation_free_space() skips it for all purposes. ?For tiny indexes, that can
> > > shift the percentage dramatically.
> > >
> >
> > ok, i will reformulate the question. why is fine ignoring non-leaf
> > pages but is not fine to ignore the meta page?
>
> pgstattuple() figures the free_percent by adding up all space available to
> hold tuples and dividing that by the simple size of the relation. Non-leaf
> pages and the meta page get identical treatment: both never hold tuples, so
> they do not contribute to the free space.

Hm. Leaf pages hold as much tuples as non-leaf pages, no? I mean
for each page element there's a value and a CTID. In non-leaf those
CTIDs point to other index pages, one level down the tree; in leaf pages
they point to the heap.

The metapage is special in that it is not used to store any user data,
just a pointer to the root page.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Mon, Jan 23, 2012 at 04:56:24PM -0300, Alvaro Herrera wrote:
> Excerpts from Noah Misch's message of vie ene 20 22:33:30 -0300 2012:
> > pgstattuple() figures the free_percent by adding up all space available to
> > hold tuples and dividing that by the simple size of the relation. Non-leaf
> > pages and the meta page get identical treatment: both never hold tuples, so
> > they do not contribute to the free space.
>
> Hm. Leaf pages hold as much tuples as non-leaf pages, no? I mean
> for each page element there's a value and a CTID. In non-leaf those
> CTIDs point to other index pages, one level down the tree; in leaf pages
> they point to the heap.

That distinction seemed important when I sent my last message, but now I agree
that it's largely irrelevant for free space purposes. If someone feels like
doing it, +1 for making pgstattuple() count non-leaf free space.

Thanks,
nm

On Mon, Jan 23, 2012 at 7:18 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Mon, Jan 23, 2012 at 04:56:24PM -0300, Alvaro Herrera wrote:
>>
>> Hm.  Leaf pages hold as much tuples as non-leaf pages, no?  I mean
>> for each page element there's a value and a CTID.  In non-leaf those
>> CTIDs point to other index pages, one level down the tree; in leaf pages
>> they point to the heap.
>
> That distinction seemed important when I sent my last message, but now I agree
> that it's largely irrelevant for free space purposes.  If someone feels like
> doing it, +1 for making pgstattuple() count non-leaf free space.
>

actually i agreed that non-leaf pages are irrelevant... i just
confirmed that in a production system with 300GB none of the indexes
in an 84M rows table nor in a heavily updated one has more than 1 root
page, all the rest are deleted, half_dead or leaf. so the posibility
of bloat coming from non-leaf pages seems very odd

but the possibility of bloat coming from the meta page doesn't exist,
AFAIUI at least

we need the most accurate value about usable free space, because the
idea is to add a sampler mode to the function so we don't scan the
whole relation. that's why we still need the function.

btw... pgstattuple also has the problem that it's not using a ring buffer

attached are two patches:
- v5: is the same original patch but only track space in leaf, deleted
and half_dead pages
- v5.1: adds the same for all kind of indexes (problem is that this is
inconsistent with the fact that pageinspect only manages btree indexes
for everything else)

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

On Tue, Jan 24, 2012 at 11:24:08AM -0500, Jaime Casanova wrote:
> On Mon, Jan 23, 2012 at 7:18 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> > If someone feels like
> > doing it, +1 for making pgstattuple() count non-leaf free space.
>
> actually i agreed that non-leaf pages are irrelevant... i just
> confirmed that in a production system with 300GB none of the indexes
> in an 84M rows table nor in a heavily updated one has more than 1 root
> page, all the rest are deleted, half_dead or leaf. so the posibility
> of bloat coming from non-leaf pages seems very odd

FWIW, the number to look at is internal_pages from pgstatindex():

[local] test=# create table t4 (c) as select * from generate_series(1,1000000);
SELECT 1000000
[local] test=# alter table t4 add primary key(c);
NOTICE: ALTER TABLE / ADD PRIMARY KEY will create implicit index "t4_pkey" for table "t4"
ALTER TABLE
[local] test=# select * from pgstatindex('t4_pkey');
-[ RECORD 1 ]------+---------
version | 2
tree_level | 2
index_size | 22478848
root_block_no | 290
internal_pages | 10
leaf_pages | 2733
empty_pages | 0
deleted_pages | 0
avg_leaf_density | 90.06
leaf_fragmentation | 0

So, 0.4% of this index. They appear in proportion to the logarithm of the
total index size. I agree that bloat centered on them is unlikely. Counting
them would be justified, but that is a question of formal accuracy rather than
practical importance.

> but the possibility of bloat coming from the meta page doesn't exist,
> AFAIUI at least
>
> we need the most accurate value about usable free space, because the
> idea is to add a sampler mode to the function so we don't scan the
> whole relation. that's why we still need the function.

I doubt we'd add this function solely on the basis that a future improvement
will make it useful. For the patch to go in now, it needs to be useful now.
(This is not a universal principle, but it mostly holds for low-complexity
patches like this one.)

All my comments below would also apply to such a broader patch.

> btw... pgstattuple also has the problem that it's not using a ring buffer
>
>
> attached are two patches:
> - v5: is the same original patch but only track space in leaf, deleted
> and half_dead pages
> - v5.1: adds the same for all kind of indexes (problem is that this is
> inconsistent with the fact that pageinspect only manages btree indexes
> for everything else)

Let's take a step back. Again, what you're proposing is essentially a faster
implementation of "SELECT free_percent FROM pgstattuple(rel)". If this code
belongs in core at all, it belongs in the pgstattuple module. Share as much
infrastructure as is reasonable between the user-visible functions of that
module. For example, I'm suspecting that the pgstat_index() call tree should
be shared, with pgstat_index_page() checking a flag to decide whether to
gather per-tuple stats.

Next, compare the bits of code that differ between pgstattuple() and
relation_free_space(), convincing yourself that the differences are justified.
Each difference will yield one of the following conclusions:

1. Your code contains an innovation that would apply to both functions. Where
not too difficult, merge these improvements into pgstattuple(). In order for
a demonstration of your new code's better performance to be interesting, we
must fix the same low-hanging fruit in its competitor. One example is the use
of the bulk read strategy. Another is the support for SP-GiST.

2. Your code is missing an essential behavior of pgstattuple(). Add it to
your code. One example is the presence of CHECK_FOR_INTERRUPTS() calls.

3. Your code behaves differently from pgstattuple() due to a fundamental
difference in their tasks. These are the only functional differences that
ought to remain in your finished patch; please point them out in comments.
For example, pgstat_heap() visits every tuple in the heap. You'll have no
reason to do that; pgstattuple() only needs it to calculate statistics other
than free_percent.

In particular, I call your attention to the fact that pgstattuple() takes
shared buffer content locks before examining pages. Your proposed patch does
not do so. I do not know with certainty whether that falls under #1 or #2.
The broad convention is to take such locks, because we elsewhere want an exact
answer. These functions are already inexact; they make no effort to observe a
consistent snapshot of the table. If you convince yourself that the error
arising from not locking buffers is reasonably bounded, we can lose the locks
(in both functions -- conclusion #1). Comments would then be in order.

With all that done, run some quick benchmarks: see how "SELECT free_percent
FROM pgstattuple(rel)" fares compared to "SELECT relation_free_space(rel)" for
a large heap and for a large B-tree index. If the timing difference is too
small to be interesting to you, remove relation_free_space() and submit your
pgstattuple() improvements alone. Otherwise, submit as written.

I suppose I didn't make all of this clear enough earlier; sorry for that.

Thanks,
nm

On Wed, Jan 25, 2012 at 9:47 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
>
> With all that done, run some quick benchmarks: see how "SELECT free_percent
> FROM pgstattuple(rel)" fares compared to "SELECT relation_free_space(rel)" for
> a large heap and for a large B-tree index.  If the timing difference is too
> small to be interesting to you, remove relation_free_space() and submit your
> pgstattuple() improvements alone.  Otherwise, submit as written.
>

Ok. I split this in three patches.

1) pgstattuple-gin_spgist.patch
This first patch adds gin and spgist support to pgstattuple, also
makes pgstattuple use a ring buffer when reading tables or indexes.

2) pgstattuple-relation_free_space.patch
This patch adds the relation_free_space function to pgstattuple.

the function relation_free_space() is faster than pgstattuple(), to
test that i initialize pgbench with a scale of 40.
In that context pgstattuple() tooks 1.4s to process pgbench_account
table and relation_free_space() tooks 730ms (half the time!)
In the index the difference is less notorious, 170ms the former and
150ms the latter.

3) pgstattuple-stats_target.patch
This patch adds a stats_target parameter to the relation_free_space()
function, it mimics the way analyze choose the blocks to read and is
faster than plain relation_free_space() but of course could be inexact
if the pages that we don't read are the ones with more free space

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

On Tue, Feb 14, 2012 at 02:04:26AM -0500, Jaime Casanova wrote:
> On Wed, Jan 25, 2012 at 9:47 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> >
> > With all that done, run some quick benchmarks: see how "SELECT free_percent
> > FROM pgstattuple(rel)" fares compared to "SELECT relation_free_space(rel)" for
> > a large heap and for a large B-tree index. ?If the timing difference is too
> > small to be interesting to you, remove relation_free_space() and submit your
> > pgstattuple() improvements alone. ?Otherwise, submit as written.
> >
>
> Ok. I split this in three patches.
>
> 1) pgstattuple-gin_spgist.patch
> This first patch adds gin and spgist support to pgstattuple, also
> makes pgstattuple use a ring buffer when reading tables or indexes.

The buffer access strategy usage bits look fine to commit. The gin and spgist
support has problems, detailed below.

> 2) pgstattuple-relation_free_space.patch
> This patch adds the relation_free_space function to pgstattuple.
>
> the function relation_free_space() is faster than pgstattuple(), to
> test that i initialize pgbench with a scale of 40.
> In that context pgstattuple() tooks 1.4s to process pgbench_account
> table and relation_free_space() tooks 730ms (half the time!)
> In the index the difference is less notorious, 170ms the former and
> 150ms the latter.

Benchmarks lasting on the order of one second are far too short. I tried the
first two patches on this 6914 MiB table and 4284 MiB index:

create table t(n) as select * from generate_series(1,200000000);
create index on t(n);

This machine has about 1 GiB of memory available for disk cache, and I used
shared_buffers = 128MB. I used a regular assert-enabled build with
debug_assertions = off. Timings:

pgstattuple.free_percent, heap: runtime 166.2s; answer 0.34
pgstattuple.free_percent, index: runtime 110.9s; answer 9.83
relation_free_space, heap: runtime 165.1s; answer 0.00838721
relation_free_space, index: runtime 108.7s; answer 2.23692

Note the disagreement in answers and the nonsensical answer from the last
test. The numbers do line up for smaller tables and indexes that I tried.

During the pgstattuple() runs on the heap, CPU usage divided evenly between
user and iowait time. With relation_free_space(), iowait kept above 80%. For
the index, pgstattuple() managed 60-80% iowait and relation_free_space() again
kept above 80%. Even so, that did not produce any significant change in
runtime. I'm guessing that readahead was highly effective here, so the I/O
bound dictated elapsed time.

Bottom line, this patch can probably achieve 50% speedups on already-in-memory
relations. It can reduce the contribution to CPU load, but not the elapsed
runtime, for relations we largely pull from disk. Do those benefits justify
the additional user-visible interface? I suppose the sort of installation
that would benefit most is one just short of the tipping point of the database
size exceeding memory size. Larger databases could not afford either
function, and smaller databases don't need to watch bloat so closely.
Offhand, I think that the I/O savings of sampling will be the real win, and
it's not worth an extra user-visible function to get the CPU usage savings
this patch offers. Other opinions welcome.

> 3) pgstattuple-stats_target.patch
> This patch adds a stats_target parameter to the relation_free_space()
> function, it mimics the way analyze choose the blocks to read and is
> faster than plain relation_free_space() but of course could be inexact
> if the pages that we don't read are the ones with more free space

This part is a fresh submission. It is simple enough that I have reviewed it.
It gives the expected speedup. However, the results are wrong:

3 runs of pgstattuple('t', 5): 0.000171412, 0.000171876, 0.000169326
3 runs of pgstattuple('t', 10): 0.000336525, 0.000344404, 0.000341314

I can get an apparent infinite loop:

create table t0(n) as select * from generate_series(1,4000000);
create index on t0(n);

[local] test=# select relation_free_space('t0_n_idx', 100);
relation_free_space
---------------------
0.0982675

Time: 133.788 ms

[local] test=# select relation_free_space('t0_n_idx', 5);
Cancel request sent
ERROR: canceling statement due to user request
Time: 24655.481 ms

As a way forward, I suggest abandoning relation_free_space() entirely and
adding a sampling capability to pgstattuple(). There are clear gains to be
had from that method. The gains of splitting out the free percent calculation
from the other pgstattuple() calculations seem meager.

If you would like, submit the buffer strategy bits as a separate patch with
its own CF entry, noting that it arose from here. That part can be Ready for
Committer. I'm marking the original CF entry Returned with Feedback.

Patch 1:

> *** a/contrib/pgstattuple/pgstatindex.c
> --- b/contrib/pgstattuple/pgstatindex.c

> + /*
> + * Buffer access strategy for reading relations, it's simpler to keep it
> + * global because pgstat_*_page() functions read one buffer at a time.
> + * pgstat_heap() and pgstat_index() should initialize it before use.
> + */
> + BufferAccessStrategy bstrategy;

This variable should be static.

> *** 450,455 ****
> --- 471,538 ----
> }
>
> /*
> + * pgstat_gin_page -- check tuples in a gin page
> + */
> + static void
> + pgstat_gin_page(pgstattuple_type *stat, Relation rel, BlockNumber blkno)
> + {
> + Buffer buf;
> + Page page;
> +
> + buf = ReadBufferExtended(rel, MAIN_FORKNUM, blkno, RBM_NORMAL, bstrategy);
> + LockBuffer(buf, GIN_SHARE);
> + page = BufferGetPage(buf);
> +
> + if (GinPageIsDeleted(page))
> + {
> + /* recyclable page */
> + stat->free_space += BLCKSZ;
> + }
> + else if (GinPageIsLeaf(page) || GinPageIsData(page))
> + {
> + pgstat_index_page(stat, page, FirstOffsetNumber,
> + PageGetMaxOffsetNumber(page));
> + }
> + else
> + {
> + /* root or node */
> + }
> +
> + UnlockReleaseBuffer(buf);
> + }

Would you discuss, preferably in comments, the various page types found in GIN
indexes and why this code is correct for all of them? At a minimum, the
comment "root or node" is incorrect.

Another thing to consider and document is how you wish to count tuples. Your
implementation counts every key as a tuple. I think it would be more useful
to count every posting tree entry as a tuple, but I haven't given it a great
deal of thought.

> +
> + /*
> + * pgstat_spgist_page -- check tuples in a spgist page
> + */
> + static void
> + pgstat_spgist_page(pgstattuple_type *stat, Relation rel, BlockNumber blkno)
> + {
> + Buffer buf;
> + Page page;
> +
> + buf = ReadBufferExtended(rel, MAIN_FORKNUM, blkno, RBM_NORMAL, bstrategy);
> + LockBuffer(buf, BUFFER_LOCK_SHARE);
> + page = BufferGetPage(buf);
> +
> + if (SpGistPageIsDeleted(page))
> + {
> + /* recyclable page */
> + stat->free_space += BLCKSZ;
> + }
> + else if (SpGistPageIsLeaf(page))
> + {
> + pgstat_index_page(stat, page, FirstOffsetNumber,
> + PageGetMaxOffsetNumber(page));
> + }
> + else
> + {
> + /* root or node */
> + }
> +
> + UnlockReleaseBuffer(buf);
> + }

pgstat_index_page will not do the right thing for SP-GiST indexes. Only
SPGIST_LIVE tuples should count as live; the other tupstates should count as
dead. Also, pgstattuple gives me a tuple count of 119288 for an SP-GiST index
of a table containing only 40000 tuples. (Disclaimer: I took my first look at
the SP-GiST code today.)

Patch 3:

> *** a/contrib/pgstattuple/pgstattuple.c
> --- b/contrib/pgstattuple/pgstattuple.c
> *************** GetHeapRelationFreeSpace(Relation rel)
> *** 691,716 ****
> {
> /* Get the current relation length */
> LockRelationForExtension(rel, ExclusiveLock);
> ! nblocks = RelationGetNumberOfBlocks(rel);
> UnlockRelationForExtension(rel, ExclusiveLock);
>
> /* Quit if we've scanned the whole relation */
> if (blkno >= nblocks)
> {
> break;
> }
>
> ! for (; blkno < nblocks; blkno++)
> {
> CHECK_FOR_INTERRUPTS();
>
> buffer = ReadBufferExtended(rel, MAIN_FORKNUM, blkno, RBM_NORMAL, bstrategy);
> LockBuffer(buffer, BUFFER_LOCK_SHARE);
>
> page = BufferGetPage(buffer);
> free_space += PageGetHeapFreeSpace(page);
>
> UnlockReleaseBuffer(buffer);
> }
> }
>
> --- 700,733 ----
> {
> /* Get the current relation length */
> LockRelationForExtension(rel, ExclusiveLock);
> ! totalBlocksInRelation = RelationGetNumberOfBlocks(rel);
> UnlockRelationForExtension(rel, ExclusiveLock);
>
> + nblocks = totalBlocksInRelation * stats_target / 100;

You have defined stats_target as a percentage of the relation to sample.
That's a poor basis for sample size. Use a constant number of pages, just as
a given default_statistics_target leads ANALYZE to take a constant number of
tuples from each table. Further background:
http://en.wikipedia.org/wiki/Sample_size_determination#Estimating_proportions_and_means

Excepting those few copied from analyze.c, this patch adds zero comments. You
even omit comments present in the corresponding analyze.c code.

nm

On Wed, Feb 22, 2012 at 12:27 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Tue, Feb 14, 2012 at 02:04:26AM -0500, Jaime Casanova wrote:
>>
>> 1) pgstattuple-gin_spgist.patch
>> This first patch adds gin and spgist support to pgstattuple, also
>> makes pgstattuple use a ring buffer when reading tables or indexes.
>
> The buffer access strategy usage bits look fine to commit.
>

ok. i extracted that part. which basically makes pgstattuple usable in
production (i mean, by not bloating shared buffers when using the
function)

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

On Fri, Mar 09, 2012 at 02:18:02AM -0500, Jaime Casanova wrote:
> On Wed, Feb 22, 2012 at 12:27 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> > On Tue, Feb 14, 2012 at 02:04:26AM -0500, Jaime Casanova wrote:
> >>
> >> 1) pgstattuple-gin_spgist.patch
> >> This first patch adds gin and spgist support to pgstattuple, also
> >> makes pgstattuple use a ring buffer when reading tables or indexes.
> >
> > The buffer access strategy usage bits look fine to commit.
> >
>
> ok. i extracted that part. which basically makes pgstattuple usable in
> production (i mean, by not bloating shared buffers when using the
> function)

I created a CF entry for this and marked it Ready for Committer. You left the
bstrategy variable non-static, but that didn't seem important enough to
justify another round trip.

On Mon, Mar 12, 2012 at 9:41 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
>
> I created a CF entry for this and marked it Ready for Committer.

i wasn't sure if create an entry this late was a good idea or not...
but now i feel better because is less probable that it will fall out
on the cracks, thanks

> You left the
> bstrategy variable non-static, but that didn't seem important enough to
> justify another round trip.
>

ah! i forgot that...

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

On Mon, Mar 12, 2012 at 11:10 PM, Jaime Casanova <jaime(at)2ndquadrant(dot)com> wrote:
> On Mon, Mar 12, 2012 at 9:41 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
>>
>> I created a CF entry for this and marked it Ready for Committer.
>
> i wasn't sure if create an entry this late was a good idea or not...
> but now i feel better because is less probable that it will fall out
> on the cracks, thanks
>
>> You left the
>> bstrategy variable non-static, but that didn't seem important enough to
>> justify another round trip.
>>
>
> ah! i forgot that...

I committed this, but I didn't like the global variable, so I adjusted
it to pass bstrategy as a parameter where needed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

This is a follow-up to the thread at
http://www.postgresql.org/message-id/4EB5FA1B.1090305@2ndQuadrant.com

A quick summary: that thread proposed adding a relation_free_space()
function to the pageinspect extension. Various review comments were
received, among which was the suggestion that the code belonged in
pg_stattuple as a faster way to calculate free_percent.

===

I've attached an extension that produces largely pgstattuple-compatible
numbers for a table without doing a full-table scan.

It scans through the table, skipping blocks that have their visibility
map bit set. For such pages, it gets the free space from the free space
map, and assumes that all remaining space on the page is taken by live
tuples. It scans other pages tuple-by-tuple and counts live and dead
tuples and free space.

Here's a comparison of fastbloat and pgstattuple output on a 50-million
row table with some holes created with a single big DELETE statement:

ams=# select * from fastbloat('x');
table_len | scanned_percent | approx_tuple_count | approx_tuple_len | approx_tuple_percent | dead_tuple_count | dead_tuple_len | dead_tuple_percent | free_space | free_percent
------------+-----------------+--------------------+------------------+----------------------+------------------+----------------+--------------------+------------+--------------
6714761216 | 17 | 41318301 | 5483815648 | 81.67 | 8681708 | 1111258624 | 16.55 | 80972912 | 1.21
(1 row)

Time: 639.455 ms

ams=# select * from pgstattuple('x');
table_len | tuple_count | tuple_len | tuple_percent | dead_tuple_count | dead_tuple_len | dead_tuple_percent | free_space | free_percent
------------+-------------+------------+---------------+------------------+----------------+--------------------+------------+--------------
6714761216 | 41318292 | 5288741376 | 78.76 | 8681708 | 1111258624 | 16.55 | 91810372 | 1.37
(1 row)

Time: 15610.651 ms

In the above, the table_len is nblocks*BLCKSZ, and the dead_tuple_count,
dead_tuple_len, dead_tuple_percent, free_space, and free_percent are all
exact. scanned_percent shows the percentage of pages that were scanned
tuple-by-tuple (the others having been skipped based on the VM bit).
The live tuple count, size, and percentage are all estimates.

The approx_tuple_count is calculated using vac_estimate_reltuples based
on the pages/tuples that were scanned. The approx_tuple_len is the exact
size of the live tuples on scanned pages, plus the approximate size from
skipped pages (BLCKSZ-GetRecordedFreeSpace()). This is an overestimate,
because it's counting the line pointer array as live tuple space.

Even in the worst case, when every page has dead tuples, fastbloat is
marginally faster than pgstattuple. The same table as the first example,
but with every even-numbered row deleted:

ams=# select * from fastbloat('x');
table_len | scanned_percent | approx_tuple_count | approx_tuple_len | approx_tuple_percent | dead_tuple_count | dead_tuple_len | dead_tuple_percent | free_space | free_percent
------------+-----------------+--------------------+------------------+----------------------+------------------+----------------+--------------------+------------+--------------
6714761216 | 100 | 20659150 | 2644371200 | 39.38 | 20659142 | 2644370176 | 39.38 | 1203068996 | 17.92
(1 row)

Time: 8924.511 ms

ams=# select * from pgstattuple('x');
table_len | tuple_count | tuple_len | tuple_percent | dead_tuple_count | dead_tuple_len | dead_tuple_percent | free_space | free_percent
------------+-------------+------------+---------------+------------------+----------------+--------------------+------------+--------------
6714761216 | 20659150 | 2644371200 | 39.38 | 20659142 | 2644370176 | 39.38 | 1203068996 | 17.92
(1 row)

Time: 13338.712 ms

Since the code depends on the visibility map to determine which pages to
skip, it does not support indexes (which have no visibility map).

(Just drop the attached files into contrib/fastbloat, and "make install"
should just work. Then just "create extension fastbloat".)

Questions and suggestions welcome.

-- Abhijit

On Wed, Apr 2, 2014 at 5:41 PM, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com> wrote:
> I've attached an extension that produces largely pgstattuple-compatible
> numbers for a table without doing a full-table scan.
>
> It scans through the table, skipping blocks that have their visibility
> map bit set. For such pages, it gets the free space from the free space
> map, and assumes that all remaining space on the page is taken by live
> tuples. It scans other pages tuple-by-tuple and counts live and dead
> tuples and free space.

That's clever. I think it might underestimate free space relative to
tuples because the free space map isn't guaranteed to be completely
correct. But I guess you knew that already...

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

At 2014-04-02 20:10:54 -0400, robertmhaas(at)gmail(dot)com wrote:
>
> I think it might underestimate free space relative to tuples because
> the free space map isn't guaranteed to be completely correct. But I
> guess you knew that already...

Yes, and tuple_len is already a slight overestimate (because it counts
the line pointer array as live tuple space for skipped pages). But for
the speedup it provides, I believe the result is still useful.

I'll mention the potential for free-space inaccuracy in the README.

-- Abhijit

On Thu, Apr 3, 2014 at 3:11 AM, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com>
wrote:
>
> This is a follow-up to the thread at
> http://www.postgresql.org/message-id/4EB5FA1B.1090305@2ndQuadrant.com
>
> A quick summary: that thread proposed adding a relation_free_space()
> function to the pageinspect extension.

> Various review comments were
> received, among which was the suggestion that the code belonged in
> pg_stattuple as a faster way to calculate free_percent.

You haven't mentioned why you didn't follow that way. After looking
at code, I also felt that it is better to add this as a version of
pg_stattuple.

> ===
>
> I've attached an extension that produces largely pgstattuple-compatible
> numbers for a table without doing a full-table scan.
>
> It scans through the table, skipping blocks that have their visibility
> map bit set. For such pages, it gets the free space from the free space
> map, and assumes that all remaining space on the page is taken by live
> tuples. It scans other pages tuple-by-tuple and counts live and dead
> tuples and free space.

Is this assumption based on the reason that if the visibility map bit of
page is set, then there is high chance that vacuum would have pruned
the dead tuples and updated FSM with freespace?
In anycase, I think it will be better if you update README and or
code comments to mention the reason of such an assumption.

1. compilation errors
1>contrib\fastbloat\fastbloat.c(450): error C2198: 'MultiXactIdIsRunning' :
too few arguments for call
1>contrib\fastbloat\fastbloat.c(467): error C2198: 'MultiXactIdIsRunning' :
too few arguments for call

Recent commit 05315 added new parameter to this API, so this usage
of API needs to be updated accordingly.

2.
/* Returns a tuple with live/dead tuple statistics for the named table.
*/

I think this is not a proper multi-line comment.

3.
fbstat_heap()
{
..
for (blkno = 0; blkno < nblocks; blkno++)
{
..
}

It is better to have CHECK_FOR_INTERRUPTS() in above loop.

4.
if (PageIsNew(page))
{
ReleaseBuffer(buf);
continue;
}

Incase of new page don't you need to account for freespace.
Why can't we safely assume that all the space in page is
free and add it to freespace.

5. Don't we need the handling for empty page (PageIsEmpty()) case?

6.
ItemIdIsDead(itemid))
{
continue;
}

What is the reason for not counting ItemIdIsDead() case for
accounting of dead tuples.

I think for Vaccum, it is okay to skip that case because same
is counted via heap_page_prune() call.

7.
HeapTupleSatisfiesVacuumNoHint()
a. Why can't we use HeapTupleSatisfiesVisibility() with dirty snapshot
as we use for pgstattuple? I think it will have the advantage of
keeping the code for fastbloat similar to pgstattuple.
b. If we want to use HeapTupleSatisfiesVacuum(), why can't we
add one parameter to function which can avoid setting of hintbit.
Are you worried about the performance impact it might cause in
other flows, if yes then which flow your are mainly worried.
c. I agree that there is advantage of avoiding hintbit code, but as
this operation will not be frequent, so not sure if its good idea
to add a separate version of tuplevisibility function

In general, I think the main advantage of this patch comes from the
fact that it can skip scanning pages based on visibilitymap, so
it seems to me that it is better if we keep other part of code similar
to pgstattuple.

8.
HeapTupleSatisfiesVacuumNoHint(HeapTuple htup, TransactionId OldestXmin)

There is some new code added in corresponding function
HeapTupleSatisfiesVacuum(), so if we want to use it, then
update the changes in this function as well and I think it
is better to move this into tqual.c along with other similar
functions.

9.
* This routine is shared by VACUUM and ANALYZE.
*/
double
vac_estimate_reltuples(Relation relation, bool is_analyze,
BlockNumber total_pages,
BlockNumber scanned_pages,
double scanned_tuples)

function header of vac_estimate_reltuples() suggests that it is
used by VACUUM and ANALYZE, I think it will be better to update
the comment w.r.t new usage as well.

10. I think it should generate resource file as is done for other modules
if you want to keep it as a separate module.
Example:
MODULE_big = btree_gin
OBJS = btree_gin.o $(WIN32RES)

11.
README.txt
> Here is an example comparing the output of fastbloat and pgstattuple for
the same table:

Do you really need to specify examples in README. I think it would be
better to specify examples in documentation (.sgml).

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Hi Amit.

Thanks for your comments, and I'm sorry it's taken me so long to
respond.

At 2014-08-03 11:18:57 +0530, amit(dot)kapila16(at)gmail(dot)com wrote:
>
> After looking at code, I also felt that it is better to add this as a
> version of pg_stattuple.

I started off trying to do that, but now I'm afraid I strongly disagree.
First, pg_stattuple works on relations and indexes, whereas fastbloat
only works on relations (because of the VM/FSM use). Second, the code
began to look hideous when mushed together, with too many ifs to avoid
locking I didn't need and so on. The logic is just too different.

> Is this assumption based on the reason that if the visibility map bit
> of page is set, then there is high chance that vacuum would have
> pruned the dead tuples and updated FSM with freespace?

Right. I'm not convinced an explanation of the VM/FSM belongs in the
fastbloat documentation, but I'll see if I can make it clearer.

> 1. compilation errors […]
> I think this is not a proper multi-line comment. […]
> It is better to have CHECK_FOR_INTERRUPTS() in above loop. […]
> Incase of new page don't you need to account for freespace. […]
> 5. Don't we need the handling for empty page (PageIsEmpty()) case?

Thanks, have fixed, will push updates soon.

> What is the reason for not counting ItemIdIsDead() case for
> accounting of dead tuples.

Will reconsider and fix.

> 7.
> HeapTupleSatisfiesVacuumNoHint()
> a. Why can't we use HeapTupleSatisfiesVisibility() with dirty snapshot
> as we use for pgstattuple?

Heavier locking. I tried to make do with the existing HTS* functions,
but fastbloat was noticeably faster in tests with the current setup.

> b. If we want to use HeapTupleSatisfiesVacuum(), why can't we add one
> parameter to function which can avoid setting of hintbit.

This is certainly a possibility, and as you say it would be better in
terms of maintenance. I didn't do it that way because I wanted to keep
the extension self-contained rather than make it depend on core changes.
If there's consensus on adding a nohint parameter, sure, I can do that.
(But the fastbloat won't work on current versions, which would suck.)

In the meantime, I'll merge the later updates from HTSVacuum into my
code. Thanks for the heads-up.

> function header of vac_estimate_reltuples() suggests that it is
> used by VACUUM and ANALYZE, I think it will be better to update
> the comment w.r.t new usage as well.

OK.

> 10. I think it should generate resource file as is done for other
> modules if you want to keep it as a separate module.

Thanks, will do.

> Do you really need to specify examples in README.

I don't *need* to, but I like it.

-- Abhijit

Hi,

On 2014-09-24 14:26:37 +0530, Abhijit Menon-Sen wrote:
> Thanks for your comments, and I'm sorry it's taken me so long to
> respond.
>
> At 2014-08-03 11:18:57 +0530, amit(dot)kapila16(at)gmail(dot)com wrote:
> >
> > After looking at code, I also felt that it is better to add this as a
> > version of pg_stattuple.
>
> I started off trying to do that, but now I'm afraid I strongly disagree.
> First, pg_stattuple works on relations and indexes, whereas fastbloat
> only works on relations (because of the VM/FSM use). Second, the code
> began to look hideous when mushed together, with too many ifs to avoid
> locking I didn't need and so on. The logic is just too different.

Why not add it to the stattuple extension, but as an independent
function (and file)? I don't really see a need for a completely new
extension, but a separate extension seems wasteful.

> > 7.
> > HeapTupleSatisfiesVacuumNoHint()
> > a. Why can't we use HeapTupleSatisfiesVisibility() with dirty snapshot
> > as we use for pgstattuple?

I think it's completely unacceptable to copy a visibility routine. And I
don't believe for a second that avoiding hint bit setting makes it legal
to not acquire a content lock for this. What's your reasoning for that
being safe? If you argue that all possible corruption has limited impact
you need to do that *much* more explicitly and verbosely.

Greetings,

Andres Freund

At 2014-09-24 11:09:24 +0200, andres(at)2ndquadrant(dot)com wrote:
>
> Why not add it to the stattuple extension, but as an independent
> function (and file)?

Thanks, that's a good idea. I'll do that.

> I think it's completely unacceptable to copy a visibility routine.

OK. Which visibility routine should I use, and should I try to create a
variant that doesn't set hint bits?

I don't have any reasoning for why it's safe to not hold a content lock.
If there is one, I need help to find it. If not, I'll acquire a content
lock. (If anyone can explain why it isn't safe, I would appreciate it.)

-- Abhijit

On 2014-09-25 10:24:39 +0530, Abhijit Menon-Sen wrote:
> At 2014-09-24 11:09:24 +0200, andres(at)2ndquadrant(dot)com wrote:
> > I think it's completely unacceptable to copy a visibility routine.
>
> OK. Which visibility routine should I use, and should I try to create a
> variant that doesn't set hint bits?

I've not yet followed your premise that you actually need one that
doesn't set hint bits...

> I don't have any reasoning for why it's safe to not hold a content lock.
> If there is one, I need help to find it. If not, I'll acquire a content
> lock. (If anyone can explain why it isn't safe, I would appreciate it.)

I don't see why it'd be safe. Without the content lock you can come
across half written tuple headers and similar things. You can try to
argue that all the possible faults of that are harmless, but I think
that'd require a tremendous amount of code review and it'd be hard to
continue to guarantee. There's a reason we acquire locks, you know :)

I think it's saner to first get this working & committed properly *with*
the lock. If you afterwards have energy to improve it further we can
another look.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

At 2014-09-25 11:41:29 +0200, andres(at)2ndquadrant(dot)com wrote:
>
> I've not yet followed your premise that you actually need one that
> doesn't set hint bits...

Oh.

All right, then I'll post a version that addresses Amit's other points,
adds a new file/function to pgstattuple, acquires content locks, and
uses HeapTupleSatisfiesVacuum, hint-bit setting and all. Maybe I'll
call it not_too_slow_bloat().

Thank you for the feedback.

-- Abhijit

On 25 September 2014 10:41, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-09-25 10:24:39 +0530, Abhijit Menon-Sen wrote:
>> At 2014-09-24 11:09:24 +0200, andres(at)2ndquadrant(dot)com wrote:
>> > I think it's completely unacceptable to copy a visibility routine.
>>
>> OK. Which visibility routine should I use, and should I try to create a
>> variant that doesn't set hint bits?
>
> I've not yet followed your premise that you actually need one that
> doesn't set hint bits...

Not least because I'm trying to solve a similar problem on another
thread, so no need to make a special case here.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2014-09-25 14:43:14 +0100, Simon Riggs wrote:
> On 25 September 2014 10:41, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On 2014-09-25 10:24:39 +0530, Abhijit Menon-Sen wrote:
> >> At 2014-09-24 11:09:24 +0200, andres(at)2ndquadrant(dot)com wrote:
> >> > I think it's completely unacceptable to copy a visibility routine.
> >>
> >> OK. Which visibility routine should I use, and should I try to create a
> >> variant that doesn't set hint bits?
> >
> > I've not yet followed your premise that you actually need one that
> > doesn't set hint bits...
>
> Not least because I'm trying to solve a similar problem on another
> thread, so no need to make a special case here.

That's mostly unrelated though - Abhijit wants to avoid them because he
tried to avoid having *any* form of lock on the buffer. That's the
reason he tried avoid hint bit setting. Since I don't believe that's
safe (at least there's by far not enough evidence about it), there's
simply no reason to avoid it.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Sep 24, 2014 at 2:26 PM, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com>
wrote:
>
> Hi Amit.
>
> Thanks for your comments, and I'm sorry it's taken me so long to
> respond.

No issues.

> At 2014-08-03 11:18:57 +0530, amit(dot)kapila16(at)gmail(dot)com wrote:

> > 7.
> > HeapTupleSatisfiesVacuumNoHint()
> > a. Why can't we use HeapTupleSatisfiesVisibility() with dirty snapshot
> > as we use for pgstattuple?
>
> Heavier locking. I tried to make do with the existing HTS* functions,
> but fastbloat was noticeably faster in tests with the current setup.

I am not sure for the purpose of this functionality, why we need to
use a different HTS (HeapTupleSatisfiesVacuum) routine as compare
to pgstat_heap(). Unless you have some specific purpose to achieve,
I think it is better to use HeapTupleSatisfiesVisibility().

> Maybe I'll call it not_too_slow_bloat().

How about pgfaststattuple() or pgquickstattuple() or pgfuzzystattuple()?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

At 2014-09-25 15:40:11 +0530, ams(at)2ndQuadrant(dot)com wrote:
>
> All right, then I'll post a version that addresses Amit's other
> points, adds a new file/function to pgstattuple, acquires content
> locks, and uses HeapTupleSatisfiesVacuum, hint-bit setting and all.

Sorry, I forgot to post this patch. It does what I listed above, and
seems to work fine (it's still quite a lot faster than pgstattuple
in many cases).

A couple of remaining questions:

1. I didn't change the handling of LP_DEAD items, because the way it is
now matches what pgstattuple counts. I'm open to changing it, though.
Maybe it makes sense to count LP_DEAD items iff lp_len != 0? Or just
leave it as it is? I think it doesn't matter much.

2. I changed the code to acquire the content locks on the buffer, as
discussed, but it still uses HeapTupleSatisfiesVacuum. Amit suggested
using HeapTupleSatisfiesVisibility, but it's not clear to me why that
would be better. I welcome advice in this matter.

(If anything, I should use HeapTupleIsSurelyDead, which doesn't set
any hint bits, but which I earlier rejected as too conservative in
its dead/not-dead decisions for this purpose.)

(I've actually patched the pgstattuple.sgml docs as well, but I want to
re-read that to make sure it's up to date, and didn't want to wait to
post the code changes.)

I also didn't change the name. I figure it's easy enough to change it
everywhere once all the remaining pieces are in place.

Comments welcome.

-- Abhijit

On 12/26/14 1:38 AM, Abhijit Menon-Sen wrote:
> At 2014-09-25 15:40:11 +0530,ams(at)2ndQuadrant(dot)com wrote:
>> >
>> >All right, then I'll post a version that addresses Amit's other
>> >points, adds a new file/function to pgstattuple, acquires content
>> >locks, and uses HeapTupleSatisfiesVacuum, hint-bit setting and all.
> Sorry, I forgot to post this patch. It does what I listed above, and
> seems to work fine (it's still quite a lot faster than pgstattuple
> in many cases).

Anything happen with this?
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

At 2015-01-26 18:47:29 -0600, Jim(dot)Nasby(at)BlueTreble(dot)com wrote:
>
> Anything happen with this?

Nothing so far.

-- Abhijit

On 1/27/15 3:56 AM, Abhijit Menon-Sen wrote:
> At 2015-01-26 18:47:29 -0600, Jim(dot)Nasby(at)BlueTreble(dot)com wrote:
>>
>> Anything happen with this?
>
> Nothing so far.

It would be best to get this into a commit fest so it's not lost.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

At 2015-01-27 17:00:27 -0600, Jim(dot)Nasby(at)BlueTreble(dot)com wrote:
>
> It would be best to get this into a commit fest so it's not lost.

It's there already.

-- Abhijit

Hi!

On 28.1.2015 05:03, Abhijit Menon-Sen wrote:
> At 2015-01-27 17:00:27 -0600, Jim(dot)Nasby(at)BlueTreble(dot)com wrote:
>>
>> It would be best to get this into a commit fest so it's not lost.
>
> It's there already.
>
> -- Abhijit
>
>

I looked at this patch today, so a few comments from me:

1) I believe the patch is slightly broken, because the version was
bumped to 1.3 but only partially, so I get this on "make install"

$ make -j9 -s install
/usr/bin/install: cannot stat ‘./pgstattuple--1.3.sql’: No such
file or directory
../../src/makefiles/pgxs.mk:129: recipe for target 'install' failed
make[1]: *** [install] Error 1
Makefile:85: recipe for target 'install-pgstattuple-recurse' failed
make: *** [install-pgstattuple-recurse] Error 2
make: *** Waiting for unfinished jobs....

The problem seems to be that Makefile already lists --1.3.sql in the
DATA section, but the file was not renamed (there's just --1.2.sql).

After fixing that, it's also necessary to fix the version in the
control file, otherwise the CREATE EXTENSION fails.

default_version = '1.2' -> '1.3'

At least that fixed the install for me.

2) Returning Datum from fbstat_heap, which is a static function seems a
bit strange to me, I'd use the HeapTuple directly, but meh ...

3) The way the tuple is built by first turning the values into strings
and then turned back into Datums by calling BuildTupleFromCStrings
is more serious I guess. Why not to just keep the Datums and call
heap_form_tuple directly?

I see the other functions in pgstattuple.c do use the string way, so
maybe there's a reason for that? Or is this just to keep the code
consistent in the extension?

4) I really doubt 'fastbloat' is a good name. I propose 'pgstatbloat'
to make that consistent with the rest of pgstattuple functions. Or
something similar, but 'fastbloat' is just plain confusing.

Otherwise, the code looks OK to me. Now, there are a few features I'd
like to have for production use (to minimize the impact):

1) no index support :-(

I'd like to see support for more relation types (at least btree
indexes). Are there any plans for that? Do we have an idea on how to
compute that?

2) sampling just a portion of the table

For example, being able to sample just 5% of blocks, making it less
obtrusive, especially on huge tables. Interestingly, there's a
TABLESAMPLE patch in this CF, so maybe it's possible to reuse some
of the methods (e.g. functions behind SYSTEM sampling)?

3) throttling

Another feature minimizing impact of running this on production might
be some sort of throttling, e.g. saying 'limit the scan to 4 MB/s'
or something along those lines.

4) prefetch

fbstat_heap is using visibility map to skip fully-visible pages,
which is nice, but if we skip too many pages it breaks readahead
similarly to bitmap heap scan. I believe this is another place where
effective_io_concurrency (i.e. prefetch) would be appropriate.

regards

--
Tomas Vondra http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2/22/15 5:41 PM, Tomas Vondra wrote:
> Otherwise, the code looks OK to me. Now, there are a few features I'd
> like to have for production use (to minimize the impact):
>
> 1) no index support:-(
>
> I'd like to see support for more relation types (at least btree
> indexes). Are there any plans for that? Do we have an idea on how to
> compute that?

It'd be cleaner if had actual an actual am function for this, but see below.

> 2) sampling just a portion of the table
>
> For example, being able to sample just 5% of blocks, making it less
> obtrusive, especially on huge tables. Interestingly, there's a
> TABLESAMPLE patch in this CF, so maybe it's possible to reuse some
> of the methods (e.g. functions behind SYSTEM sampling)?
>
> 3) throttling
>
> Another feature minimizing impact of running this on production might
> be some sort of throttling, e.g. saying 'limit the scan to 4 MB/s'
> or something along those lines.
>
> 4) prefetch
>
> fbstat_heap is using visibility map to skip fully-visible pages,
> which is nice, but if we skip too many pages it breaks readahead
> similarly to bitmap heap scan. I believe this is another place where
> effective_io_concurrency (i.e. prefetch) would be appropriate.

All of those wishes are solved in one way or another by vacuum and/or
analyze. If we had a hook in the tuple scanning loop and at the end of
vacuum you could just piggy-back on it. But really all we'd need for
vacuum to be able to report this info is one more field in LVRelStats, a
call to GetRecordedFreeSpace for all-visible pages, and some logic to
deal with pages skipped because we couldn't get the vacuum lock.

Should we just add this to vacuum instead?
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 23.2.2015 03:20, Jim Nasby wrote:
> On 2/22/15 5:41 PM, Tomas Vondra wrote:
>> Otherwise, the code looks OK to me. Now, there are a few features I'd
>> like to have for production use (to minimize the impact):
>>
>> 1) no index support:-(
>>
>> I'd like to see support for more relation types (at least btree
>> indexes). Are there any plans for that? Do we have an idea on how to
>> compute that?
>
> It'd be cleaner if had actual an actual am function for this, but see
> below.
>
>> 2) sampling just a portion of the table
>>
>> For example, being able to sample just 5% of blocks, making it less
>> obtrusive, especially on huge tables. Interestingly, there's a
>> TABLESAMPLE patch in this CF, so maybe it's possible to reuse some
>> of the methods (e.g. functions behind SYSTEM sampling)?
>>
>> 3) throttling
>>
>> Another feature minimizing impact of running this on production might
>> be some sort of throttling, e.g. saying 'limit the scan to 4 MB/s'
>> or something along those lines.
>>
>> 4) prefetch
>>
>> fbstat_heap is using visibility map to skip fully-visible pages,
>> which is nice, but if we skip too many pages it breaks readahead
>> similarly to bitmap heap scan. I believe this is another place where
>> effective_io_concurrency (i.e. prefetch) would be appropriate.
>
> All of those wishes are solved in one way or another by vacuum and/or
> analyze. If we had a hook in the tuple scanning loop and at the end of
> vacuum you could just piggy-back on it. But really all we'd need for
> vacuum to be able to report this info is one more field in LVRelStats, a
> call to GetRecordedFreeSpace for all-visible pages, and some logic to
> deal with pages skipped because we couldn't get the vacuum lock.
>
> Should we just add this to vacuum instead?

Possibly. I think the ultimate goal is to be able to get this info
easily and without disrupting the system performance too much (which is
difficult without sampling/throttling). If we can stuff that into
autovacuum reasonably, and then get the info from catalogs, I'm OK with
that.

However I'm not sure putting this into autovacuum is actually possible,
because how do you merge data from multiple partial runs (when each of
them skipped different pages)? Also, autovacuum is not the only place
where we free space - we'd have to handle HOT for example, I guess.

--
Tomas Vondra http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2/22/15 8:32 PM, Tomas Vondra wrote:
> On 23.2.2015 03:20, Jim Nasby wrote:
>> On 2/22/15 5:41 PM, Tomas Vondra wrote:
>>> Otherwise, the code looks OK to me. Now, there are a few features I'd
>>> like to have for production use (to minimize the impact):
>>>
>>> 1) no index support:-(
>>>
>>> I'd like to see support for more relation types (at least btree
>>> indexes). Are there any plans for that? Do we have an idea on how to
>>> compute that?
>>
>> It'd be cleaner if had actual an actual am function for this, but see
>> below.
>>
>>> 2) sampling just a portion of the table
>>>
>>> For example, being able to sample just 5% of blocks, making it less
>>> obtrusive, especially on huge tables. Interestingly, there's a
>>> TABLESAMPLE patch in this CF, so maybe it's possible to reuse some
>>> of the methods (e.g. functions behind SYSTEM sampling)?
>>>
>>> 3) throttling
>>>
>>> Another feature minimizing impact of running this on production might
>>> be some sort of throttling, e.g. saying 'limit the scan to 4 MB/s'
>>> or something along those lines.
>>>
>>> 4) prefetch
>>>
>>> fbstat_heap is using visibility map to skip fully-visible pages,
>>> which is nice, but if we skip too many pages it breaks readahead
>>> similarly to bitmap heap scan. I believe this is another place where
>>> effective_io_concurrency (i.e. prefetch) would be appropriate.
>>
>> All of those wishes are solved in one way or another by vacuum and/or
>> analyze. If we had a hook in the tuple scanning loop and at the end of
>> vacuum you could just piggy-back on it. But really all we'd need for
>> vacuum to be able to report this info is one more field in LVRelStats, a
>> call to GetRecordedFreeSpace for all-visible pages, and some logic to
>> deal with pages skipped because we couldn't get the vacuum lock.
>>
>> Should we just add this to vacuum instead?
>
> Possibly. I think the ultimate goal is to be able to get this info
> easily and without disrupting the system performance too much (which is
> difficult without sampling/throttling). If we can stuff that into
> autovacuum reasonably, and then get the info from catalogs, I'm OK with
> that.

Doing the counting in vacuum/analyze (auto or not) is quite easy, and it
would happen at the same time we're doing useful work. We would
automatically get the benefit of the throttling and sampling work that
those routines already do.

> However I'm not sure putting this into autovacuum is actually possible,
> because how do you merge data from multiple partial runs (when each of
> them skipped different pages)?

ISTM that's just a form of sampling, no?

Besides, we don't need the same lock for figuring out bloat. We could
still measure bloat even if we can't vacuum the page, but I think that's
overkill. If we're skipping enough pages to mess with the bloat
measurement then we most likely need to teach vacuum how to revisit pages.

> Also, autovacuum is not the only place
> where we free space - we'd have to handle HOT for example, I guess.

I wasn't thinking about trying to keep live bloat statistics, so HOT
wouldn't affect this.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 24.2.2015 19:08, Jim Nasby wrote:
> On 2/22/15 8:32 PM, Tomas Vondra wrote:
>> On 23.2.2015 03:20, Jim Nasby wrote:
>>> On 2/22/15 5:41 PM, Tomas Vondra wrote:
>>>> Otherwise, the code looks OK to me. Now, there are a few features I'd
>>>> like to have for production use (to minimize the impact):
>>>>
>>>> 1) no index support:-(
>>>>
>>>> I'd like to see support for more relation types (at least btree
>>>> indexes). Are there any plans for that? Do we have an idea on
>>>> how to
>>>> compute that?
>>>
>>> It'd be cleaner if had actual an actual am function for this, but see
>>> below.
>>>
>>>> 2) sampling just a portion of the table
>>>>
>>>> For example, being able to sample just 5% of blocks, making it
>>>> less
>>>> obtrusive, especially on huge tables. Interestingly, there's a
>>>> TABLESAMPLE patch in this CF, so maybe it's possible to reuse some
>>>> of the methods (e.g. functions behind SYSTEM sampling)?
>>>>
>>>> 3) throttling
>>>>
>>>> Another feature minimizing impact of running this on production
>>>> might
>>>> be some sort of throttling, e.g. saying 'limit the scan to 4 MB/s'
>>>> or something along those lines.
>>>>
>>>> 4) prefetch
>>>>
>>>> fbstat_heap is using visibility map to skip fully-visible pages,
>>>> which is nice, but if we skip too many pages it breaks readahead
>>>> similarly to bitmap heap scan. I believe this is another place
>>>> where
>>>> effective_io_concurrency (i.e. prefetch) would be appropriate.
>>>
>>> All of those wishes are solved in one way or another by vacuum and/or
>>> analyze. If we had a hook in the tuple scanning loop and at the end of
>>> vacuum you could just piggy-back on it. But really all we'd need for
>>> vacuum to be able to report this info is one more field in LVRelStats, a
>>> call to GetRecordedFreeSpace for all-visible pages, and some logic to
>>> deal with pages skipped because we couldn't get the vacuum lock.
>>>
>>> Should we just add this to vacuum instead?
>>
>> Possibly. I think the ultimate goal is to be able to get this info
>> easily and without disrupting the system performance too much (which is
>> difficult without sampling/throttling). If we can stuff that into
>> autovacuum reasonably, and then get the info from catalogs, I'm OK with
>> that.
>
> Doing the counting in vacuum/analyze (auto or not) is quite easy, and it
> would happen at the same time we're doing useful work. We would
> automatically get the benefit of the throttling and sampling work that
> those routines already do.
>
>> However I'm not sure putting this into autovacuum is actually possible,
>> because how do you merge data from multiple partial runs (when each of
>> them skipped different pages)?
>
> ISTM that's just a form of sampling, no?

Maybe.

I was thinking about collecting the necessary info during the VACUUM
phase, and somehow keeping track of free space in the whole table. I
thought there would be trouble exactly because this phase only processes
pages that possibly need vacuuming (so it wouldn't be a truly random
sample, making the estimation tricky).

But maybe that's not really true and it is possible to do that somehow.
For example what if we kept track of how much space each VACUUM freed,
and keeping running sum?

It might also be done during the ANALYZE, but that seems a bit
complicated because that's based on a sample of rows, not pages. Also,
the autovacuum_analyze_factor is 0.2 by default, so could end up with up
to 20% bloat without knowing it (vacuum_factor=0.1 is not great either,
but it's better).

> Besides, we don't need the same lock for figuring out bloat. We
> could still measure bloat even if we can't vacuum the page, but I
> think that's overkill. If we're skipping enough pages to mess with
> the bloat measurement then we most likely need to teach vacuum how to
> revisit pages.
>
>> Also, autovacuum is not the only place
>> where we free space - we'd have to handle HOT for example, I guess.
>
> I wasn't thinking about trying to keep live bloat statistics, so HOT
> wouldn't affect this.

I'm afraid this might cause the estimate to drift away over time, but I
guess it depends on implementation - e.g. if doing this in ANALYZE, it'd
be mostly immune to this, while with collecting incremental info during
VACUUM it might be a problem I guess.

Anyway, we don't have a patch trying to do that (certainly not in this
CF). I think it makes sense to add fastbloat() to pageinspect. Maybe
we'll get autovacuum-based solution in the future, but we don't have
that right now.

Actually, wouldn't that be a nice GSoC project? The scope seems about
right, not touching too many parts of the code base, etc.

--
Tomas Vondra http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2/25/15 2:56 PM, Tomas Vondra wrote:
> On 24.2.2015 19:08, Jim Nasby wrote:
>> On 2/22/15 8:32 PM, Tomas Vondra wrote:
>>> On 23.2.2015 03:20, Jim Nasby wrote:
>>>> On 2/22/15 5:41 PM, Tomas Vondra wrote:
>>>>> Otherwise, the code looks OK to me. Now, there are a few features I'd
>>>>> like to have for production use (to minimize the impact):
>>>>>
>>>>> 1) no index support:-(
>>>>>
>>>>> I'd like to see support for more relation types (at least btree
>>>>> indexes). Are there any plans for that? Do we have an idea on
>>>>> how to
>>>>> compute that?
>>>>
>>>> It'd be cleaner if had actual an actual am function for this, but see
>>>> below.
>>>>
>>>>> 2) sampling just a portion of the table
>>>>>
>>>>> For example, being able to sample just 5% of blocks, making it
>>>>> less
>>>>> obtrusive, especially on huge tables. Interestingly, there's a
>>>>> TABLESAMPLE patch in this CF, so maybe it's possible to reuse some
>>>>> of the methods (e.g. functions behind SYSTEM sampling)?
>>>>>
>>>>> 3) throttling
>>>>>
>>>>> Another feature minimizing impact of running this on production
>>>>> might
>>>>> be some sort of throttling, e.g. saying 'limit the scan to 4 MB/s'
>>>>> or something along those lines.
>>>>>
>>>>> 4) prefetch
>>>>>
>>>>> fbstat_heap is using visibility map to skip fully-visible pages,
>>>>> which is nice, but if we skip too many pages it breaks readahead
>>>>> similarly to bitmap heap scan. I believe this is another place
>>>>> where
>>>>> effective_io_concurrency (i.e. prefetch) would be appropriate.
>>>>
>>>> All of those wishes are solved in one way or another by vacuum and/or
>>>> analyze. If we had a hook in the tuple scanning loop and at the end of
>>>> vacuum you could just piggy-back on it. But really all we'd need for
>>>> vacuum to be able to report this info is one more field in LVRelStats, a
>>>> call to GetRecordedFreeSpace for all-visible pages, and some logic to
>>>> deal with pages skipped because we couldn't get the vacuum lock.
>>>>
>>>> Should we just add this to vacuum instead?
>>>
>>> Possibly. I think the ultimate goal is to be able to get this info
>>> easily and without disrupting the system performance too much (which is
>>> difficult without sampling/throttling). If we can stuff that into
>>> autovacuum reasonably, and then get the info from catalogs, I'm OK with
>>> that.
>>
>> Doing the counting in vacuum/analyze (auto or not) is quite easy, and it
>> would happen at the same time we're doing useful work. We would
>> automatically get the benefit of the throttling and sampling work that
>> those routines already do.
>>
>>> However I'm not sure putting this into autovacuum is actually possible,
>>> because how do you merge data from multiple partial runs (when each of
>>> them skipped different pages)?
>>
>> ISTM that's just a form of sampling, no?
>
> Maybe.
>
> I was thinking about collecting the necessary info during the VACUUM
> phase, and somehow keeping track of free space in the whole table. I
> thought there would be trouble exactly because this phase only processes
> pages that possibly need vacuuming (so it wouldn't be a truly random
> sample, making the estimation tricky).

Well, all-visible pages can still be listed in the FSM, so we'd have at
least that info. I don't think there can be dead tuples on an
all-visible page; if so, that means free space is all we care about. So
when pulling bloat info we'd just have to scan the VSM and FSM;
presumably that's pretty cheap.

> But maybe that's not really true and it is possible to do that somehow.
> For example what if we kept track of how much space each VACUUM freed,
> and keeping running sum?

Well, then we'd also have to keep track of space we used. I don't think
we want to do that.

On a completely unrelated note, the idea of logging details about vacuum
runs in a table is appealing. Forcing users to something like pgBadger
for that data seems pretty silly to me. But that's obviously a
completely separate discussion from this one.

> It might also be done during the ANALYZE, but that seems a bit
> complicated because that's based on a sample of rows, not pages.

Right, but you've got the page anyway so I doubt it'd cost much extra to
measure the bloat on it. Might want to guard against counting a page
twice, but...

> Also,
> the autovacuum_analyze_factor is 0.2 by default, so could end up with up
> to 20% bloat without knowing it (vacuum_factor=0.1 is not great either,
> but it's better).

... I don't think it's practical to get this bloat measurement terribly
precise, no do I think we need to. Anyone that needs better than 20%
accuracy can probably afford to fire off a manual vacuum at the same
time (or run a scan-only version).

>> Besides, we don't need the same lock for figuring out bloat. We
>> could still measure bloat even if we can't vacuum the page, but I
>> think that's overkill. If we're skipping enough pages to mess with
>> the bloat measurement then we most likely need to teach vacuum how to
>> revisit pages.
>>
>>> Also, autovacuum is not the only place
>>> where we free space - we'd have to handle HOT for example, I guess.
>>
>> I wasn't thinking about trying to keep live bloat statistics, so HOT
>> wouldn't affect this.
>
> I'm afraid this might cause the estimate to drift away over time, but I
> guess it depends on implementation - e.g. if doing this in ANALYZE, it'd
> be mostly immune to this, while with collecting incremental info during
> VACUUM it might be a problem I guess.

Yeah, I just don't see the need for that level of accuracy.

> Anyway, we don't have a patch trying to do that (certainly not in this
> CF). I think it makes sense to add fastbloat() to pageinspect. Maybe
> we'll get autovacuum-based solution in the future, but we don't have
> that right now.

I still think we could add this as an option to at least vacuum in this
CF. It should be far less code and gets us throttling for free. It
wouldn't be hard to add a "BLOAT ONLY" option to bypass all the other
work; it'd still be less code.

That said, I don't want to block this; I think it's useful. Though,
perhaps it would be better as an extension instead of in contrib? I
don't think it should be very version dependent?

> Actually, wouldn't that be a nice GSoC project? The scope seems about
> right, not touching too many parts of the code base, etc.

In it's simplest form I think it'd be too small, but if we got more
advanced than simply adding some counters to vacuum then I agree.

I think I'd rather gave a system for logging important stuff (like
vacuum stats) in tables with a way to prevent infinite growth though. ;)
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 25 February 2015 at 23:30, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:

> That said, I don't want to block this; I think it's useful. Though, perhaps
> it would be better as an extension instead of in contrib? I don't think it
> should be very version dependent?

The whole point of this is to get it into contrib.

It could have published it as an extension months ago.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, RemoteDBA, Training & Services

On 26/02/15 13:32, Simon Riggs wrote:
> On 25 February 2015 at 23:30, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
>
>> That said, I don't want to block this; I think it's useful. Though, perhaps
>> it would be better as an extension instead of in contrib? I don't think it
>> should be very version dependent?
>
> The whole point of this is to get it into contrib.
>
> It could have published it as an extension months ago.
>

Is this intended to replace pg_freespacemap? Not trying to be
overprotective or anything :-) but is this new extension/module
better/faster/stronger/more accurate etc? If so then excellent! Ohth was
wondering if people either a) didn't know about pg_freespacemap or b)
consider that it is crap and won't use it.

Cheers

Mark

Mark Kirkwood wrote:
> On 26/02/15 13:32, Simon Riggs wrote:
> >On 25 February 2015 at 23:30, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
> >
> >>That said, I don't want to block this; I think it's useful. Though, perhaps
> >>it would be better as an extension instead of in contrib? I don't think it
> >>should be very version dependent?
> >
> >The whole point of this is to get it into contrib.
> >
> >It could have published it as an extension months ago.
> >
>
> Is this intended to replace pg_freespacemap? Not trying to be overprotective
> or anything :-) but is this new extension/module better/faster/stronger/more
> accurate etc? If so then excellent! Ohth was wondering if people either a)
> didn't know about pg_freespacemap or b) consider that it is crap and won't
> use it.

It's a replacement for pgstattuple as far as I recall, not
pg_freespacemap. The problem with pgstattuple is that it reads every
single page of the table; this fast tool skips reading pages that are
marked all-visible in the visibility map. The disadvantage of
pg_freespacemap is that it doesn't have page data more recent than the
last vacuum, IIRC.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Dec 26, 2014 at 1:08 PM, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com>
wrote:
>
> At 2014-09-25 15:40:11 +0530, ams(at)2ndQuadrant(dot)com wrote:
> >
> > All right, then I'll post a version that addresses Amit's other
> > points, adds a new file/function to pgstattuple, acquires content
> > locks, and uses HeapTupleSatisfiesVacuum, hint-bit setting and all.
>
> Sorry, I forgot to post this patch. It does what I listed above, and
> seems to work fine (it's still quite a lot faster than pgstattuple
> in many cases).
>

I think one of the comment is not handled in latest patch.
5. Don't we need the handling for empty page (PageIsEmpty()) case?

I think we should have siimilar for PageIsEmpty()
as you have done for PageIsNew() in your patch.

+ stat.tuple_count = vac_estimate_reltuples(rel, false, nblocks, scanned,
+ stat.tuple_count);

Here for scanned tuples, you have used the live tuple counter
whereas I think we need to count HEAPTUPLE_INSERT_IN_PROGRESS
and HEAPTUPLE_DELETE_IN_PROGRESS and
HEAPTUPLE_RECENTLY_DEAD.

Refer the similar logic in Vacuum.

Although I am not in favor of using HeapTupleSatisfiesVacuum(), however
if you want to use it then lets be consistent with what Vacuum does
for estimation of tuples.

> A couple of remaining questions:
>
> 1. I didn't change the handling of LP_DEAD items, because the way it is
> now matches what pgstattuple counts. I'm open to changing it, though.
> Maybe it makes sense to count LP_DEAD items iff lp_len != 0? Or just
> leave it as it is? I think it doesn't matter much.
>
> 2. I changed the code to acquire the content locks on the buffer, as
> discussed, but it still uses HeapTupleSatisfiesVacuum. Amit suggested
> using HeapTupleSatisfiesVisibility, but it's not clear to me why that
> would be better. I welcome advice in this matter.
>

The reason to use HeapTupleSatisfiesVacuum() is that it helps us
in freezing and some other similar stuff which is required by
Vacuum. Also it could be beneficial if we want take specific
action for various result codes of Vaccum. I think as this routine
mostly gives the estimated count, so it might not matter much even
if we use HeapTupleSatisfiesVacuum(), however it is better to be
consistent with pgstat_heap() in this case.
Do you have any reason for using HeapTupleSatisfiesVacuum() rather
than what is used in pgstat_heap()?

> (If anything, I should use HeapTupleIsSurelyDead, which doesn't set
> any hint bits, but which I earlier rejected as too conservative in
> its dead/not-dead decisions for this purpose.)
>
> (I've actually patched the pgstattuple.sgml docs as well, but I want to
> re-read that to make sure it's up to date, and didn't want to wait to
> post the code changes.)
>

Sure, post the same when it is ready.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Mon, Feb 23, 2015 at 7:11 AM, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
wrote:
> On 28.1.2015 05:03, Abhijit Menon-Sen wrote:
> > At 2015-01-27 17:00:27 -0600, Jim(dot)Nasby(at)BlueTreble(dot)com wrote:
> >>
> Otherwise, the code looks OK to me. Now, there are a few features I'd
> like to have for production use (to minimize the impact):
>
> 1) no index support :-(
>
> I'd like to see support for more relation types (at least btree
> indexes). Are there any plans for that? Do we have an idea on how to
> compute that?
>
> 2) sampling just a portion of the table
>
> For example, being able to sample just 5% of blocks, making it less
> obtrusive, especially on huge tables. Interestingly, there's a
> TABLESAMPLE patch in this CF, so maybe it's possible to reuse some
> of the methods (e.g. functions behind SYSTEM sampling)?
>
> 3) throttling
>
> Another feature minimizing impact of running this on production might
> be some sort of throttling, e.g. saying 'limit the scan to 4 MB/s'
> or something along those lines.
>

I think these features could be done separately if anybody is interested.
The patch in its proposed form seems useful to me.

> 4) prefetch
>
> fbstat_heap is using visibility map to skip fully-visible pages,
> which is nice, but if we skip too many pages it breaks readahead
> similarly to bitmap heap scan. I believe this is another place where
> effective_io_concurrency (i.e. prefetch) would be appropriate.
>

Good point. We can even think of using the technique used by Vacuum
which is skip only when we can skip atleast SKIP_PAGES_THRESHOLD
pages.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Hi.

I'm just posting this WIP patch where I've renamed fastbloat to
pgstatbloat as suggested by Tomas, and added in the documentation, and
so on. I still have to incorporate Amit's comments about the estimation
of reltuples according to the way vacuum does it, and I expect to post
that tomorrow (I just need to test a little more).

In the meantime, if anyone else was having trouble installing the
extension due to the incorrect version in the control file, this is the
patch you should be using.

-- Abhijit

At 2015-03-31 22:43:49 +0530, ams(at)2ndQuadrant(dot)com wrote:
>
> I'm just posting this WIP patch where I've renamed fastbloat to
> pgstatbloat as suggested by Tomas, and added in the documentation, and
> so on.

Here's the revised version that also adds the count of RECENTLY_DEAD and
INSERT/DELETE_IN_PROGRESS tuples to the call to vac_estimate_reltuples.

Tomas, I agree that the build_output_tuple function could use cleaning
up, but it's the same as the corresponding code in pgstattuple, and it
seems to me reasonable to clean both up together in a separate patch.

-- Abhijit

On Fri, Apr 3, 2015 at 9:07 PM, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com>
wrote:
>
> At 2015-03-31 22:43:49 +0530, ams(at)2ndQuadrant(dot)com wrote:
> >
> > I'm just posting this WIP patch where I've renamed fastbloat to
> > pgstatbloat as suggested by Tomas, and added in the documentation, and
> > so on.
>
> Here's the revised version that also adds the count of RECENTLY_DEAD and
> INSERT/DELETE_IN_PROGRESS tuples to the call to vac_estimate_reltuples.
>

I think you have missed to address the below point:

>> 4) prefetch
>>
>> fbstat_heap is using visibility map to skip fully-visible pages,
>> which is nice, but if we skip too many pages it breaks readahead
>> similarly to bitmap heap scan. I believe this is another place where
>> effective_io_concurrency (i.e. prefetch) would be appropriate.
>>

> Good point. We can even think of using the technique used by Vacuum
> which is skip only when we can skip atleast SKIP_PAGES_THRESHOLD
> pages.

Apart from this, one minor point:
+typedef struct pgstatbloat_output_type
+{
+ uint64 table_len;
+ uint64 tuple_count;
+ uint64 misc_count;

misc_count sounds out of place, may be non_live_count?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

At 2015-04-18 12:28:36 +0530, amit(dot)kapila16(at)gmail(dot)com wrote:
>
> I think you have missed to address the below point:
>
> >> 4) prefetch

Updated patch attached, as well as the diff against the earlier version
just to make it easier to see the exact change I made (which is to copy
the skip logic from lazy_scan_heap, as you suggested).

I'm not entirely convinced that this change is worthwhile, but it's easy
to make and difficult to argue against, so here it is. (I did test, and
it seems to work OK after the change.)

Amit, Tomas, thanks again for your review comments.

-- Abhijit

On Wed, Apr 22, 2015 at 6:33 PM, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com>
wrote:
>
> At 2015-04-18 12:28:36 +0530, amit(dot)kapila16(at)gmail(dot)com wrote:
> >
> > I think you have missed to address the below point:
> >
> > >> 4) prefetch
>
> Updated patch attached, as well as the diff against the earlier version
> just to make it easier to see the exact change I made (which is to copy
> the skip logic from lazy_scan_heap, as you suggested).
>

Few minor issues:
1.
warning on windows

\pgstatbloat.c(193): warning C4715: 'pgstatbloat' : not all control paths
return a value

2.
Failed to install pgstattuple--1.3.sql

You have to name sql file as pgstattuple--1.3.sql rather
than pgstattuple--1.2.sql

Other than above 2 points, patch looks good to me.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

At 2015-04-24 07:22:27 +0530, amit(dot)kapila16(at)gmail(dot)com wrote:
>
> Few minor issues:
> 1.
> warning on windows
>
> \pgstatbloat.c(193): warning C4715: 'pgstatbloat' : not all control paths
> return a value

This is because the function ends with an ereport(ERROR, …). What would
you suggest here? Just stick a PG_RETURN_NULL() at the end?

> 2.
> Failed to install pgstattuple--1.3.sql
>
> You have to name sql file as pgstattuple--1.3.sql rather
> than pgstattuple--1.2.sql

How are you applying the patch? It already contains this:

diff --git a/contrib/pgstattuple/pgstattuple--1.2.sql b/contrib/pgstattuple/pgstattuple--1.3.sql
similarity index 73%
rename from contrib/pgstattuple/pgstattuple--1.2.sql
rename to contrib/pgstattuple/pgstattuple--1.3.sql
index e5fa2f5..93593ee 100644
--- a/contrib/pgstattuple/pgstattuple--1.2.sql
+++ b/contrib/pgstattuple/pgstattuple--1.3.sql

So it should do the right thing with git apply.

-- Abhijit

On Fri, Apr 24, 2015 at 8:04 AM, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com>
wrote:
>
> At 2015-04-24 07:22:27 +0530, amit(dot)kapila16(at)gmail(dot)com wrote:
> >
> > Few minor issues:
> > 1.
> > warning on windows
> >
> > \pgstatbloat.c(193): warning C4715: 'pgstatbloat' : not all control
paths
> > return a value
>
> This is because the function ends with an ereport(ERROR, …). What would
> you suggest here?

For similar code in function (pgstattuple.c->pgstat_relation()), it simply
return 0;

> Just stick a PG_RETURN_NULL() at the end?
>

That should also work.

> > 2.
> > Failed to install pgstattuple--1.3.sql
> >
> > You have to name sql file as pgstattuple--1.3.sql rather
> > than pgstattuple--1.2.sql
>
> How are you applying the patch? It already contains this:
>
> diff --git a/contrib/pgstattuple/pgstattuple--1.2.sql
b/contrib/pgstattuple/pgstattuple--1.3.sql
> similarity index 73%
> rename from contrib/pgstattuple/pgstattuple--1.2.sql
> rename to contrib/pgstattuple/pgstattuple--1.3.sql
> index e5fa2f5..93593ee 100644
> --- a/contrib/pgstattuple/pgstattuple--1.2.sql
> +++ b/contrib/pgstattuple/pgstattuple--1.3.sql
>
> So it should do the right thing with git apply.
>

I was using patch -p1 ...

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

At 2015-04-24 08:35:40 +0530, amit(dot)kapila16(at)gmail(dot)com wrote:
>
> > Just stick a PG_RETURN_NULL() at the end?
>
> That should also work.

OK, updated patch attached with just that one change.

I'm not doing anything about the rename. I don't know if it's possible
to make patch(1) rename a file at all (it wasn't, but maybe something
has changed recently?).

Note to friendly neighbourhood committers: the patch is expected to
rename pgstattuple--1.2.sql to pgstattuple--1.3.sql, which it does
if applied using git apply.

-- Abhijit

On Fri, Apr 24, 2015 at 8:46 AM, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com>
wrote:
>
> At 2015-04-24 08:35:40 +0530, amit(dot)kapila16(at)gmail(dot)com wrote:
> >
> > > Just stick a PG_RETURN_NULL() at the end?
> >
> > That should also work.
>
> OK, updated patch attached with just that one change.
>

Patch looks good to me, will mark it as Ready for Committer if Tomas
or anyone else doesn't have more to add in terms of review.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 04/24/15 14:58, Amit Kapila wrote:
> On Fri, Apr 24, 2015 at 8:46 AM, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com
> <mailto:ams(at)2ndquadrant(dot)com>> wrote:
> >
> > At 2015-04-24 08:35:40 +0530, amit(dot)kapila16(at)gmail(dot)com
> <mailto:amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > > Just stick a PG_RETURN_NULL() at the end?
> > >
> > > That should also work.
> >
> > OK, updated patch attached with just that one change.
> >
>
> Patch looks good to me, will mark it as Ready for Committer if Tomas
> or anyone else doesn't have more to add in terms of review.

FWIW, I'm OK with the patch as is. Your reviews were spot on.

regards

--
Tomas Vondra http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Apr 24, 2015 at 8:05 PM, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
wrote:
>
>
>
> On 04/24/15 14:58, Amit Kapila wrote:
>>
>> On Fri, Apr 24, 2015 at 8:46 AM, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com
>> <mailto:ams(at)2ndquadrant(dot)com>> wrote:
>> >
>> > At 2015-04-24 08:35:40 +0530, amit(dot)kapila16(at)gmail(dot)com
>> <mailto:amit(dot)kapila16(at)gmail(dot)com> wrote:
>> > >
>> > > > Just stick a PG_RETURN_NULL() at the end?
>> > >
>> > > That should also work.
>> >
>> > OK, updated patch attached with just that one change.
>> >
>>
>> Patch looks good to me, will mark it as Ready for Committer if Tomas
>> or anyone else doesn't have more to add in terms of review.
>
>
> FWIW, I'm OK with the patch as is. Your reviews were spot on.
>

Thanks, I have marked this patch as Ready For Committer.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Hi,

On 2015-04-24 08:46:48 +0530, Abhijit Menon-Sen wrote:
> diff --git a/contrib/pgstattuple/pgstatbloat.c b/contrib/pgstattuple/pgstatbloat.c
> new file mode 100644
> index 0000000..612e22b
> --- /dev/null
> +++ b/contrib/pgstattuple/pgstatbloat.c
> @@ -0,0 +1,389 @@
> +/*
> + * contrib/pgstattuple/pgstatbloat.c
> + *
> + * Abhijit Menon-Sen <ams(at)2ndQuadrant(dot)com>
> + * Portions Copyright (c) 2001,2002 Tatsuo Ishii (from pgstattuple)

I think for new extension we don't really add authors and such anymore.

> + * Permission to use, copy, modify, and distribute this software and
> + * its documentation for any purpose, without fee, and without a
> + * written agreement is hereby granted, provided that the above
> + * copyright notice and this paragraph and the following two
> + * paragraphs appear in all copies.
> + *
> + * IN NO EVENT SHALL THE AUTHOR BE LIABLE TO ANY PARTY FOR DIRECT,
> + * INDIRECT, SPECIAL, INCIDENTAL, OR CONSEQUENTIAL DAMAGES, INCLUDING
> + * LOST PROFITS, ARISING OUT OF THE USE OF THIS SOFTWARE AND ITS
> + * DOCUMENTATION, EVEN IF THE UNIVERSITY OF CALIFORNIA HAS BEEN ADVISED
> + * OF THE POSSIBILITY OF SUCH DAMAGE.
> + *
> + * THE AUTHOR SPECIFICALLY DISCLAIMS ANY WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE. THE SOFTWARE PROVIDED HEREUNDER IS ON AN "AS
> + * IS" BASIS, AND THE AUTHOR HAS NO OBLIGATIONS TO PROVIDE MAINTENANCE,
> + * SUPPORT, UPDATES, ENHANCEMENTS, OR MODIFICATIONS.
> + */

Shouldn't be here in a contrib module.

> +PG_FUNCTION_INFO_V1(pgstatbloat);

> +CREATE FUNCTION pgstatbloat(IN reloid regclass,
> + OUT table_len BIGINT, -- physical table length in bytes
> + OUT scanned_percent FLOAT8, -- what percentage of the table's pages was scanned
> + OUT approx_tuple_count BIGINT, -- estimated number of live tuples
> + OUT approx_tuple_len BIGINT, -- estimated total length in bytes of live tuples
> + OUT approx_tuple_percent FLOAT8, -- live tuples in % (based on estimate)
> + OUT dead_tuple_count BIGINT, -- exact number of dead tuples
> + OUT dead_tuple_len BIGINT, -- exact total length in bytes of dead tuples
> + OUT dead_tuple_percent FLOAT8, -- dead tuples in % (based on estimate)
> + OUT free_space BIGINT, -- exact free space in bytes
> + OUT free_percent FLOAT8) -- free space in %

Hm. I do wonder if this should really be called 'statbloat'. Perhaps
it'd more appropriately be called pg_estimate_bloat or somesuch?

Also, is free_space really exact? The fsm isn't byte exact.

> +static Datum
> +build_output_type(pgstatbloat_output_type *stat, FunctionCallInfo fcinfo)
> +{
> +#define NCOLUMNS 10
> +#define NCHARS 32
> +
> + HeapTuple tuple;
> + char *values[NCOLUMNS];
> + char values_buf[NCOLUMNS][NCHARS];
> + int i;
> + double tuple_percent;
> + double dead_tuple_percent;
> + double free_percent; /* free/reusable space in % */
> + double scanned_percent;
> + TupleDesc tupdesc;
> + AttInMetadata *attinmeta;
> +
> + /* Build a tuple descriptor for our result type */
> + if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
> + elog(ERROR, "return type must be a row type");
> +
> + /*
> + * Generate attribute metadata needed later to produce tuples from raw C
> + * strings
> + */
> + attinmeta = TupleDescGetAttInMetadata(tupdesc);
> +
> + if (stat->table_len == 0)
> + {
> + tuple_percent = 0.0;
> + dead_tuple_percent = 0.0;
> + free_percent = 0.0;
> + }
> + else
> + {
> + tuple_percent = 100.0 * stat->tuple_len / stat->table_len;
> + dead_tuple_percent = 100.0 * stat->dead_tuple_len / stat->table_len;
> + free_percent = 100.0 * stat->free_space / stat->table_len;
> + }
> +
> + scanned_percent = 0.0;
> + if (stat->total_pages != 0)
> + {
> + scanned_percent = 100 * stat->scanned_pages / stat->total_pages;
> + }
> +
> + for (i = 0; i < NCOLUMNS; i++)
> + values[i] = values_buf[i];
> + i = 0;
> + snprintf(values[i++], NCHARS, INT64_FORMAT, stat->table_len);
> + snprintf(values[i++], NCHARS, "%.2f", scanned_percent);
> + snprintf(values[i++], NCHARS, INT64_FORMAT, stat->tuple_count);
> + snprintf(values[i++], NCHARS, INT64_FORMAT, stat->tuple_len);
> + snprintf(values[i++], NCHARS, "%.2f", tuple_percent);
> + snprintf(values[i++], NCHARS, INT64_FORMAT, stat->dead_tuple_count);
> + snprintf(values[i++], NCHARS, INT64_FORMAT, stat->dead_tuple_len);
> + snprintf(values[i++], NCHARS, "%.2f", dead_tuple_percent);
> + snprintf(values[i++], NCHARS, INT64_FORMAT, stat->free_space);
> + snprintf(values[i++], NCHARS, "%.2f", free_percent);
> +
> + tuple = BuildTupleFromCStrings(attinmeta, values);
> +
> + return HeapTupleGetDatum(tuple);
> +}

Why go through C strings? I personally think we really shouldn't add
more callers to BuildTupleFromCStrings, it's such an absurd interface.

> + switch (rel->rd_rel->relkind)
> + {
> + case RELKIND_RELATION:
> + case RELKIND_MATVIEW:
> + PG_RETURN_DATUM(pgstatbloat_heap(rel, fcinfo));
> + case RELKIND_TOASTVALUE:
> + err = "toast value";
> + break;
> + case RELKIND_SEQUENCE:
> + err = "sequence";
> + break;
> + case RELKIND_INDEX:
> + err = "index";
> + break;
> + case RELKIND_VIEW:
> + err = "view";
> + break;
> + case RELKIND_COMPOSITE_TYPE:
> + err = "composite type";
> + break;
> + case RELKIND_FOREIGN_TABLE:
> + err = "foreign table";
> + break;
> + default:
> + err = "unknown";
> + break;
> + }
>

This breaks translateability. I'm not sure that's worthy of concern. I
think it'd actually be fine to just say that the relation has to be a
table or matview.

> +static Datum
> +pgstatbloat_heap(Relation rel, FunctionCallInfo fcinfo)
> +{
> + /*
> + * We use the visibility map to skip over SKIP_PAGES_THRESHOLD or
> + * more contiguous all-visible pages. See the explanation in
> + * lazy_scan_heap for the rationale.
> + */

I don't believe that rationale is really true these days. I'd much
rather just rip this out here than copy the rather complex logic.

> + for (blkno = 0; blkno < nblocks; blkno++)
> + {

> + stat.free_space += PageGetHeapFreeSpace(page);
> +
> + if (PageIsNew(page) || PageIsEmpty(page))
> + {
> + UnlockReleaseBuffer(buf);
> + continue;
> + }

I haven't checked, but I'm not sure that it's safe/meaningful to call
PageGetHeapFreeSpace() on a new page.

Greetings,

Andres Freund

At 2015-05-09 02:20:51 +0200, andres(at)anarazel(dot)de wrote:
>
> > + * Abhijit Menon-Sen <ams(at)2ndQuadrant(dot)com>
> > + * Portions Copyright (c) 2001,2002 Tatsuo Ishii (from pgstattuple)
>
> I think for new extension we don't really add authors and such anymore.

OK, I'll get rid of the boilerplate.

> Hm. I do wonder if this should really be called 'statbloat'. Perhaps
> it'd more appropriately be called pg_estimate_bloat or somesuch?

Since we've moved it into pgstattuple, perhaps pgstattuple_approximate()
or pgstattuple_estimated() would be a better name. I don't really care,
I'll change it to whatever people like.

> Also, is free_space really exact? The fsm isn't byte exact.

You're right, I'll fix that.

> Why go through C strings? I personally think we really shouldn't add
> more callers to BuildTupleFromCStrings, it's such an absurd interface.

I just copied this more or less blindly from pgstattuple. I'll fix it
and submit a separate patch to fix the equivalent code in pgstattuple.

> I think it'd actually be fine to just say that the relation has to be
> a table or matview.

Good point. Agreed.

> I don't believe that rationale is really true these days. I'd much
> rather just rip this out here than copy the rather complex logic.

Yes, thanks, I very much agree that this isn't really worth copying.
I'll drop it in my next submission.

> 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.

Thanks for the feedback. I'll submit a revised patch shortly.

-- Abhijit

Hi Andres.

I've attached an updated patch for pgstatbloat, as well as a patch to
replace two uses of BuildTupleFromCStrings() elsewhere in pgstattuple.

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.

Also:

At 2015-05-09 15:36:49 +0530, ams(at)2ndQuadrant(dot)com wrote:
>
> At 2015-05-09 02:20:51 +0200, andres(at)anarazel(dot)de wrote:
> >
> > 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?

-- Abhijit

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

At 2015-05-11 19:15:47 +0200, andres(at)anarazel(dot)de wrote:
>
> TBH, I'd rather not touch unrelated things right now. We're pretty
> badly behind...

OK. That patch is independent; just ignore it.

> I don't really care how it's named, as long as it makes clear that
> it's not an exact measurement.

Not having heard any better suggestions, I picked "pgstatapprox" as a
compromise between length and familiarity/consistency with pgstattuple.

> > Should I count the space it would have free if it were initialised,
> > but leave the page alone for VACUUM to deal with?

And this is what the attached patch does.

I also cleaned up a few things that I didn't like but had left alone to
make the code look similar to pgstattuple. In particular, build_tuple()
now does nothing but build a tuple from values calculated earlier in
pgstatapprox_heap().

Thank you.

-- Abhijit

P.S. What, if anything, should be done about the complicated and likely
not very useful skip-only-min#-blocks logic in lazy_scan_heap?

On 2015-05-12 00:42:14 +0530, Abhijit Menon-Sen wrote:
> At 2015-05-11 19:15:47 +0200, andres(at)anarazel(dot)de wrote:
> > I don't really care how it's named, as long as it makes clear that
> > it's not an exact measurement.
>
> Not having heard any better suggestions, I picked "pgstatapprox" as a
> compromise between length and familiarity/consistency with pgstattuple.

I can live with that, although I'd personally go with
pgstattuple_approx()...

Other than that and some minor local changes I've made, I'm ready to
commit this.

Greetings,

Andres Freund

At 2015-05-13 03:04:11 +0200, andres(at)anarazel(dot)de wrote:
>
> I can live with that, although I'd personally go with
> pgstattuple_approx()...

I could live with that too. Here's an incremental patch to rename the
function. (I'm not resubmitting the whole thing since you said you've
made some other changes too.)

Thank you.

-- Abhijit