On Sat, 3 Sept 2022 at 00:37, Ranier Vilela <ranier.vf@gmail.com> wrote:
>> But +1 to fix this and other issues even if they would never crash.
Yeah, I don't think any of this coding would lead to a crash, but it's
pretty weird coding and we should fix it.
> 1. Once that ranges->nranges is invariant, avoid the loop if ranges->nranges <= 0.
> This matches the current behavior.
>
> 2. Once that ranges->nsorted is invariant, avoid the loop if ranges->nsorted <= 0.
> This matches the current behavior.
>
> 3. Remove the invariant cxt from ranges->nsorted loop.
>
> 4. Avoid possible overflows when using int to store length strings.
>
> 5. Avoid possible out-of-bounds when ranges->nranges == 0.
>
> 6. Avoid overhead when using unnecessary StringInfoData to convert Datum a to Text b.
I've ripped out #4 and #6 for now. I think we should do #6 in master
only, probably as part of a wider cleanup of StringInfo misusages.
I also spent some time trying to ensure I understand this code
correctly. I was unable to work out what the nsorted field was from
just the comments. I needed to look at the code to figure it out, so I
think the comments for that field need to be improved. A few of the
others were not that clear either. I hope I've improved them in the
attached.
I was also a bit confused at various other comments. e.g:
/*
* Is the value greater than the maxval? If yes, we'll recurse to the
* right side of range array.
*/
I don't see any sort of recursion going on here. All I see are
skipping of values that are out of bounds of the lower bound of the
lowest range, and above the upper bound of the highest range.
I propose to backpatch the attached into v14 tomorrow morning (about
12 hours from now).
The only other weird coding I found was in brin_range_deserialize:
for (i = 0; (i < nvalues) && (!typbyval); i++)
I imagine most compilers would optimize that so that the typbyval
check is done before the first loop and not done on every loop, but I
don't think that coding practice helps the human readers out much. I
left that one alone, for now.
David
