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