Re: BUG #18247: Integer overflow leads to negative width

On Fri, Dec 15, 2023 at 11:43 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Richard Guo <guofenglinux(at)gmail(dot)com> writes:
> > On Thu, Dec 14, 2023 at 10:43 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> Probably better to clamp tuple width estimates to MaxAllocSize.
> >> Anything larger would not correspond to reality anyhow.
>
> > Fair point. How about the attached patch?
>
> We'd need to hit at least build_joinrel_tlist too.

Right. And I think we also need to hit add_placeholders_to_joinrel.

> Not sure
> offhand whether this is enough to cover upper-relation tlists.

I think we need to do the same to create_one_window_path. For
intermediate WindowAggPaths, we need to add the WindowFuncs to their
output targets, and that would increase the width of the tlists.

Also, it is possible that there could be tuple-width overflow occurring
within function get_rel_data_width(). The return value of this function
is used to calculate density, or returned by get_relation_data_width().
So I wonder if we could change the return type of get_rel_data_width()
and get_relation_data_width() to be double, and handle the overflow in
the callers if needed. But this may lead to ABI break.

> As far as the specifics go, is it enough to clamp once? I think
> we'd either have to clamp after each addition, or change the
> running-sum variables to double and clamp just before storing
> into the final width field. The latter seems probably
> less error-prone in the long run.

Agreed.

> Also, given that we'll need at least three copies of the clamp
> rule, I wonder if it's worth inventing a function comparable
> to clamp_row_est().

Yeah, I think we should do that.

Attached is an updated patch for all the changes. It also changes the
loop_count parameter to be double for compute_bitmap_pages() in passing.
It does not improve the comments for compute_bitmap_pages() though.

Thanks
Richard