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

On Mon, Dec 18, 2023 at 4:44 PM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
>
>
> 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.
>
I guess using double for the sum is in case of overflow of int64?
pg_class.relnatts
is of type int16, I guess it's not possible to overflow int64.

Using *int64* for the tuple_width is more intuitive, the
clamp_width_est will be:

+int
+clamp_width_est(int64 tuple_width)
+{
+ /*
+ * Clamp tuple-width estimate to MaxAllocSize in case it exceeds the limit
+ * or overflows. Anything larger than MaxAllocSize would not align with
+ * reality.
+ */
+ if (tuple_width > MaxAllocSize)
+ tuple_width = MaxAllocSize;
+
+ return (int) tuple_width;
+}

> Thanks
> Richard

--
Regards
Junwang Zhao