> 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