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