Re: [BUG] false positive in bt_index_check in case of short 4B varlena datum

On Wed, Jan 24, 2024 at 3:24 AM Michael Zhilin <m(dot)zhilin(at)postgrespro(dot)ru> wrote:
>
> Hi,
>
> Thank you, Jian, for nice comments!
> PFA version with your recommendations.
>
> Andrey,
> I didn't yet check your patches, but at least compiler complains about added, but unused variable "miss_oversized_tuple".
>
> verify_nbtree.c:2898:7: warning: unused variable 'miss_oversized_tuple' [-Wunused-variable]
> bool miss_oversized_tuple = false;
>
> So patch has been updated to fix this warning.
>
> Attached v4, rebased version with Jian's comments & removed unused variable.
>

this deserve some comments, given the whole C file, a large portion of
is comments
+ data_size = MAXALIGN(heap_compute_data_size(tupleDescriptor,
+ normalized, isnull)
+ + MAXALIGN(sizeof(IndexTupleData) + sizeof(IndexAttributeBitMapData)));
+ if ((data_size & INDEX_SIZE_MASK) != data_size)
+ {
+ return NULL;
+ }

whitespace error.
git apply $PATCHES/v4-0003-amcheck-avoid-failing-on-oversized-tuples.patch
/home/jian/Downloads/patches/v4-0003-amcheck-avoid-failing-on-oversized-tuples.patch:147:
trailing whitespace.
*
warning: 1 line adds whitespace errors.

Is this part unnecessary?
+-- directory paths are passed to us in environment variables

I'm not native English speaker, but I doubt this sentence conveys the
meaning properly.
+ if (!state->has_oversized_tuples)
+ elog(NOTICE, "Index contain tuples that cannot fit into index page,
if toasted with current toast policy");

+ * If the tuple is exampt from checking due to has_oversized_tuples
this function
+ * returns NULL.
maybe
+ * If the tuple is exempt from checking due to has_oversized_tuples,
this function
+ * returns NULL.

you changed
`bool toast_free[INDEX_MAX_KEYS];`
to
`bool need_free[INDEX_MAX_KEYS];`
maybe some comments address the changes, otherwise people would say
why change (maybe I am over thinking).