Обсуждение: Re: [HACKERS] PG_GETARG_GISTENTRY?

> On Apr 5, 2017, at 1:27 PM, Mark Dilger <hornschnorter@gmail.com> wrote:
>
>
>> On Apr 5, 2017, at 1:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> Mark Dilger <hornschnorter@gmail.com> writes:
>>> I have written a patch to fix these macro definitions across src/ and contrib/.
>>> Find the patch, attached.  All regression tests pass on my Mac laptop.
>>
>> Thanks for doing the legwork on that.
>
> You are welcome.
>
>> This seems a bit late for v10,
>> especially since it's only cosmetic
>
> Agreed.
>
>> , but please put it in the first
>> v11 commitfest.
>
> Done.
>
>>
>>> I don't find any inappropriate uses of _P where _PP would be called for.  I do,
>>> however, notice that some datatypes' functions are written to use PG_GETARG_*_P
>>> where PG_GETARG_*_PP might be more efficient.
>>
>> Yeah.  I think Noah did some work in that direction already, but I don't
>> believe he claimed to have caught everything.  Feel free to push further.
>
> Thanks for clarifying.
>

Here is a small patch for the next open commitfest which handles a case
that Noah's commits 9d7726c2ba06b932f791f2d0cc5acf73cc0b4dca and
3a0d473192b2045cbaf997df8437e7762d34f3ba apparently missed.

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.

Mark Dilger


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Apr 24, 2017 at 09:25:25AM -0700, Mark Dilger wrote:
> Here is a small patch for the next open commitfest which handles a case
> that Noah's commits 9d7726c2ba06b932f791f2d0cc5acf73cc0b4dca and
> 3a0d473192b2045cbaf997df8437e7762d34f3ba apparently missed.

The scope for those commits was wrappers of PG_DETOAST_DATUM_PACKED(), which
does not include PG_DETOAST_DATUM_SLICE().

> 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.

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