Horton

Thread

Commits

  1. Make DatumGetFoo/PG_GETARG_FOO/PG_RETURN_FOO macro names more consistent.

  2. Use wrappers of PG_DETOAST_DATUM_PACKED() more.

  3. Recommend wrappers of PG_DETOAST_DATUM_PACKED().

  1. PG_GETARG_GISTENTRY?

    Andres Freund <andres@anarazel.de> — 2017-03-29T19:27:15Z

    Hi,

we have a good number of '(GISTENTRY *) PG_GETARG_POINTER(n)' in our
code - looks a bit better & shorter to have PG_GETARG_GISTENTRY(n).

Arugments against?

Greetings,

Andres Freund

  2. Re: PG_GETARG_GISTENTRY?

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-03-30T03:54:22Z

    Andres Freund <andres@anarazel.de> writes:
> we have a good number of '(GISTENTRY *) PG_GETARG_POINTER(n)' in our
> code - looks a bit better & shorter to have PG_GETARG_GISTENTRY(n).

Should be PG_GETARG_GISTENTRY_P to match existing conventions,
otherwise +1

			regards, tom lane

  3. Re: PG_GETARG_GISTENTRY?

    Robert Haas <robertmhaas@gmail.com> — 2017-04-05T16:06:43Z

    On Wed, Mar 29, 2017 at 11:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andres Freund <andres@anarazel.de> writes:
>> we have a good number of '(GISTENTRY *) PG_GETARG_POINTER(n)' in our
>> code - looks a bit better & shorter to have PG_GETARG_GISTENTRY(n).
>
> Should be PG_GETARG_GISTENTRY_P to match existing conventions,
> otherwise +1

I have never quite understood why some of those macros have _P or _PP
on the end and others don't.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  4. Re: PG_GETARG_GISTENTRY?

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-05T16:23:02Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Mar 29, 2017 at 11:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Andres Freund <andres@anarazel.de> writes:
>>> we have a good number of '(GISTENTRY *) PG_GETARG_POINTER(n)' in our
>>> code - looks a bit better & shorter to have PG_GETARG_GISTENTRY(n).

>> Should be PG_GETARG_GISTENTRY_P to match existing conventions,
>> otherwise +1

> I have never quite understood why some of those macros have _P or _PP
> on the end and others don't.

_P means "pointer to".  _PP was introduced later to mean "pointer to
packed (ie, possibly short-header) datum".  Macros that mean to fetch
pointers to pass-by-ref data, but aren't using either of those naming
conventions, are violating project conventions, not least because you
don't know what they're supposed to do with short-header varlena input.
If I had a bit more spare time I'd run around and change any such macros.

In short, if you are supposed to write

	FOO  *val = PG_GETARG_FOO(n);

then the macro designer blew it, because the name implies that it
returns FOO, not pointer to FOO.  This should be

	FOO  *val = PG_GETARG_FOO_P(n);

			regards, tom lane

  5. Re: PG_GETARG_GISTENTRY?

    Mark Dilger <hornschnorter@gmail.com> — 2017-04-05T20:04:29Z

    > On Apr 5, 2017, at 9:23 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Wed, Mar 29, 2017 at 11:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Andres Freund <andres@anarazel.de> writes:
>>>> we have a good number of '(GISTENTRY *) PG_GETARG_POINTER(n)' in our
>>>> code - looks a bit better & shorter to have PG_GETARG_GISTENTRY(n).
> 
>>> Should be PG_GETARG_GISTENTRY_P to match existing conventions,
>>> otherwise +1
> 
>> I have never quite understood why some of those macros have _P or _PP
>> on the end and others don't.
> 
> _P means "pointer to".  _PP was introduced later to mean "pointer to
> packed (ie, possibly short-header) datum".  Macros that mean to fetch
> pointers to pass-by-ref data, but aren't using either of those naming
> conventions, are violating project conventions, not least because you
> don't know what they're supposed to do with short-header varlena input.
> If I had a bit more spare time I'd run around and change any such macros.
> 
> In short, if you are supposed to write
> 
> 	FOO  *val = PG_GETARG_FOO(n);
> 
> then the macro designer blew it, because the name implies that it
> returns FOO, not pointer to FOO.  This should be
> 
> 	FOO  *val = PG_GETARG_FOO_P(n);
> 
> 			regards, tom lane

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.

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.  Varbit's bitoctetlength function
could detoast only the header ala PG_DETOAST_DATUM_SLICE to return the
octet length, rather than detoasting the whole thing.  But that seems a different
issue, and patches to change that might have been rejected in the past so far as I
know, so I did not attempt any such changes here.

Mark Dilger

  6. Re: PG_GETARG_GISTENTRY?

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-05T20:12:23Z

    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.  This seems a bit late for v10,
especially since it's only cosmetic, but please put it in the first
v11 commitfest.

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

			regards, tom lane

  7. Re: PG_GETARG_GISTENTRY?

    Mark Dilger <hornschnorter@gmail.com> — 2017-04-05T20:27:02Z

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

  8. Re: PG_GETARG_GISTENTRY?

    Mark Dilger <hornschnorter@gmail.com> — 2017-04-24T16:25:25Z

    > 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

  9. Re: PG_GETARG_GISTENTRY?

    Noah Misch <noah@leadboat.com> — 2017-04-25T01:10:52Z

    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.

  10. Use PG_DETOAST_DATUM_SLICE in bitlength() (was Re: PG_GETARG_GISTENTRY?)

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-08-16T11:00:03Z

    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

  11. Renaming PG_GETARG functions (was Re: PG_GETARG_GISTENTRY?)

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-12T20:07:35Z

    [ changing subject line to possibly draw more attention ]

Mark Dilger <hornschnorter@gmail.com> writes:
>> On Apr 5, 2017, at 9:23 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> In short, if you are supposed to write
>> 	FOO  *val = PG_GETARG_FOO(n);
>> then the macro designer blew it, because the name implies that it
>> returns FOO, not pointer to FOO.  This should be
>> 	FOO  *val = PG_GETARG_FOO_P(n);

> I have written a patch to fix these macro definitions across src/ and
> contrib/.

So to summarize, this patch proposes to rename some DatumGetFoo,
PG_GETARG_FOO, and PG_RETURN_FOO macros for these datatypes:

NDBOX (contrib/cube)
HSTORE
LTREE and other contrib/ltree types

PG_GETARG_ANY_ARRAY (and there are some related macros it maybe should
have touched, like PG_RETURN_EXPANDED_ARRAY)

JSONB

RANGE

The contrib types don't seem like much of a problem, but I wonder
whether anyone feels that rationalizing the names for array, JSON,
or range-type macros will break too much code.

One option if we do feel that way is that we could provide the
old names as alternatives, thus not breaking external modules.
But that seems like it's sabotaging the basic goal of improving
consistency of naming.

If there are not objections, I plan to push forward with this.

			regards, tom lane

  12. Re: Renaming PG_GETARG functions (was Re: PG_GETARG_GISTENTRY?)

    Mark Dilger <hornschnorter@gmail.com> — 2017-09-12T22:16:01Z

    > On Sep 12, 2017, at 1:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> [ changing subject line to possibly draw more attention ]
> 
> Mark Dilger <hornschnorter@gmail.com> writes:
>>> On Apr 5, 2017, at 9:23 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> In short, if you are supposed to write
>>> 	FOO  *val = PG_GETARG_FOO(n);
>>> then the macro designer blew it, because the name implies that it
>>> returns FOO, not pointer to FOO.  This should be
>>> 	FOO  *val = PG_GETARG_FOO_P(n);
> 
>> I have written a patch to fix these macro definitions across src/ and
>> contrib/.
> 

Thanks, Tom, for reviewing my patch.

  13. Re: Renaming PG_GETARG functions (was Re: PG_GETARG_GISTENTRY?)

    Daniel Gustafsson <daniel@yesql.se> — 2017-09-15T15:00:44Z

    > On 12 Sep 2017, at 22:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> [ changing subject line to possibly draw more attention ]
> 
> Mark Dilger <hornschnorter@gmail.com> writes:
>>> On Apr 5, 2017, at 9:23 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> In short, if you are supposed to write
>>> 	FOO  *val = PG_GETARG_FOO(n);
>>> then the macro designer blew it, because the name implies that it
>>> returns FOO, not pointer to FOO.  This should be
>>> 	FOO  *val = PG_GETARG_FOO_P(n);
> 
>> I have written a patch to fix these macro definitions across src/ and
>> contrib/.
> 
> So to summarize, this patch proposes to rename some DatumGetFoo,
> PG_GETARG_FOO, and PG_RETURN_FOO macros for these datatypes:
> 
> NDBOX (contrib/cube)
> HSTORE
> LTREE and other contrib/ltree types
> 
> PG_GETARG_ANY_ARRAY (and there are some related macros it maybe should
> have touched, like PG_RETURN_EXPANDED_ARRAY)
> 
> JSONB
> 
> RANGE
> 
> The contrib types don't seem like much of a problem, but I wonder
> whether anyone feels that rationalizing the names for array, JSON,
> or range-type macros will break too much code.
> 
> One option if we do feel that way is that we could provide the
> old names as alternatives, thus not breaking external modules.
> But that seems like it's sabotaging the basic goal of improving
> consistency of naming.
> 
> If there are not objections, I plan to push forward with this.

Judging by this post, I’m updating this to “Ready for Committer”.

cheers ./daniel

  14. Re: PG_GETARG_GISTENTRY?

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-18T19:26:36Z

    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.

Pushed after some rebasing and some minor additional editorialization.

The original point about adding a wrapper for GISTENTRY fetches remains
open ...

			regards, tom lane