Re: [HACKERS] PG_GETARG_GISTENTRY?
Mark Dilger 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PG_GETARG_GISTENTRY?
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PG_GETARG_GISTENTRY?
> On Apr 5, 2017, at 1:27 PM, Mark Dilger wrote: > > >> On Apr 5, 2017, at 1:12 PM, Tom Lane wrote: >> >> Mark Dilger 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 varbit_packed.patch.1 Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PG_GETARG_GISTENTRY?
> On Apr 5, 2017, at 1:12 PM, Tom Lane wrote: > > Mark Dilger 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PG_GETARG_GISTENTRY?
Mark Dilger 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PG_GETARG_GISTENTRY?
> On Apr 5, 2017, at 9:23 AM, Tom Lane wrote: > > Robert Haas writes: >> On Wed, Mar 29, 2017 at 11:54 PM, Tom Lane wrote: >>> Andres Freund 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 PG_GETARG_foo_P.patch.1 Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PG_GETARG_GISTENTRY?
Robert Haas writes: > On Wed, Mar 29, 2017 at 11:54 PM, Tom Lane wrote: >> Andres Freund 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PG_GETARG_GISTENTRY?
On Wed, Mar 29, 2017 at 11:54 PM, Tom Lane wrote: > Andres Freund 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PG_GETARG_GISTENTRY?
Andres Freund 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] PG_GETARG_GISTENTRY?
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers