On 04/25/2017 04:10 AM, Noah Misch wrote:
> On Mon, Apr 24, 2017 at 09:25:25AM -0700, Mark Dilger wrote:
>> Noah, if you left this case out intentionally, sorry for the noise. I did not
>> immediately see any reason not to follow your lead for this function.
>
> This is not following my lead, but that doesn't make it bad. It's just a
> different topic.
(Changed subject line accordingly.)
From the patch:
> --- a/src/backend/utils/adt/varbit.c
> +++ b/src/backend/utils/adt/varbit.c
> @@ -1187,7 +1187,7 @@ bit_overlay(VarBit *t1, VarBit *t2, int sp, int sl)
> Datum
> bitlength(PG_FUNCTION_ARGS)
> {
> - VarBit *arg = PG_GETARG_VARBIT_P(0);
> + VarBit *arg = PG_GETARG_VARBIT_P_SLICE(0,0,VARHDRSZ+VARBITHDRSZ);
>
> PG_RETURN_INT32(VARBITLEN(arg));
> }
That doesn't look quite right. PG_GETARG_VARBIT_P_SLICE(X, m, n) returns
n bytes, from offset m, within the varlena. Offset 0 points to just
after the varlen header, i.e. the bit length. AFAICS, there's no need to
include VARHDRSZ here, and this should be just "arg =
PG_GETARG_VARBIT_P_SLICE(0, 0, VARBITHDRSZ)". It's a harmless mistake to
fetch more data than needed, but let's try to not be confused over how
slices work.
I wonder if having a PG_GETARG_VARBIT_P_SLICE macro like this is really
a good idea. It might be useful to be able to fetch just the header, to
get the length, like in this function. And bitgetbit() function would
benefit from being able to fetch just a slice of the data, containing
the bit its interested in. But this macro seems quite inconvenient for
both of those use cases. I'm not sure what to do instead, but I think
that needs some more thought.
I'd suggest expanding this patch, to also make bitgetbit to fetch just a
slice, and see what that looks like.
- Heikki
