tableam: abstracting relation sizing code

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

tableam: abstracting relation sizing code

Robert Haas
Hi,

It looks to me as though any table AM that uses the relation forks
supported by PostgreSQL in a more or less normal manner is likely to
require an implementation of the relation_size callback that is
identical to the one for heap, and an implementation of the
estimate_rel_size method that is extremely similar to the one for
heap.  The latter is especially troubling as the amount of code
duplication is non-trivial, and it's full of special hacks.

Here is a patch that tries to improve the situation.  I don't know
whether there is some better approach; this seemed like the obvious
thing to do.

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

0001-tableam-Provide-helper-functions-for-relation-sizing.patch (19K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: tableam: abstracting relation sizing code

Michael Paquier-2
On Thu, Jun 06, 2019 at 04:40:53PM -0400, Robert Haas wrote:

> It looks to me as though any table AM that uses the relation forks
> supported by PostgreSQL in a more or less normal manner is likely to
> require an implementation of the relation_size callback that is
> identical to the one for heap, and an implementation of the
> estimate_rel_size method that is extremely similar to the one for
> heap.  The latter is especially troubling as the amount of code
> duplication is non-trivial, and it's full of special hacks.
>
> Here is a patch that tries to improve the situation.  I don't know
> whether there is some better approach; this seemed like the obvious
> thing to do.
Looks like a neat split.

"allvisfrac", "pages" and "tuples" had better be documented about
which result they represent.

+ * usable_bytes_per_page should contain the approximate number of bytes per
+ * page usable for tuple data, excluding the page header and any anticipated
+ * special space.
[...]
+table_block_estimate_rel_size(Relation rel, int32 *attr_widths,
+                             BlockNumber *pages, double *tuples,
+                             double *allvisfrac,
+                             Size overhead_bytes_per_tuple,
+                             Size usable_bytes_per_page)

Could you explain what's the use cases you have in mind for
usable_bytes_per_page?  All AMs using smgr need to have a page header,
which implies that the usable number of bytes is (BLCKSZ - page
header) like heap for tuple data.  In the AMs you got to work with, do
you store some extra data in the page which is not used for tuple
storage?  I think that makes sense, just wondering about the use
case.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: tableam: abstracting relation sizing code

Daniel Gustafsson
In reply to this post by Robert Haas
> On 6 Jun 2019, at 22:40, Robert Haas <[hidden email]> wrote:

> It looks to me as though any table AM that uses the relation forks
> supported by PostgreSQL in a more or less normal manner is likely to
> require an implementation of the relation_size callback that is
> identical to the one for heap, and an implementation of the
> estimate_rel_size method that is extremely similar to the one for
> heap.  The latter is especially troubling as the amount of code
> duplication is non-trivial, and it's full of special hacks.

Makes sense. Regarding one of the hacks:

+ * HACK: if the relation has never yet been vacuumed, use a minimum size
+ * estimate of 10 pages.  The idea here is to avoid assuming a
+ * newly-created table is really small, even if it currently is, because
+ * that may not be true once some data gets loaded into it.

When this is a generic function for AM’s, would it make sense to make the
minimum estimate a passed in value rather than hardcoded at 10?  I don’t have a
case in mind, but I also don’t think that assumptions made for heap necessarily
makes sense for all AM’s. Just thinking out loud.

> Here is a patch that tries to improve the situation.  I don't know
> whether there is some better approach; this seemed like the obvious
> thing to do.

A small nitpick on the patch:

+ * estimate_rel_size callback, because it has a few additional paramters.

s/paramters/parameters/

cheers ./daniel

Reply | Threaded
Open this post in threaded view
|

Re: tableam: abstracting relation sizing code

Robert Haas
In reply to this post by Michael Paquier-2
On Thu, Jun 6, 2019 at 10:08 PM Michael Paquier <[hidden email]> wrote:
> Looks like a neat split.

Thanks.

> "allvisfrac", "pages" and "tuples" had better be documented about
> which result they represent.

A lot of the table AM stuff (and the related slot stuff) lacks
function header comments; I don't like that and think it should be
improved. However, that's not the job of this patch. I think it's
completely correct for this patch to document, as it does, that the
arguments have the same meaning as for the estimate_rel_size method,
and leave it at that. There is certainly negative value in duplicating
the definitions in multiple places, where they might get out of sync.
The header comment for table_relation_estimate_size() refers the
reader to the comments for estimate_rel_size(), which says:

 * estimate_rel_size - estimate # pages and # tuples in a table or index
 *
 * We also estimate the fraction of the pages that are marked all-visible in
 * the visibility map, for use in estimation of index-only scans.
 *
 * If attr_widths isn't NULL, it points to the zero-index entry of the
 * relation's attr_widths[] cache; we fill this in if we have need to compute
 * the attribute widths for estimation purposes.

...which AFAICT constitutes as much documentation of these parameters
as we have got.  I think this is all a bit byzantine and could
probably be made clearer, but (1) fortunately it's not all that hard
to guess what these are supposed to mean and (2) I don't feel obliged
to do semi-related comment cleanup every time I patch tableam.

> Could you explain what's the use cases you have in mind for
> usable_bytes_per_page?  All AMs using smgr need to have a page header,
> which implies that the usable number of bytes is (BLCKSZ - page
> header) like heap for tuple data.  In the AMs you got to work with, do
> you store some extra data in the page which is not used for tuple
> storage?  I think that makes sense, just wondering about the use
> case.

Exactly.  BLCKSZ - page header is probably the maximum unless you roll
your own page format, but you could easily have less if you use the
special space.  zheap is storing transaction slots there; you could
store an epoch to avoid freezing, and there's probably quite a few
other reasonable possibilities.

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


Reply | Threaded
Open this post in threaded view
|

Re: tableam: abstracting relation sizing code

Robert Haas
In reply to this post by Daniel Gustafsson
On Fri, Jun 7, 2019 at 4:11 AM Daniel Gustafsson <[hidden email]> wrote:

> Makes sense. Regarding one of the hacks:
>
> +        * HACK: if the relation has never yet been vacuumed, use a minimum size
> +        * estimate of 10 pages.  The idea here is to avoid assuming a
> +        * newly-created table is really small, even if it currently is, because
> +        * that may not be true once some data gets loaded into it.
>
> When this is a generic function for AM’s, would it make sense to make the
> minimum estimate a passed in value rather than hardcoded at 10?  I don’t have a
> case in mind, but I also don’t think that assumptions made for heap necessarily
> makes sense for all AM’s. Just thinking out loud.
I think that's probably going in the wrong direction.  It's arguable,
of course.  However, it seems to me that there's nothing heap-specific
about the number 10.  It's not computed based on the format of a heap
page or a heap tuple.  It's just somebody's guess (likely Tom's) about
how to plan with empty relations.  If somebody finds that another
number works better for their AM, it's probably also better for heap,
at least on that person's workload.  It seems more likely to me that
this needs to be a GUC or reloption than that it needs to be an
AM-specific property.  It also seems to me that if the parameters of
the hack randomly vary from one AM to another, it's likely to create
confusing plan differences that have nothing to do with the actual
merits of what the AMs are doing.  But all that being said, I'm not
blocking anybody from fooling around with this; nobody's obliged to
use the helper function.  It's just there for people who want the same
AM-independent logic that the heap uses.

> > Here is a patch that tries to improve the situation.  I don't know
> > whether there is some better approach; this seemed like the obvious
> > thing to do.
>
> A small nitpick on the patch:
>
> + * estimate_rel_size callback, because it has a few additional paramters.
>
> s/paramters/parameters/

Good catch, and now I notice that the callback is not called
estimate_rel_size but relation_estimate_size.  Updated patch attached;
thanks for the review.

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

v2-0001-tableam-Provide-helper-functions-for-relation-siz.patch (19K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: tableam: abstracting relation sizing code

Robert Haas
On Fri, Jun 7, 2019 at 8:43 AM Robert Haas <[hidden email]> wrote:
> Good catch, and now I notice that the callback is not called
> estimate_rel_size but relation_estimate_size.  Updated patch attached;
> thanks for the review.

Let's try that one more time, and this time perhaps I'll make it compile.

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

v3-0001-tableam-Provide-helper-functions-for-relation-siz.patch (19K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: tableam: abstracting relation sizing code

Andres Freund
In reply to this post by Robert Haas
Hi,

On 2019-06-07 08:32:37 -0400, Robert Haas wrote:

> On Thu, Jun 6, 2019 at 10:08 PM Michael Paquier <[hidden email]> wrote:
> > "allvisfrac", "pages" and "tuples" had better be documented about
> > which result they represent.
>
> A lot of the table AM stuff (and the related slot stuff) lacks
> function header comments; I don't like that and think it should be
> improved. However, that's not the job of this patch. I think it's
> completely correct for this patch to document, as it does, that the
> arguments have the same meaning as for the estimate_rel_size method,
> and leave it at that. There is certainly negative value in duplicating
> the definitions in multiple places, where they might get out of sync.
> The header comment for table_relation_estimate_size() refers the
> reader to the comments for estimate_rel_size(), which says:

Note that these function ended up that way by precisely this logic... ;)

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: tableam: abstracting relation sizing code

Alvaro Herrera-9
In reply to this post by Robert Haas
On 2019-Jun-07, Robert Haas wrote:

> On Fri, Jun 7, 2019 at 8:43 AM Robert Haas <[hidden email]> wrote:
> > Good catch, and now I notice that the callback is not called
> > estimate_rel_size but relation_estimate_size.  Updated patch attached;
> > thanks for the review.
>
> Let's try that one more time, and this time perhaps I'll make it compile.

It looks good to me, passes tests.  This version seems to introduce a warning
in my build:

/pgsql/source/master/src/backend/access/table/tableam.c: In function 'table_block_relation_estimate_size':
/pgsql/source/master/src/backend/access/table/tableam.c:633:12: warning: implicit declaration of function 'rint' [-Wimplicit-function-declaration]
  *tuples = rint(density * (double) curpages);
            ^~~~
/pgsql/source/master/src/backend/access/table/tableam.c:633:12: warning: incompatible implicit declaration of built-in function 'rint'
/pgsql/source/master/src/backend/access/table/tableam.c:633:12: note: include '<math.h>' or provide a declaration of 'rint'

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


Reply | Threaded
Open this post in threaded view
|

Re: tableam: abstracting relation sizing code

Daniel Gustafsson
In reply to this post by Robert Haas
> On 7 Jun 2019, at 14:43, Robert Haas <[hidden email]> wrote:

> I think that's probably going in the wrong direction.  It's arguable,
> of course.  However, it seems to me that there's nothing heap-specific
> about the number 10.  It's not computed based on the format of a heap
> page or a heap tuple.  It's just somebody's guess (likely Tom's) about
> how to plan with empty relations.  If somebody finds that another
> number works better for their AM, it's probably also better for heap,
> at least on that person's workload.  

Fair enough, that makes sense.

> Good catch, and now I notice that the callback is not called
> estimate_rel_size but relation_estimate_size.  Updated patch attached;
> thanks for the review.

The commit message still refers to it as estimate_rel_size though. The comment on
table_block_relation_estimate_size does too but that one might be intentional.

The v3 patch applies cleanly and passes tests (I did not see the warning that
Alvaro mentioned, so yay for testing on multiple compilers).

During re-review, the below part stood out as a bit odd however:

+ if (curpages < 10 &&
+ relpages == 0 &&
+ !rel->rd_rel->relhassubclass)
+ curpages = 10;
+
+ /* report estimated # pages */
+ *pages = curpages;
+ /* quick exit if rel is clearly empty */
+ if (curpages == 0)
+ {
+ *tuples = 0;
+ *allvisfrac = 0;
+ return;
+ }

While I know this codepath isn’t introduced by this patch (it was introduced in
696d78469f3), I hadn’t seen it before so sorry for thread-jacking slightly.

Maybe I’m a bit thick but if the rel is totally empty and without children,
then curpages as well as relpages would be both zero.  But if so, how can we
enter the second "quick exit” block since curpages by then will be increased to
10 in the block just before for the empty case?  It seems to me that the blocks
should be the other way around to really have a fast path, but I might be
missing something.

cheers ./daniel

Reply | Threaded
Open this post in threaded view
|

Re: tableam: abstracting relation sizing code

Robert Haas
On Fri, Jun 7, 2019 at 6:42 PM Daniel Gustafsson <[hidden email]> wrote:
> > Good catch, and now I notice that the callback is not called
> > estimate_rel_size but relation_estimate_size.  Updated patch attached;
> > thanks for the review.
>
> The commit message still refers to it as estimate_rel_size though. The comment on
> table_block_relation_estimate_size does too but that one might be intentional.

Oops.  New version attached, hopefully fixing those and the compiler
warning Alvaro noted.

> During re-review, the below part stood out as a bit odd however:
>
> +       if (curpages < 10 &&
> +               relpages == 0 &&
> +               !rel->rd_rel->relhassubclass)
> +               curpages = 10;
> +
> +       /* report estimated # pages */
> +       *pages = curpages;
> +       /* quick exit if rel is clearly empty */
> +       if (curpages == 0)
> +       {
> +               *tuples = 0;
> +               *allvisfrac = 0;
> +               return;
> +       }
>
> While I know this codepath isn’t introduced by this patch (it was introduced in
> 696d78469f3), I hadn’t seen it before so sorry for thread-jacking slightly.
>
> Maybe I’m a bit thick but if the rel is totally empty and without children,
> then curpages as well as relpages would be both zero.  But if so, how can we
> enter the second "quick exit” block since curpages by then will be increased to
> 10 in the block just before for the empty case?  It seems to me that the blocks
> should be the other way around to really have a fast path, but I might be
> missing something.
Well, as you say, I'm just moving the code.  However, note that
curpages is the size of the relation RIGHT NOW whereas relpages is the
size the last time the relation was analyzed.  So I guess the case
you're wondering about would happen if the relation were analyzed and
then truncated.  It seems there are lots of things that could be done
here in the hopes of improving things, like keeping track in pg_class
of whether analyze has ever happened rather than using relpages == 0
as a bad approximation, but I'd rather not drift further OT, so if
you're in the mood to talk about that stuff, I would appreciate it if
you could start a new thread.

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

v3-0001-tableam-Provide-helper-functions-for-relation-siz.patch (20K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: tableam: abstracting relation sizing code

Alvaro Herrera-9
On 2019-Jun-10, Robert Haas wrote:

> On Fri, Jun 7, 2019 at 6:42 PM Daniel Gustafsson <[hidden email]> wrote:

> > The commit message still refers to it as estimate_rel_size though. The comment on
> > table_block_relation_estimate_size does too but that one might be intentional.
>
> Oops.  New version attached, hopefully fixing those and the compiler
> warning Alvaro noted.

It does fix the warning, thanks.

> > Maybe I’m a bit thick but if the rel is totally empty and without children,
> > then curpages as well as relpages would be both zero.  But if so, how can we
> > enter the second "quick exit” block since curpages by then will be increased to
> > 10 in the block just before for the empty case?  It seems to me that the blocks
> > should be the other way around to really have a fast path, but I might be
> > missing something.
>
> Well, as you say, I'm just moving the code.

I agree that you're just moving the code, but this seems to have been
recently broken in 696d78469f37 -- it was correct before that (the
heuristic for never vacuumed rels was in optimizer/plancat.c).  So in
reality the problem that Daniel pointed out is an open item for pg12.

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


Reply | Threaded
Open this post in threaded view
|

Re: tableam: abstracting relation sizing code

Robert Haas
On Mon, Jun 10, 2019 at 3:46 PM Alvaro Herrera <[hidden email]> wrote:
> I agree that you're just moving the code, but this seems to have been
> recently broken in 696d78469f37 -- it was correct before that (the
> heuristic for never vacuumed rels was in optimizer/plancat.c).  So in
> reality the problem that Daniel pointed out is an open item for pg12.

I took a look at this but I don't see that Andres did anything in that
commit other than move code.  In the new code,
heapam_estimate_rel_size() does this:

+    if (curpages < 10 &&
+        relpages == 0 &&
+        !rel->rd_rel->relhassubclass)
+        curpages = 10;
+
+    /* report estimated # pages */
+    *pages = curpages;
+    /* quick exit if rel is clearly empty */
+    if (curpages == 0)
+    {
+        *tuples = 0;
+        *allvisfrac = 0;
+        return;
+    }

And here's what the code in estimate_rel_size looked like before the
commit you mention:

             if (curpages < 10 &&
                 rel->rd_rel->relpages == 0 &&
                 !rel->rd_rel->relhassubclass &&
                 rel->rd_rel->relkind != RELKIND_INDEX)
                 curpages = 10;

             /* report estimated # pages */
             *pages = curpages;
             /* quick exit if rel is clearly empty */
             if (curpages == 0)
             {
                 *tuples = 0;
                 *allvisfrac = 0;
                 break;
             }

It's all the same, except that now that the test is in heap-specific
code it no longer needs to test for RELKIND_INDEX.

I may be missing something here, but I don't know what it is.

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


Reply | Threaded
Open this post in threaded view
|

Re: tableam: abstracting relation sizing code

Daniel Gustafsson
> On 11 Jun 2019, at 15:17, Robert Haas <[hidden email]> wrote:

> I may be missing something here, but I don't know what it is.

After looking at it closer yesterday I think my original question came from a
misunderstanding of the codepath, so I too don’t think there is an issue here
(unless I’m also missing something).

cheers ./daniel

Reply | Threaded
Open this post in threaded view
|

Re: tableam: abstracting relation sizing code

Alvaro Herrera-9
In reply to this post by Robert Haas
On 2019-Jun-11, Robert Haas wrote:

> I may be missing something here, but I don't know what it is.

Huh, you're right, I misread the diff.  Thanks for double-checking.

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


Reply | Threaded
Open this post in threaded view
|

Re: tableam: abstracting relation sizing code

Daniel Gustafsson
In reply to this post by Robert Haas
> On 10 Jun 2019, at 21:35, Robert Haas <[hidden email]> wrote:
>
> On Fri, Jun 7, 2019 at 6:42 PM Daniel Gustafsson <[hidden email]> wrote:
>>> Good catch, and now I notice that the callback is not called
>>> estimate_rel_size but relation_estimate_size.  Updated patch attached;
>>> thanks for the review.
>>
>> The commit message still refers to it as estimate_rel_size though. The comment on
>> table_block_relation_estimate_size does too but that one might be intentional.
>
> Oops.  New version attached, hopefully fixing those and the compiler
> warning Alvaro noted.

+1 on this version of the patch, no warning, passes tests and looks good.

cheers ./daniel

Reply | Threaded
Open this post in threaded view
|

Re: tableam: abstracting relation sizing code

Andres Freund
In reply to this post by Robert Haas
Hi,

On 2019-06-10 15:35:18 -0400, Robert Haas wrote:

> On Fri, Jun 7, 2019 at 6:42 PM Daniel Gustafsson <[hidden email]> wrote:
> > > Good catch, and now I notice that the callback is not called
> > > estimate_rel_size but relation_estimate_size.  Updated patch attached;
> > > thanks for the review.
> >
> > The commit message still refers to it as estimate_rel_size though. The comment on
> > table_block_relation_estimate_size does too but that one might be intentional.
>
> Oops.  New version attached, hopefully fixing those and the compiler
> warning Alvaro noted.

Just to understand: What version are you targeting? It seems pretty
clearly v13 material to me?

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: tableam: abstracting relation sizing code

Robert Haas
On Tue, Jun 11, 2019 at 7:17 PM Andres Freund <[hidden email]> wrote:
> Just to understand: What version are you targeting? It seems pretty
> clearly v13 material to me?

My current plan is to commit this to v13 as soon as the tree opens.

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