On 14.06.23 20:41, Corey Huinker wrote:
> So maybe we should make table_block_relation_estimate_size smarter to
> also consider the fillfactor in the "no statistics" branch, per the
> attached patch.
>
>
> I like this a lot. The reasoning is obvious, the fix is simple,it
> doesn't upset any make-check-world tests, and in order to get a
> performance regression we'd need a table whose fillfactor has been
> changed after the data was loaded but before an analyze happens, and
> that's a narrow enough case to accept.
>
> My only nitpick is to swap
>
> (usable_bytes_per_page * fillfactor / 100) / tuple_width
>
> with
>
> (usable_bytes_per_page * fillfactor) / (tuple_width * 100)
>
>
> as this will eliminate the extra remainder truncation, and it also gets
> the arguments "in order" algebraically.
The fillfactor is in percent, so it makes sense to divide it by 100
first before doing any further arithmetic with it. I find your version
of this to be more puzzling without any explanation. You could do
fillfactor/100.0 to avoid the integer division, but then, the comment
above that says "integer division is intentional here", without any
further explanation. I think a bit more explanation of all the
subtleties here would be good in any case.