Re: compress method for spgist - 2

[Tom Lane]

Alexander Korotkov  writes:
> On Thu, Jan 4, 2018 at 1:17 AM, Dagfinn Ilmari Mannsåker 
> wrote:
>> This patch added two copies of the poly_ops row to the "Built-in SP-GiST
>> Operator Classes" table in spgist.sgml.

> Thank for fixing this!  I'm sure that Teodor will push this after end of
> New Year holidays in Russia.

He didn't ... so I did.

regards, tom lane

Re: compress method for spgist - 2

[Daniel Gustafsson]

> On 04 Jan 2018, at 06:17, Dagfinn Ilmari Mannsåker  wrote:
> 
> Teodor Sigaev  writes:
> 
>>> Now, this patch is ready for committer from my point of view.
>> 
>> Thank you, pushed
> 
> This patch added two copies of the poly_ops row to the "Built-in SP-GiST
> Operator Classes" table in spgist.sgml.  The attached patched removes
> one of them.

Patch looks good, marked as Ready for Committer in the CF app.

cheers ./daniel

Re: compress method for spgist - 2

[Alexander Korotkov]

On Thu, Jan 4, 2018 at 1:17 AM, Dagfinn Ilmari Mannsåker 
wrote:

> Teodor Sigaev  writes:
>
> >>
> >> Now, this patch is ready for committer from my point of view.
> >
> > Thank you, pushed
>
> This patch added two copies of the poly_ops row to the "Built-in SP-GiST
> Operator Classes" table in spgist.sgml.


Right.


> The attached patched removes
> one of them.
>

Thank for fixing this!  I'm sure that Teodor will push this after end of
New Year holidays in Russia.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: compress method for spgist - 2

[Dagfinn Ilmari Mannsåker]

Teodor Sigaev  writes:

>>
>> Now, this patch is ready for committer from my point of view.
>
> Thank you, pushed

This patch added two copies of the poly_ops row to the "Built-in SP-GiST
Operator Classes" table in spgist.sgml.  The attached patched removes
one of them.

- ilmari
-- 
- Twitter seems more influential [than blogs] in the 'gets reported in
  the mainstream press' sense at least.   - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
  to a mainstream media article.  - Calle Dybedahl

>From 481afc4476f6eb3ec357ed795ce382ff1cb432fa Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Wed, 3 Jan 2018 22:04:09 +
Subject: [PATCH] Remove duplicate poly_ops row from SP-GiST opclass table

Commit ff963b393c added two identical copies of this row.
---
 doc/src/sgml/spgist.sgml | 18 --
 1 file changed, 18 deletions(-)

diff --git a/doc/src/sgml/spgist.sgml b/doc/src/sgml/spgist.sgml
index 51bb60c92a..e47f70be89 100644
--- a/doc/src/sgml/spgist.sgml
+++ b/doc/src/sgml/spgist.sgml
@@ -148,24 +148,6 @@
|&>
   
  
- 
-  poly_ops
-  polygon
-  
-   <<
-   &<
-   &&
-   &>
-   >>
-   ~=
-   @>
-   <@
-   &<|
-   <<|
-   |>>
-   |&>
-  
- 
  
   text_ops
   text
-- 
2.15.1

Re: compress method for spgist - 2

[Teodor Sigaev]

Now, this patch is ready for committer from my point of view.


Thank you, pushed

--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/

Re: compress method for spgist - 2

[Alexander Korotkov]

On Thu, Dec 7, 2017 at 3:17 AM, Nikita Glukhov 
wrote:

> On 06.12.2017 21:49, Alexander Korotkov wrote:
>
> On Wed, Dec 6, 2017 at 6:08 PM, Nikita Glukhov 
> wrote:
>
>> On 05.12.2017 23:59, Alexander Korotkov wrote:
>>
>> On Tue, Dec 5, 2017 at 1:14 PM, Darafei Praliaskouski 
>> wrote:
>>
>>> The following review has been posted through the commitfest application:
>>> make installcheck-world:  not tested
>>> Implements feature:   not tested
>>> Spec compliant:   not tested
>>> Documentation:tested, passed
>>>
>>> I've read the updated patch and see my concerns addressed.
>>>
>>> I'm looking forward to SP-GiST compress method support, as it will allow
>>> usage of SP-GiST index infrastructure for PostGIS.
>>>
>>> The new status of this patch is: Ready for Committer
>>>
>>
>> I went trough this patch.  I found documentation changes to be not
>> sufficient.  And I've made some improvements.
>>
>> In particular, I didn't understand why is reconstructedValue claimed to
>> be of spgConfigOut.leafType while it should be of spgConfigIn.attType both
>> from general logic and code.  I've fixed that.  Nikita, correct me if I'm
>> wrong.
>>
>>
>> I think we are reconstructing a leaf datum, so documentation was correct
>> but the code in spgWalk() and freeScanStackEntry() wrongly used attType
>> instead of attLeafType. Fixed patch is attached.
>>
>
> Reconstructed datum is used for index-only scan.  Thus, it should be
> original indexed datum of attType, unless we have decompress method and
> pass reconstructed datum through it.
>
> But if we have compress method and do not have decompress method then
> index-only scan seems to be impossible.
>

Sorry, I didn't realize that we don't return reconstructed value
immediately, but return only leafValue provided by leafConsistent.  In this
case, leafConsistent is responsible to convert value from
spgConfigOut.leafType to spgConfigIn.attType.

TBH, practical example of SP-GiST opclass with both compress method and
index-only scan support doesn't come to my mind, because compress method is
typically needed when we have lossy representation of index keys.  For
example, in GiST all the opclasses where compress method do useful work use
lossy representation of keys.  Nevertheless, it's good to not cut
possibility of index-only scans when spgConfigOut.leafType !=
spgConfigIn.attType.  And to lay responsibility for conversion on
leafConsistent seems like elegant way to do this.  So, that's OK for me.

Also, I wonder should we check for existence of compress method when
>> attType and leafType are not the same in spgvalidate() function?  We don't
>> do this for now.
>>
>> I've added compress method existence check to spgvalidate().
>>
> It would be nice to evade double calling of config method.  Possible
> option could be to memorize difference between attribute type and leaf type
> in high bit of functionset, which is guaranteed to be free.
>
> I decided to simply set compress method's bit in functionset when this
> method it is not required.
>

Looks good for me.

Now, this patch is ready for committer from my point of view.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: compress method for spgist - 2

[Nikita Glukhov]

On 06.12.2017 21:49, Alexander Korotkov wrote:
On Wed, Dec 6, 2017 at 6:08 PM, Nikita Glukhov 
mailto:n.glu...@postgrespro.ru>> wrote:


On 05.12.2017 23:59, Alexander Korotkov wrote:


On Tue, Dec 5, 2017 at 1:14 PM, Darafei Praliaskouski
mailto:m...@komzpa.net>> wrote:

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

I've read the updated patch and see my concerns addressed.

I'm looking forward to SP-GiST compress method support, as it
will allow usage of SP-GiST index infrastructure for PostGIS.

The new status of this patch is: Ready for Committer


I went trough this patch.  I found documentation changes to be
not sufficient.  And I've made some improvements.

In particular, I didn't understand why is reconstructedValue
claimed to be of spgConfigOut.leafType while it should be of
spgConfigIn.attType both from general logic and code.  I've fixed
that.  Nikita, correct me if I'm wrong.


I think we are reconstructing a leaf datum, so documentation was
correct but the code in spgWalk() and freeScanStackEntry() wrongly
used attType instead of attLeafType. Fixed patch is attached.


Reconstructed datum is used for index-only scan.  Thus, it should be 
original indexed datum of attType, unless we have decompress method 
and pass reconstructed datum through it.
But if we have compress method and do not have decompress method then 
index-only scan seems to be impossible.



Also, I wonder should we check for existence of compress method
when attType and leafType are not the same in spgvalidate()
function?  We don't do this for now.

I've added compress method existence check to spgvalidate().

It would be nice to evade double calling of config method.  Possible 
option could be to memorize difference between attribute type and leaf 
type in high bit of functionset, which is guaranteed to be free.
I decided to simply set compress method's bit in functionset when this 
method it is not required.


--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
diff --git a/doc/src/sgml/spgist.sgml b/doc/src/sgml/spgist.sgml
index 139c8ed..b4a8be4 100644
--- a/doc/src/sgml/spgist.sgml
+++ b/doc/src/sgml/spgist.sgml
@@ -240,20 +240,22 @@
 
  
   There are five user-defined methods that an index operator class for
-  SP-GiST must provide.  All five follow the convention
-  of accepting two internal arguments, the first of which is a
-  pointer to a C struct containing input values for the support method,
-  while the second argument is a pointer to a C struct where output values
-  must be placed.  Four of the methods just return void, since
-  all their results appear in the output struct; but
+  SP-GiST must provide, and one is optional.  All five
+  mandatory methods follow the convention of accepting two internal
+  arguments, the first of which is a pointer to a C struct containing input
+  values for the support method, while the second argument is a pointer to a
+  C struct where output values must be placed.  Four of the mandatory methods just
+  return void, since all their results appear in the output struct; but
   leaf_consistent additionally returns a boolean result.
   The methods must not modify any fields of their input structs.  In all
   cases, the output struct is initialized to zeroes before calling the
-  user-defined method.
+  user-defined method.  Optional sixth method compress
+  accepts datum to be indexed as the only argument and returns value suitable
+  for physical storage in leaf tuple.
  
 
  
-  The five user-defined methods are:
+  The five mandatory user-defined methods are:
  
 
  
@@ -283,6 +285,7 @@ typedef struct spgConfigOut
 {
 Oid prefixType; /* Data type of inner-tuple prefixes */
 Oid labelType;  /* Data type of inner-tuple node labels */
+Oid leafType;   /* Data type of leaf-tuple values */
 boolcanReturnData;  /* Opclass can reconstruct original data */
 boollongValuesOK;   /* Opclass can cope with values > 1 page */
 } spgConfigOut;
@@ -305,6 +308,22 @@ typedef struct spgConfigOut
   class is capable of segmenting long values by repeated suffixing
   (see ).
  
+
+ 
+  leafType is typically the same as
+  attType.  For the reasons of backward
+  compatibility, method config can
+  leave leafType uninitialized; that would
+  give the same effect as setting leafType equal
+  to attType.  When attType
+  and leafType are different, then optional
+  method compress must be provided.
+  Method compress is responsible
+  for transformation of datums to be indexed from attType
+  to leafType.

Re: compress method for spgist - 2

[Alexander Korotkov]

On Wed, Dec 6, 2017 at 6:08 PM, Nikita Glukhov 
wrote:

> On 05.12.2017 23:59, Alexander Korotkov wrote:
>
> On Tue, Dec 5, 2017 at 1:14 PM, Darafei Praliaskouski 
> wrote:
>
>> The following review has been posted through the commitfest application:
>> make installcheck-world:  not tested
>> Implements feature:   not tested
>> Spec compliant:   not tested
>> Documentation:tested, passed
>>
>> I've read the updated patch and see my concerns addressed.
>>
>> I'm looking forward to SP-GiST compress method support, as it will allow
>> usage of SP-GiST index infrastructure for PostGIS.
>>
>> The new status of this patch is: Ready for Committer
>>
>
> I went trough this patch.  I found documentation changes to be not
> sufficient.  And I've made some improvements.
>
> In particular, I didn't understand why is reconstructedValue claimed to be
> of spgConfigOut.leafType while it should be of spgConfigIn.attType both
> from general logic and code.  I've fixed that.  Nikita, correct me if I'm
> wrong.
>
>
> I think we are reconstructing a leaf datum, so documentation was correct
> but the code in spgWalk() and freeScanStackEntry() wrongly used attType
> instead of attLeafType. Fixed patch is attached.
>

Reconstructed datum is used for index-only scan.  Thus, it should be
original indexed datum of attType, unless we have decompress method and
pass reconstructed datum through it.

> Also, I wonder should we check for existence of compress method when
> attType and leafType are not the same in spgvalidate() function?  We don't
> do this for now.
>
> I've added compress method existence check to spgvalidate().
>

It would be nice to evade double calling of config method.  Possible option
could be to memorize difference between attribute type and leaf type in
high bit of functionset, which is guaranteed to be free.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: compress method for spgist - 2

[Nikita Glukhov]

On 05.12.2017 23:59, Alexander Korotkov wrote:

On Tue, Dec 5, 2017 at 1:14 PM, Darafei Praliaskouski > wrote:


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

I've read the updated patch and see my concerns addressed.

I'm looking forward to SP-GiST compress method support, as it will
allow usage of SP-GiST index infrastructure for PostGIS.

The new status of this patch is: Ready for Committer


I went trough this patch.  I found documentation changes to be not 
sufficient.  And I've made some improvements.


In particular, I didn't understand why is reconstructedValue claimed 
to be of spgConfigOut.leafType while it should be of 
spgConfigIn.attType both from general logic and code.  I've fixed 
that.  Nikita, correct me if I'm wrong.


I think we are reconstructing a leaf datum, so documentation was correct 
but the code in spgWalk() and freeScanStackEntry() wrongly used attType 
instead of attLeafType. Fixed patch is attached.


Also, I wonder should we check for existence of compress method when 
attType and leafType are not the same in spgvalidate() function?  We 
don't do this for now.

I've added compress method existence check to spgvalidate().


0002-spgist-polygon-8.patch is OK for me so soon.


--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
diff --git a/doc/src/sgml/spgist.sgml b/doc/src/sgml/spgist.sgml
index 139c8ed..b4a8be4 100644
--- a/doc/src/sgml/spgist.sgml
+++ b/doc/src/sgml/spgist.sgml
@@ -240,20 +240,22 @@
 
  
   There are five user-defined methods that an index operator class for
-  SP-GiST must provide.  All five follow the convention
-  of accepting two internal arguments, the first of which is a
-  pointer to a C struct containing input values for the support method,
-  while the second argument is a pointer to a C struct where output values
-  must be placed.  Four of the methods just return void, since
-  all their results appear in the output struct; but
+  SP-GiST must provide, and one is optional.  All five
+  mandatory methods follow the convention of accepting two internal
+  arguments, the first of which is a pointer to a C struct containing input
+  values for the support method, while the second argument is a pointer to a
+  C struct where output values must be placed.  Four of the mandatory methods just
+  return void, since all their results appear in the output struct; but
   leaf_consistent additionally returns a boolean result.
   The methods must not modify any fields of their input structs.  In all
   cases, the output struct is initialized to zeroes before calling the
-  user-defined method.
+  user-defined method.  Optional sixth method compress
+  accepts datum to be indexed as the only argument and returns value suitable
+  for physical storage in leaf tuple.
  
 
  
-  The five user-defined methods are:
+  The five mandatory user-defined methods are:
  
 
  
@@ -283,6 +285,7 @@ typedef struct spgConfigOut
 {
 Oid prefixType; /* Data type of inner-tuple prefixes */
 Oid labelType;  /* Data type of inner-tuple node labels */
+Oid leafType;   /* Data type of leaf-tuple values */
 boolcanReturnData;  /* Opclass can reconstruct original data */
 boollongValuesOK;   /* Opclass can cope with values > 1 page */
 } spgConfigOut;
@@ -305,6 +308,22 @@ typedef struct spgConfigOut
   class is capable of segmenting long values by repeated suffixing
   (see ).
  
+
+ 
+  leafType is typically the same as
+  attType.  For the reasons of backward
+  compatibility, method config can
+  leave leafType uninitialized; that would
+  give the same effect as setting leafType equal
+  to attType.  When attType
+  and leafType are different, then optional
+  method compress must be provided.
+  Method compress is responsible
+  for transformation of datums to be indexed from attType
+  to leafType.
+  Note: both consistent functions will get scankeys
+  unchanged, without transformation using compress.
+ 
  
 
 
@@ -380,10 +399,16 @@ typedef struct spgChooseOut
 } spgChooseOut;
 
 
-   datum is the original datum that was to be inserted
-   into the index.
-   leafDatum is initially the same as
-   datum, but can change at lower levels of the tree
+   datum is the original datum of
+   spgConfigIn.attType
+   type that was to be inserted into the index.
+   leafDatum is a value of
+   spgConfigOut.leafType
+   type which is initially an result of method
+   compress applied to datum
+   when method compress is provided, or same value as
+   datum otherwise.
+   leafDatum can change at lower 

Re: compress method for spgist - 2

[Alexander Korotkov]

On Tue, Dec 5, 2017 at 1:14 PM, Darafei Praliaskouski  wrote:

> The following review has been posted through the commitfest application:
> make installcheck-world:  not tested
> Implements feature:   not tested
> Spec compliant:   not tested
> Documentation:tested, passed
>
> I've read the updated patch and see my concerns addressed.
>
> I'm looking forward to SP-GiST compress method support, as it will allow
> usage of SP-GiST index infrastructure for PostGIS.
>
> The new status of this patch is: Ready for Committer
>

I went trough this patch.  I found documentation changes to be not
sufficient.  And I've made some improvements.

In particular, I didn't understand why is reconstructedValue claimed to be
of spgConfigOut.leafType while it should be of spgConfigIn.attType both
from general logic and code.  I've fixed that.  Nikita, correct me if I'm
wrong.

Also, I wonder should we check for existence of compress method when
attType and leafType are not the same in spgvalidate() function?  We don't
do this for now.

0002-spgist-polygon-8.patch is OK for me so soon.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


0001-spgist-compress-method-9.patch
Description: Binary data

Re: compress method for spgist - 2

[Darafei Praliaskouski]

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

I've read the updated patch and see my concerns addressed.

I'm looking forward to SP-GiST compress method support, as it will allow usage 
of SP-GiST index infrastructure for PostGIS.

The new status of this patch is: Ready for Committer

Re: [HACKERS] compress method for spgist - 2

[Nikita Glukhov]

On 30.11.2017 05:31, Michael Paquier wrote:


I can see as well that the patches posted at the beginning of the
thread got reviews but that those did not get answered. The set of
patches also have conflicts with HEAD so they need a rebase. For those
reasons I am marking this entry as returned with feedback.


Rebased patches are attached.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
>From 1c251ff129f8e9f33d14df547d97ba549b109648 Mon Sep 17 00:00:00 2001
From: Nikita Glukhov 
Date: Tue, 5 Dec 2017 02:38:50 +0300
Subject: [PATCH 1/2] spgist-compress-method-8

---
 doc/src/sgml/spgist.sgml| 54 ++---
 src/backend/access/spgist/spgdoinsert.c | 44 +++
 src/backend/access/spgist/spgutils.c| 23 --
 src/backend/access/spgist/spgvalidate.c | 24 ++-
 src/include/access/spgist.h |  5 ++-
 src/include/access/spgist_private.h |  8 +++--
 6 files changed, 127 insertions(+), 31 deletions(-)

diff --git a/doc/src/sgml/spgist.sgml b/doc/src/sgml/spgist.sgml
index 139c8ed..55c1b06 100644
--- a/doc/src/sgml/spgist.sgml
+++ b/doc/src/sgml/spgist.sgml
@@ -240,20 +240,21 @@
 
  
   There are five user-defined methods that an index operator class for
-  SP-GiST must provide.  All five follow the convention
-  of accepting two internal arguments, the first of which is a
-  pointer to a C struct containing input values for the support method,
-  while the second argument is a pointer to a C struct where output values
-  must be placed.  Four of the methods just return void, since
-  all their results appear in the output struct; but
+  SP-GiST must provide and one optional. All five mandatory
+  methos follow the convention of accepting two internal arguments,
+  the first of which is a pointer to a C struct containing input values for 
+  the support method, while the second argument is a pointer to a C struct 
+  where output values must be placed.  Four of the methods just return 
+  void, since all their results appear in the output struct; but
   leaf_consistent additionally returns a boolean result.
   The methods must not modify any fields of their input structs.  In all
   cases, the output struct is initialized to zeroes before calling the
-  user-defined method.
+  user-defined method. Optional method compress accepts
+  datum to be indexed and returns values which actually will be indexed.
  
 
  
-  The five user-defined methods are:
+  The five mandatory user-defined methods are:
  
 
  
@@ -283,6 +284,7 @@ typedef struct spgConfigOut
 {
 Oid prefixType; /* Data type of inner-tuple prefixes */
 Oid labelType;  /* Data type of inner-tuple node labels */
+Oid leafType;   /* Data type of leaf */
 boolcanReturnData;  /* Opclass can reconstruct original data */
 boollongValuesOK;   /* Opclass can cope with values > 1 page */
 } spgConfigOut;
@@ -303,7 +305,15 @@ typedef struct spgConfigOut
   longValuesOK should be set true only when the
   attType is of variable length and the operator
   class is capable of segmenting long values by repeated suffixing
-  (see ).
+  (see ). leafType
+  usually has the same value as attType but if
+  it's different then optional method  compress
+  should be provided. Method  compress is responsible
+  for transformation from attType to 
+  leafType. In this case all other function
+  should accept leafType values. Note: both
+  consistent functions will get scankeys
+  unchanged, without compress transformation.
  
  
 
@@ -624,7 +634,8 @@ typedef struct spgInnerConsistentOut
reconstructedValue is the value reconstructed for the
parent tuple; it is (Datum) 0 at the root level or if the
inner_consistent function did not provide a value at the
-   parent level.
+   parent level. reconstructedValue should be always a
+   spgConfigOut.leafType type.
traversalValue is a pointer to any traverse data
passed down from the previous call of inner_consistent
on the parent index tuple, or NULL at the root level.
@@ -730,7 +741,8 @@ typedef struct spgLeafConsistentOut
reconstructedValue is the value reconstructed for the
parent tuple; it is (Datum) 0 at the root level or if the
inner_consistent function did not provide a value at the
-   parent level.
+   parent level. reconstructedValue should be always a
+   spgConfigOut.leafType type. 
traversalValue is a pointer to any traverse data
passed down from the previous call of inner_consistent
on the parent index tuple, or NULL at the root level.
@@ -757,6 +769,26 @@ typedef struct spgLeafConsistentOut
 

 
+ 
+  The optional user-defined method is:
+ 
+
+ 
+
+ Datum compress(Datum in)
+ 
+  
+   Converts the data

Re: [HACKERS] compress method for spgist - 2

[Michael Paquier]

On Fri, Sep 22, 2017 at 9:03 AM, Nikita Glukhov  wrote:
> Should I start a separate thread for this issue and add patches to
> commitfest?

Yes, please. It would be nice if you could spawn a separate thread for
what looks like a bug, and separate topics should have their own
thread. This will attract more attention from other hackers as this is
unrelated to this thread. Adding an entry in the CF app under the
category "Bug Fix" also avoids losing any items worth fixing.

I can see as well that the patches posted at the beginning of the
thread got reviews but that those did not get answered. The set of
patches also have conflicts with HEAD so they need a rebase. For those
reasons I am marking this entry as returned with feedback.
-- 
Michael