Re: [HACKERS] Allowing GIN array_ops to work on anyarray

[Tom Lane]

Enrique Meneses  writes:
> Great, given it does not apply to this patch, then all the other tests
> passed and the change looks good.

Pushed, thanks for the review!

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] Allowing GIN array_ops to work on anyarray

[Enrique Meneses]

Great, given it does not apply to this patch, then all the other tests
passed and the change looks good.

Thank you,
Enrique


On Mon, Sep 26, 2016 at 6:27 AM Tom Lane  wrote:

> Enrique Meneses  writes:
> > I was not sure what "Spec compliant means"... so I did not select as
> tested or passed. What should I do to validate that this change is "Spec
> compliant"?
>
> It's irrelevant to this patch, AFAICS.  The SQL standard doesn't discuss
> indexes at all, much less legislate on which operator classes ought to
> exist for GIN indexes.
>
> regards, tom lane
>

Re: [HACKERS] Allowing GIN array_ops to work on anyarray

[Tom Lane]

Enrique Meneses  writes:
> I was not sure what "Spec compliant means"... so I did not select as tested 
> or passed. What should I do to validate that this change is "Spec compliant"?

It's irrelevant to this patch, AFAICS.  The SQL standard doesn't discuss
indexes at all, much less legislate on which operator classes ought to
exist for GIN indexes.

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] Allowing GIN array_ops to work on anyarray

[Enrique Meneses]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   not tested
Documentation:tested, passed

I built and installed (make world / make install-world) github branch 
REL9_6_STABLE and applied the patch (patch -p1 < 
patch/gin-true-anyarray-opclass-1.patch).

I then upgraded my development database (postgres 9.5) using pg_upgrade. This 
database has one table with an UUID array gin index. The database was upgraded 
correctly to postgresql 9.6 and I was able to successfully connect to it from a 
web application which uses the database. There were no conflicts so I expect 
others to be able to upgrade without issues.

I then dropped the database and re-created it... I made sure that I no longer 
used my prior operator class definition. I re-started my web application and 
confirmed it works. This verifies the feature works as designed.

The following is no longer required:

CREATE OPERATOR CLASS _uuid_ops DEFAULT
  FOR TYPE _uuid USING gin AS
  OPERATOR 1 &&(anyarray, anyarray),
  OPERATOR 2 @>(anyarray, anyarray),
  OPERATOR 3 <@(anyarray, anyarray),
  OPERATOR 4 =(anyarray, anyarray),
  FUNCTION 1 uuid_cmp(uuid, uuid),
  FUNCTION 2 ginarrayextract(anyarray, internal, internal),
  FUNCTION 3 ginqueryarrayextract(anyarray, internal, smallint, internal, 
internal, internal, internal),
  FUNCTION 4 ginarrayconsistent(internal, smallint, anyarray, integer, 
internal, internal, internal, internal),
  STORAGE uuid;

I also ran "make installcheck-world" and all the tests passed.

The new status of this patch is: Waiting on Author

-- 
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] Allowing GIN array_ops to work on anyarray

[Enrique Meneses]

I was not sure what "Spec compliant means"... so I did not select as tested or 
passed. What should I do to validate that this change is "Spec compliant"?
-- 
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] Allowing GIN array_ops to work on anyarray

[Enrique Meneses]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   not tested
Documentation:tested, passed

I am resending this due to an earlier error message from the mailing list:
-
These are my comments for the review of 
https://commitfest.postgresql.org/10/708/ 

I built and installed (make world / make install-world) github branch 
REL9_6_STABLE and applied the patch (patch -p1 < 
patch/gin-true-anyarray-opclass-1.patch).

I then upgraded my development database (postgres 9.5) using pg_upgrade. This 
database has one table with an UUID array gin index. The database was upgraded 
correctly to postgresql 9.6 and I was able to successfully connect to it from a 
web application which uses the database. There were no conflicts so I expect 
others to be able to upgrade without issues.

I then dropped the database and re-created it... I made sure that I no longer 
used my prior operator class definition. I re-started my web application and 
confirmed it works. This verifies the feature works as designed.

The following is no longer required:

CREATE OPERATOR CLASS _uuid_ops DEFAULT
  FOR TYPE _uuid USING gin AS
  OPERATOR 1 &&(anyarray, anyarray),
  OPERATOR 2 @>(anyarray, anyarray),
  OPERATOR 3 <@(anyarray, anyarray),
  OPERATOR 4 =(anyarray, anyarray),
  FUNCTION 1 uuid_cmp(uuid, uuid),
  FUNCTION 2 ginarrayextract(anyarray, internal, internal),
  FUNCTION 3 ginqueryarrayextract(anyarray, internal, smallint, internal, 
internal, internal, internal),
  FUNCTION 4 ginarrayconsistent(internal, smallint, anyarray, integer, 
internal, internal, internal, internal),
  STORAGE uuid;

I also ran "make installcheck-world" and all the tests passed.

I am not sure what "Spec compliant means"... so I did not select as tested or 
passed. What should I do to validate that this change is "Spec compliant"?


The new status of this patch is: Waiting on Author

-- 
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] Allowing GIN array_ops to work on anyarray

[M Enrique]

This is awesome. I will build it to start using and testing it in my
development environment. Thank you so much for making this change.

On Thu, Aug 11, 2016 at 11:33 AM Tom Lane  wrote:

> In
> https://www.postgresql.org/message-id/15293.1466536...@sss.pgh.pa.us
> I speculated that it might not take too much to replace all the variants
> of GIN array_ops with a single polymorphic opclass over anyarray.
> Attached is a proposed patch that does that.
>
> There are two bits of added functionality needed to make this work:
>
> 1. We need to abstract the storage type.  The patch does this by teaching
> catalog/index.c to recognize an opckeytype specified as ANYELEMENT with an
> opcintype of ANYARRAY, and doing the array element type lookup at index
> creation time.
>
> 2. We need to abstract the key comparator.  The patch does this by
> teaching gin/ginutil.c that if the opclass omits a GIN_COMPARE_PROC,
> it should look up the default btree comparator for the index key type.
>
> Both of these seem to me to be reasonable general-purpose behaviors with
> potential application to other opclasses.
>
> In the aforementioned message I worried that a core opclass defined this
> way might conflict with user-built opclasses for specific array types,
> but it seems to work out fine without any additional tweaks: CREATE INDEX
> already prefers an exact match if it finds one, and only falls back to
> matching anyarray when it doesn't.  Also, all the replaced opclasses are
> presently default for their types, which means that pg_dump won't print
> them explicitly in CREATE INDEX commands, so we don't have a dump/reload
> or pg_upgrade hazard from them disappearing.
>
> A potential downside is that for an opclass defined this way, we add a
> lookup_type_cache() call to each initGinState() call.  That's basically
> just a single dynahash lookup once the caches are populated, so it's not
> much added cost, but conceivably it could be measurable in bulk insert
> operations.  If it does prove objectionable my inclination would be to
> look into ways to avoid the repetitive function lookups of initGinState,
> perhaps by letting it cache that stuff in the index's relcache entry.
>
> I'll put this on the September commitfest docket.
>
> regards, tom lane
>
>

[HACKERS] Allowing GIN array_ops to work on anyarray

[Tom Lane]

In
https://www.postgresql.org/message-id/15293.1466536...@sss.pgh.pa.us
I speculated that it might not take too much to replace all the variants
of GIN array_ops with a single polymorphic opclass over anyarray.
Attached is a proposed patch that does that.

There are two bits of added functionality needed to make this work:

1. We need to abstract the storage type.  The patch does this by teaching
catalog/index.c to recognize an opckeytype specified as ANYELEMENT with an
opcintype of ANYARRAY, and doing the array element type lookup at index
creation time.

2. We need to abstract the key comparator.  The patch does this by
teaching gin/ginutil.c that if the opclass omits a GIN_COMPARE_PROC,
it should look up the default btree comparator for the index key type.

Both of these seem to me to be reasonable general-purpose behaviors with
potential application to other opclasses.

In the aforementioned message I worried that a core opclass defined this
way might conflict with user-built opclasses for specific array types,
but it seems to work out fine without any additional tweaks: CREATE INDEX
already prefers an exact match if it finds one, and only falls back to
matching anyarray when it doesn't.  Also, all the replaced opclasses are
presently default for their types, which means that pg_dump won't print
them explicitly in CREATE INDEX commands, so we don't have a dump/reload
or pg_upgrade hazard from them disappearing.

A potential downside is that for an opclass defined this way, we add a
lookup_type_cache() call to each initGinState() call.  That's basically
just a single dynahash lookup once the caches are populated, so it's not
much added cost, but conceivably it could be measurable in bulk insert
operations.  If it does prove objectionable my inclination would be to
look into ways to avoid the repetitive function lookups of initGinState,
perhaps by letting it cache that stuff in the index's relcache entry.

I'll put this on the September commitfest docket.

regards, tom lane

diff --git a/doc/src/sgml/gin.sgml b/doc/src/sgml/gin.sgml
index 05d92eb..d0d2ba8 100644
*** a/doc/src/sgml/gin.sgml
--- b/doc/src/sgml/gin.sgml
***
*** 441,462 
   
  
   
!There are three methods that an operator class for
 GIN must provide:
  
!  
! 
!  int compare(Datum a, Datum b)
!  
!   
!Compares two keys (not indexed items!) and returns an integer less than
!zero, zero, or greater than zero, indicating whether the first key is
!less than, equal to, or greater than the second.  Null keys are never
!passed to this function.
!   
!  
! 
! 
  
   Datum *extractValue(Datum itemValue, int32 *nkeys,
  bool **nullFlags)
--- 441,450 
   
  
   
!There are two methods that an operator class for
 GIN must provide:
  
!   
  
   Datum *extractValue(Datum itemValue, int32 *nkeys,
  bool **nullFlags)
***
*** 645,651 
--- 633,670 
   
  

+  
+ 
+  
+   In addition, GIN must have a way to sort the key values stored in the index.
+   The operator class can define the sort ordering by specifying a comparison
+   method:
  
+   
+ 
+  int compare(Datum a, Datum b)
+  
+   
+Compares two keys (not indexed items!) and returns an integer less than
+zero, zero, or greater than zero, indicating whether the first key is
+less than, equal to, or greater than the second.  Null keys are never
+passed to this function.
+   
+  
+ 
+   
+ 
+   Alternatively, if the operator class does not provide a compare
+   method, GIN will look up the default btree operator class for the index
+   key data type, and use its comparison function.  It is recommended to
+   specify the comparison function in a GIN operator class that is meant for
+   just one data type, as looking up the btree operator class costs a few
+   cycles.  However, polymorphic GIN operator classes (such
+   as array_ops) typically cannot specify a single comparison
+   function.
+  
+ 
+  
Optionally, an operator class for GIN can supply the
following method:
  
diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c
index a2450f4..8bfa924 100644
*** a/src/backend/access/gin/ginutil.c
--- b/src/backend/access/gin/ginutil.c
***
*** 22,28 
--- 22,30 
  #include "miscadmin.h"
  #include "storage/indexfsm.h"
  #include "storage/lmgr.h"
+ #include "utils/builtins.h"
  #include "utils/index_selfuncs.h"
+ #include "utils/typcache.h"
  
  
  /*
*** initGinState(GinState *state, Relation i
*** 104,112 
  		origTupdesc->attrs[i]->attcollation);
  		}
  
! 		fmgr_info_copy(&(state->compareFn[i]),
! 	   index_getprocinfo(index, i + 1, GIN_COMPARE_PROC),
! 	   CurrentMemoryContext);
  		fmgr_info_copy(&(state->extractValueFn[i]),
  	   index_getprocinfo(index, i + 1,