tableam: abstracting relation sizing code

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

On 6 Jun 2019, at 22:40, Robert Haas <robertmhaas@gmail.com> 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

On Thu, Jun 6, 2019 at 10:08 PM Michael Paquier <michael@paquier.xyz> 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

On Fri, Jun 7, 2019 at 4:11 AM Daniel Gustafsson <daniel@yesql.se> 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

On Fri, Jun 7, 2019 at 8:43 AM Robert Haas <robertmhaas@gmail.com> 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

Hi,

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

On Thu, Jun 6, 2019 at 10:08 PM Michael Paquier <michael@paquier.xyz> 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

On 2019-Jun-07, Robert Haas wrote:

On Fri, Jun 7, 2019 at 8:43 AM Robert Haas <robertmhaas@gmail.com> 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

On 7 Jun 2019, at 14:43, Robert Haas <robertmhaas@gmail.com> 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

On Fri, Jun 7, 2019 at 6:42 PM Daniel Gustafsson <daniel@yesql.se> 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

On 2019-Jun-10, Robert Haas wrote:

On Fri, Jun 7, 2019 at 6:42 PM Daniel Gustafsson <daniel@yesql.se> 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

On Mon, Jun 10, 2019 at 3:46 PM Alvaro Herrera <alvherre@2ndquadrant.com> 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

On 11 Jun 2019, at 15:17, Robert Haas <robertmhaas@gmail.com> 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

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

On 10 Jun 2019, at 21:35, Robert Haas <robertmhaas@gmail.com> wrote:

On Fri, Jun 7, 2019 at 6:42 PM Daniel Gustafsson <daniel@yesql.se> 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

Hi,

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

On Fri, Jun 7, 2019 at 6:42 PM Daniel Gustafsson <daniel@yesql.se> 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

On Tue, Jun 11, 2019 at 7:17 PM Andres Freund <andres@anarazel.de> 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

On Wed, Jun 12, 2019 at 9:14 AM Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Jun 11, 2019 at 7:17 PM Andres Freund <andres@anarazel.de> 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.

Committed.

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