Em seg., 5 de set. de 2022 às 07:15, David Rowley <dgrowleyml@gmail.com> escreveu:
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. */
The second comment in the v3 patch, must be:
/* * Is the value greater than the maxval? If yes, we'll recurse * to the right side of the range array. */
I think this is copy-and-paste thinko with the word "minval".
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 think this kind recursion, because the loop is restarted
with:
start = (midpoint + 1); continue;
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.
