Re: [HACKERS] hstore: add hstore_length function

[Robert Haas]

On Wed, Jun 8, 2016 at 10:44 AM, Robert Haas  wrote:
> On Mon, Jun 6, 2016 at 7:57 PM, Korbin Hoffman  wrote:
>> With regards to your second point- I've been maintaining consistency
>> with the rest of the hstore module. Hstore's _size is internally
>> stored as a uint, but all uses of HS_COUNT across the feature end up
>> stored in a signed int. I could only find (grep) a few occurrences of
>> PG_RETURN_UINT32 across the entire codebase, and none in the hstore
>> module. If there's strong consensus for change, though, I'm happy to
>> do so.
>
> The PG_RETURN_BLAH macro chosen should match the declared return type
> of that function.  So if your function, for example, returns int4 (or
> integer, which is the same thing), PG_RETURN_INT32 is correct.
>
> There are no built-in SQL datatypes for unsigned integers, which is
> why you did not find many uses of PG_RETURN_UINT32 in the code base.

Since this patch was never updated in response to this review, I am
marking it "Returned with Feedback" in this CommitFest.  If it is
updated, it can be resubmitted to a future CommitFest.

-- 
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] hstore: add hstore_length function

[Robert Haas]

On Mon, Jun 6, 2016 at 7:57 PM, Korbin Hoffman  wrote:
> With regards to your second point- I've been maintaining consistency
> with the rest of the hstore module. Hstore's _size is internally
> stored as a uint, but all uses of HS_COUNT across the feature end up
> stored in a signed int. I could only find (grep) a few occurrences of
> PG_RETURN_UINT32 across the entire codebase, and none in the hstore
> module. If there's strong consensus for change, though, I'm happy to
> do so.

The PG_RETURN_BLAH macro chosen should match the declared return type
of that function.  So if your function, for example, returns int4 (or
integer, which is the same thing), PG_RETURN_INT32 is correct.

There are no built-in SQL datatypes for unsigned integers, which is
why you did not find many uses of PG_RETURN_UINT32 in the code base.

-- 
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] hstore: add hstore_length function

[Korbin Hoffman]

Thanks for the review, Fabrízio.

Attached is the updated patch, rebased and tested against master.

I integrated feedback from your first point and am no longer
HS_POLLUTE'ing the namespace for the new function.

With regards to your second point- I've been maintaining consistency
with the rest of the hstore module. Hstore's _size is internally
stored as a uint, but all uses of HS_COUNT across the feature end up
stored in a signed int. I could only find (grep) a few occurrences of
PG_RETURN_UINT32 across the entire codebase, and none in the hstore
module. If there's strong consensus for change, though, I'm happy to
do so.

Thanks,
Korbin Hoffman

On Mon, Jun 6, 2016 at 1:23 PM, Fabrízio de Royes Mello
 wrote:
>
>
> On Fri, Jun 3, 2016 at 7:58 AM, Korbin Hoffman  wrote:
>>
>> Hi there-
>>
>> I've attached a small patch exposing HS_COUNT to the user as
>> "hstore_length(hstore)". Documentation, upgrade sql files, and a few
>> tests are also included.
>>
>> Almost every hstore function calls HS_COUNT, but I couldn't determine
>> if there was a reason this exposure didn't already exist.
>>
>> Without this function, I've been converting an hstore into an array
>> and then counting it - a more expensive operation (~30-40% slower than
>> SELECTing the hstore itself in a few of my tests).
>>
>> I will add this thread and patch to the next Commitfest.
>>
>
> Something goes wrong when applying against master:
>
> $ git apply ~/Downloads/hstore_length-v1.patch
> error: contrib/hstore/Makefile: already exists in working directory
> error: contrib/hstore/expected/hstore.out: already exists in working
> directory
> error: contrib/hstore/hstore--1.3.sql: already exists in working directory
> error: contrib/hstore/hstore.control: already exists in working directory
> error: contrib/hstore/hstore_op.c: already exists in working directory
> error: contrib/hstore/sql/hstore.sql: already exists in working directory
> error: doc/src/sgml/hstore.sgml: already exists in working directory
>
>
> Anyway I have some comments:
>
> 1) I don't see any reason to add this sort of thing if you're adding a new
> function
>
> + HSTORE_POLLUTE(hstore_length, length);
>
>
> 2) Shouldn't this declaration use 'uint32' instead of 'int' ??
>
> + int count = HS_COUNT(hs);
> +
> + PG_RETURN_INT32(count);
>
> maybe
>
> +  uint32 count = HS_COUNT(hs);
> +
> +  PG_RETURN_UINT32(count);
>
> Regards,
>
> --
> Fabrízio de Royes Mello
> Consultoria/Coaching PostgreSQL
>>> Timbira: http://www.timbira.com.br
>>> Blog: http://fabriziomello.github.io
>>> Linkedin: http://br.linkedin.com/in/fabriziomello
>>> Twitter: http://twitter.com/fabriziomello
>>> Github: http://github.com/fabriziomello


hstore_length-v2.patch
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] hstore: add hstore_length function

[Fabrízio de Royes Mello]

On Fri, Jun 3, 2016 at 7:58 AM, Korbin Hoffman  wrote:
>
> Hi there-
>
> I've attached a small patch exposing HS_COUNT to the user as
> "hstore_length(hstore)". Documentation, upgrade sql files, and a few
> tests are also included.
>
> Almost every hstore function calls HS_COUNT, but I couldn't determine
> if there was a reason this exposure didn't already exist.
>
> Without this function, I've been converting an hstore into an array
> and then counting it - a more expensive operation (~30-40% slower than
> SELECTing the hstore itself in a few of my tests).
>
> I will add this thread and patch to the next Commitfest.
>

Something goes wrong when applying against master:

$ git apply ~/Downloads/hstore_length-v1.patch
error: contrib/hstore/Makefile: already exists in working directory
error: contrib/hstore/expected/hstore.out: already exists in working
directory
error: contrib/hstore/hstore--1.3.sql: already exists in working directory
error: contrib/hstore/hstore.control: already exists in working directory
error: contrib/hstore/hstore_op.c: already exists in working directory
error: contrib/hstore/sql/hstore.sql: already exists in working directory
error: doc/src/sgml/hstore.sgml: already exists in working directory


Anyway I have some comments:

1) I don't see any reason to add this sort of thing if you're adding a new
function

+ HSTORE_POLLUTE(hstore_length, length);


2) Shouldn't this declaration use 'uint32' instead of 'int' ??

+ int count = HS_COUNT(hs);
+
+ PG_RETURN_INT32(count);

maybe

+  uint32 count = HS_COUNT(hs);
+
+  PG_RETURN_UINT32(count);

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello