王跃林 <violin0613@tju.edu.cn> writes:
> The analysis and fix look correct. The BtreeGistNotEqual branch is
> the only strategy that bypasses the is_leaf check and passes
> potentially truncated internal-node keys directly to f_eq, which is
> unsafe for types like bit and varbit that require a minimum valid
> header size. Returning true for internal nodes is the right
> conservative choice and is consistent with how the other strategies
> handle the internal-node case.
None of this passes the smell test for me. If it's unsafe to
call the type's f_eq function on a truncated key, how is it
any safer to call the f_cmp function? IOW, why aren't *all* the
cases in gbt_var_consistent() broken?
I think the difference is that for bit/varbit f_eq and f_cmp aren't the
same kind of function. f_eq is biteq (reads bit_len, does VARSIZE-8),
while f_cmp is byteacmp (plain byte-wise, only needs the varlena
header). On internal pages every other strategy goes through
f_cmp/byteacmp, and only the <> branch reaches f_eq. So I believe
that's why the byte-wise cases stay safe on a truncated key and <>
doesn't.
It looks to me like gbt_var_node_truncate adjusts the length words
of the truncated keys so that they are still valid, just shorter.
Or at least it's trying to. That works fine for text and bytea,
but it's not fine for bit/varbit because (a) it fails to update the
"bit_len" field that follows the length word, and (b) the common
prefix length selection logic doesn't know that it mustn't truncate
away any part of the bit_len field. So my own thought about fixing
this is that type bit needs a custom truncation method.
That was my first assumption too, but it looks like the node key has
no bit_len to preserve: gbt_bit_l2n/gbt_bit_xfrm seem to drop it and
keep only the raw bits before truncation runs. If so, a custom
truncation may not help, since the crash seems to come from <> calling
biteq rather than a damaged bit_len. Though I'm not fully sure I'm
reading the l2n step right.
(I continue to regret that we ever accepted such underdocumented code.
I think we ought to reverse-engineer a comment explaining what
gbt_var_consistent is doing, eg, why are all of the tests seemingly
reversed?)
Agreed, we can add a comment there.
Regards,
Ayush
