Re: [HACKERS] Allow GiST opcalsses without compress\decompres functions

[Andrey Borodin]

> 22 окт. 2017 г., в 21:21, Tom Lane  написал(а):
> 
> Andrey Borodin  writes:
>> I was looking for a way to correctly drop compress\decompress functions from 
>> opclasses.
> 
> Making a new opclass seems like a pretty grotty answer; it won't
> help existing installations.

Unfortunately in cube's case that's the only option: cube allows 
100-dimensional cubes, while 94-dimensional cubes could be toasted (according 
to code).

> I think what you need is to undo opclasscmds.c's decision that the
> dependencies should be INTERNAL.  I tried this:

Thanks Tom, that's exactly what I need. I'll push patches with this to November 
commitfest.

Best regards, Andrey Borodin.

-- 
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] Allow GiST opcalsses without compress\decompres functions

[Tom Lane]

Andrey Borodin  writes:
> I was looking for a way to correctly drop compress\decompress functions from 
> opclasses.

Making a new opclass seems like a pretty grotty answer; it won't
help existing installations.

I think what you need is to undo opclasscmds.c's decision that the
dependencies should be INTERNAL.  I tried this:

regression=# ALTER OPERATOR FAMILY gist_seg_ops USING gist drop function 3 
(seg);
ERROR:  cannot drop function 3 (seg, seg) of operator family gist_seg_ops for 
access method gist: gseg_compress(internal) because operator class gist_seg_ops 
for access method gist requires it

regression=# update pg_depend set deptype = 'a' where classid = 
'pg_amproc'::regclass and objid = (select objid from pg_depend where classid = 
'pg_amproc'::regclass and refclassid = 'pg_proc'::regclass and refobjid = 
'gseg_compress(internal)'::regprocedure) and refclassid = 
'pg_opclass'::regclass and deptype = 'i';
UPDATE 1
regression=# ALTER OPERATOR FAMILY gist_seg_ops USING gist drop function 3 
(seg);
ALTER OPERATOR FAMILY

For safety, that update requires a bunch more pg_catalog
schema-qualification than I bothered with, but you get the idea.

I'm not sure if needing this hack indicates that we need more ALTER
OPERATOR CLASS/FAMILY syntax to provide a less hacky way of solving
the problem.  The idea of removing core entries of an opclass has
never come up before, and I'm not sure it would ever come up again.
If we come across another use-case maybe then would be the time to
fix it more cleanly.

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] Allow GiST opcalsses without compress\decompres functions

[Andrey Borodin]

Hi, Tom!
> 20 сент. 2017 г., в 8:38, Tom Lane  написал(а):
> 
> Andrey Borodin  writes:
>> [ 0001-Allow-uncompressed-GiST-4.patch ]
> 
> Pushed, with a bit more work on the documentation and some minor
> cosmetic changes.
> 
> I did not like the fact that the new code paths added by the patch
> were untested, so I went ahead and removed the no-longer-needed
> no-op functions in the core GiST opclasses.  There's still room
> to improve the contrib opclasses, but that's much more tedious
> (you need to write an extension update script) so I didn't feel
> like messing with those now.

I was looking for a way to correctly drop compress\decompress functions from 
opclasses.
There are two cases: when the functions do something unnecessary (like 
TOASTing) and when they are just no-ops.

First case occurs, for example, in cube. Please see patch attached. There is no 
DROP DEFAULT and I'm doing
UPDATE pg_opclass SET opcdefault = FALSE WHERE opcname = 'gist_cube_ops';
I'm not sure it is correct way, but I haven't found any other workaround. Then 
I just create new default opclass without compress\decompress.

Second case, for example in seg. I have tried 
alter operator family gist_seg_ops using gist drop function 3 (seg);
It does not work: dependency between opclass and method is DEPENDENCY_INTERNAL 
and command is refused.
Should I create new opclass as with cube or, may be, I can delete functions 
from system catalog?

Best regards, Andrey Borodin.



0001-Create-cube-opclass-without-compress-decompress.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] Allow GiST opcalsses without compress\decompres functions

[Alexander Korotkov]

On Wed, Sep 20, 2017 at 5:02 PM, Tom Lane  wrote:

> Alexander Korotkov  writes:
> > On Wed, Sep 20, 2017 at 4:25 PM, Tom Lane  wrote:
> >> Yes.  We don't allow out-of-line values, but we do allow compressed and
> >> short-header values.
>
> > BTW, this comment looks still invalid for me...
>
> >> #define SIGLENINT  4 /* >122 => key will *toast*, so very slow!!! */
>
> I haven't done the math to see if 122 is exactly the right value,
> but at some point index_form_tuple would decide that the tuple was
> uncomfortably big and try to compress it.  So the comment is right
> in general terms.
>

If comment means toast as compression, not out-of-line storage then it's
true.
However, it would be good to rephrase this comment to clarify that.

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

Re: [HACKERS] Allow GiST opcalsses without compress\decompres functions

[Tom Lane]

Alexander Korotkov  writes:
> On Wed, Sep 20, 2017 at 4:25 PM, Tom Lane  wrote:
>> Yes.  We don't allow out-of-line values, but we do allow compressed and
>> short-header values.

> BTW, this comment looks still invalid for me...

>> #define SIGLENINT  4 /* >122 => key will *toast*, so very slow!!! */

I haven't done the math to see if 122 is exactly the right value,
but at some point index_form_tuple would decide that the tuple was
uncomfortably big and try to compress it.  So the comment is right
in general terms.

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] Allow GiST opcalsses without compress\decompres functions

[Alexander Korotkov]

On Wed, Sep 20, 2017 at 4:25 PM, Tom Lane  wrote:

> Andrey Borodin  writes:
> > You mentioned that probably there cannot be TOASTed values in the index.
> > I need to settle this question in more deterministic way.
> > Can you point where to look at the code or who to ask:
> > Can there be TOASTed values in index tuples?
>
> Yes.  We don't allow out-of-line values, but we do allow compressed and
> short-header values.
>
> If you don't believe me, look at index_form_tuple().
>

OK.
So GiST opclass should either implement decompress method with detoasting
or detoast every input key in all other methods.

BTW, this comment looks still invalid for me...

> #define SIGLENINT  4 /* >122 => key will *toast*, so very slow!!! */


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

Re: [HACKERS] Allow GiST opcalsses without compress\decompres functions

[Tom Lane]

Andrey Borodin  writes:
> You mentioned that probably there cannot be TOASTed values in the index. 
> I need to settle this question in more deterministic way.
> Can you point where to look at the code or who to ask:
> Can there be TOASTed values in index tuples?

Yes.  We don't allow out-of-line values, but we do allow compressed and
short-header values.

If you don't believe me, look at index_form_tuple().

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] Allow GiST opcalsses without compress\decompres functions

[Alexander Korotkov]

On Wed, Sep 20, 2017 at 12:43 PM, Andrey Borodin 
wrote:

> Hello Tom! Thanks for committing the patch!
>
> > 20 сент. 2017 г., в 8:38, Tom Lane  написал(а):
> >
> > Andrey Borodin  writes:
> >> [ 0001-Allow-uncompressed-GiST-4.patch ]
> >
> > ... There's still room
> > to improve the contrib opclasses
>
> I have one important question regarding compress\decompres functions.
>
> You mentioned that probably there cannot be TOASTed values in the index.
> I need to settle this question in more deterministic way.
> Can you point where to look at the code or who to ask:
> Can there be TOASTed values in index tuples?
>
> If answer is NO, we can get rid of much more useless code.
>

And get rid of misleading comments too.  For instance, this comment in
hstore_gist.c seems misleading for me.

#define SIGLENINT  4 /* >122 => key will *toast*, so very slow!!! */


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

Re: [HACKERS] Allow GiST opcalsses without compress\decompres functions

[Andrey Borodin]

Hello Tom! Thanks for committing the patch!

> 20 сент. 2017 г., в 8:38, Tom Lane  написал(а):
> 
> Andrey Borodin  writes:
>> [ 0001-Allow-uncompressed-GiST-4.patch ]
> 
> ... There's still room
> to improve the contrib opclasses

I have one important question regarding compress\decompres functions.

You mentioned that probably there cannot be TOASTed values in the index. 
I need to settle this question in more deterministic way.
Can you point where to look at the code or who to ask:
Can there be TOASTed values in index tuples?

If answer is NO, we can get rid of much more useless code.

Best regards, Andrey Borodin.

-- 
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] Allow GiST opcalsses without compress\decompres functions

[Tom Lane]

Andrey Borodin  writes:
> [ 0001-Allow-uncompressed-GiST-4.patch ]

Pushed, with a bit more work on the documentation and some minor
cosmetic changes.

I did not like the fact that the new code paths added by the patch
were untested, so I went ahead and removed the no-longer-needed
no-op functions in the core GiST opclasses.  There's still room
to improve the contrib opclasses, but that's much more tedious
(you need to write an extension update script) so I didn't feel
like messing with those now.

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] Allow GiST opcalsses without compress\decompres functions

[Alexander Korotkov]

On Fri, Sep 15, 2017 at 3:36 PM, Andrey Borodin 
wrote:

> > 14 сент. 2017 г., в 18:42, Alexander Korotkov 
> написал(а):
> > It would be good if someone would write patch for removing useless
> compress/decompress methods from builtin and contrib GiST opclasses.
> Especially when it gives benefit in IndexOnlyScan enabling.
> If the patch will be accepted, I'll either do this myself for commitfest
> at November or charge some students from Yandex Data School to do this
> (they earn some points in algorithms course for contributing to OSS).
>

Great!  I'm looking forward see their patches on commitfest!

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

Re: [HACKERS] Allow GiST opcalsses without compress\decompres functions

[Andrey Borodin]

Dmitry and Alexander, thank you for looking into the patch!

> 14 сент. 2017 г., в 18:42, Alexander Korotkov  
> написал(а):
> It would be good if someone would write patch for removing useless 
> compress/decompress methods from builtin and contrib GiST opclasses.  
> Especially when it gives benefit in IndexOnlyScan enabling.
If the patch will be accepted, I'll either do this myself for commitfest at 
November or charge some students from Yandex Data School to do this (they earn 
some points in algorithms course for contributing to OSS).

> BTW, this change in the patch look suspicious for me
> 
> We are typically evade changing formatting in fragments of codes not directly 
> touched by the patch.

Thanks for spotting this out. Here's fixed version.

Best regards, Andrey Borodin.



0001-Allow-uncompressed-GiST-4.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] Allow GiST opcalsses without compress\decompres functions

[Alexander Korotkov]

On Thu, Sep 14, 2017 at 2:20 PM, Dmitriy Sarafannikov <
dsarafanni...@yandex.ru> wrote:

> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:   tested, passed
> Spec compliant:   tested, passed
> Documentation:tested, passed
>
> This is simple and intuitive patch. Code looks pretty clear and well
> documented.
>
> I think it is good idea to decrease overhead on calling fake
> compressing/decomressing
> functions. This eliminates the need to implement the fetch function in
> such cases.
>
> I also tried to disable compress and decompress functions in contrib/cube:
> diff --git a/contrib/cube/cube--1.2.sql b/contrib/cube/cube--1.2.sql
> index dea2614888..26301b71d7 100644
> --- a/contrib/cube/cube--1.2.sql
> +++ b/contrib/cube/cube--1.2.sql
> @@ -370,8 +370,6 @@ CREATE OPERATOR CLASS gist_cube_ops
>
> FUNCTION1   g_cube_consistent (internal, cube,
> smallint, oid, internal),
> FUNCTION2   g_cube_union (internal, internal),
> -   FUNCTION3   g_cube_compress (internal),
> -   FUNCTION4   g_cube_decompress (internal),
> FUNCTION5   g_cube_penalty (internal, internal,
> internal),
> FUNCTION6   g_cube_picksplit (internal, internal),
> FUNCTION7   g_cube_same (cube, cube, internal),
>
> And it is enables IndexOnlyScan, this is expected behaviour.
> + -- test explain
> + set enable_seqscan to 'off';
> + set enable_bitmapscan to 'off';
> + select count(*) from test_cube where c && '(3000,1000),(0,0)';
> +  count
> + ---
> +  5
> + (1 row)
> +
> + explain analyze select c from test_cube where c && '(3000,1000),(0,0)';
> +QUERY PLAN
> + 
> 
> +  Index Only Scan using test_cube_ix on test_cube  (cost=0.15..56.43
> rows=16 width=32) (actual time=0.015..0.018 rows=5 loops=1)
> +Index Cond: (c && '(3000, 1000),(0, 0)'::cube)
> +Heap Fetches: 5
> +  Planning time: 0.038 ms
> +  Execution time: 0.032 ms
> + (5 rows)
>
> The new status of this patch is: Ready for Committer


It would be good if someone would write patch for removing useless
compress/decompress methods from builtin and contrib GiST opclasses.
Especially when it gives benefit in IndexOnlyScan enabling.

BTW, this change in the patch look suspicious for me.

> @@ -913,7 +931,6 @@ gistproperty(Oid index_oid, int attno,
>   ReleaseSysCache(tuple);
>
>   /* Now look up the opclass family and input datatype. */
> -
>   tuple = SearchSysCache1(CLAOID, ObjectIdGetDatum(opclass));
>   if (!HeapTupleIsValid(tuple))
>   {

We are typically evade changing formatting in fragments of codes not
directly touched by the patch.

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

Re: [HACKERS] Allow GiST opcalsses without compress\decompres functions

[Dmitriy Sarafannikov]

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

This is simple and intuitive patch. Code looks pretty clear and well documented.

I think it is good idea to decrease overhead on calling fake 
compressing/decomressing
functions. This eliminates the need to implement the fetch function in such 
cases.

I also tried to disable compress and decompress functions in contrib/cube:
diff --git a/contrib/cube/cube--1.2.sql b/contrib/cube/cube--1.2.sql
index dea2614888..26301b71d7 100644
--- a/contrib/cube/cube--1.2.sql
+++ b/contrib/cube/cube--1.2.sql
@@ -370,8 +370,6 @@ CREATE OPERATOR CLASS gist_cube_ops

FUNCTION1   g_cube_consistent (internal, cube, smallint, 
oid, internal),
FUNCTION2   g_cube_union (internal, internal),
-   FUNCTION3   g_cube_compress (internal),
-   FUNCTION4   g_cube_decompress (internal),
FUNCTION5   g_cube_penalty (internal, internal, internal),
FUNCTION6   g_cube_picksplit (internal, internal),
FUNCTION7   g_cube_same (cube, cube, internal),

And it is enables IndexOnlyScan, this is expected behaviour.
+ -- test explain
+ set enable_seqscan to 'off';
+ set enable_bitmapscan to 'off';
+ select count(*) from test_cube where c && '(3000,1000),(0,0)';
+  count
+ ---
+  5
+ (1 row)
+
+ explain analyze select c from test_cube where c && '(3000,1000),(0,0)';
+QUERY PLAN
+ 

+  Index Only Scan using test_cube_ix on test_cube  (cost=0.15..56.43 rows=16 
width=32) (actual time=0.015..0.018 rows=5 loops=1)
+Index Cond: (c && '(3000, 1000),(0, 0)'::cube)
+Heap Fetches: 5
+  Planning time: 0.038 ms
+  Execution time: 0.032 ms
+ (5 rows)

The new status of this patch is: Ready for Committer

-- 
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] Allow GiST opcalsses without compress\decompres functions

[Andrey Borodin]

> 11 сент. 2017 г., в 12:57, Dmitriy Sarafannikov  
> написал(а):
> Hi Andrew! Thanks for the patch, but patch 
> 0001-allow-uncompressed-Gist-2.patch no longer applies on current master 
> branch.
> Please could you rebase it?
Sure, see attachment. Thanks for looking into the patch!

Best regards, Andrey Borodin. 


0001-Allow-uncompressed-GiST-3.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] Allow GiST opcalsses without compress\decompres functions

[Dmitriy Sarafannikov]

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

Hi Andrew! Thanks for the patch, but patch 0001-allow-uncompressed-Gist-2.patch 
no longer applies on current master branch.
Please could you rebase it?

Some info from git apply command:
Checking patch doc/src/sgml/gist.sgml...
Checking patch src/backend/access/gist/gist.c...
Checking patch src/backend/access/gist/gistget.c...
Checking patch src/backend/access/gist/gistutil.c...
error: while searching for:
GISTENTRY  *dep;

gistentryinit(*e, k, r, pg, o, l);
dep = (GISTENTRY *)

DatumGetPointer(FunctionCall1Coll(>decompressFn[nkey],

   giststate->supportCollation[nkey],

error: patch failed: src/backend/access/gist/gistutil.c:550
error: while searching for:

gistentryinit(centry, attdata[i], r, NULL, 
(OffsetNumber) 0,
  isleaf);
cep = (GISTENTRY *)

DatumGetPointer(FunctionCall1Coll(>compressFn[i],

  giststate->supportCollation[i],

  PointerGetDatum()));
compatt[i] = cep->key;
}
}

error: patch failed: src/backend/access/gist/gistutil.c:585
Hunk #3 succeeded at 648 (offset -7 lines).
Hunk #4 succeeded at 924 (offset -7 lines).
Hunk #5 succeeded at 944 (offset -7 lines).
Checking patch src/backend/access/gist/gistvalidate.c...
Applied patch doc/src/sgml/gist.sgml cleanly.
Applied patch src/backend/access/gist/gist.c cleanly.
Applied patch src/backend/access/gist/gistget.c cleanly.
Applying patch src/backend/access/gist/gistutil.c with 2 rejects...
Rejected hunk #1.
Rejected hunk #2.
Hunk #3 applied cleanly.
Hunk #4 applied cleanly.
Hunk #5 applied cleanly.
Applied patch src/backend/access/gist/gistvalidate.c cleanly.

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] Allow GiST opcalsses without compress\decompres functions

[Andrew Borodin]

2017-05-29 0:00 GMT+05:00 Tom Lane :
> But the opclass's consistent function might expect to be always given an
> untoasted input, in which case you'd need the decompress function to fix
> that up.

In cube data is detoasted at least 4 times before going to
g_cube_internal_consistent(), including once in g_cube_consistent()
But I'll address that in another patch.

Here's new version of the patch, with:
1. Corrected docs
2. Any combination of dompress\decompress\fetch is allowed
3. (Existence of fetch) or (absence of compress) leads to IOS

The last point seems counterintuitive, but I do not see anything wrong.

Best regards, Andrey Borodin, Octonica.


0001-Allow-uncompressed-GiST-2.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] Allow GiST opcalsses without compress\decompres functions

[Tom Lane]

Andrew Borodin  writes:
> I'm not expert in toasting, cube's compress does nothing while cube
> decomress does detoasting. Is this for reason?

The original input datum could be toasted --- at least to the extent of
being compressed in-line or having short header; I do not think we allow
out-of-line storage in an index.  This is OK for storage within the index,
and it would also be OK for an IOS to return the value in that form.
But the opclass's consistent function might expect to be always given an
untoasted input, in which case you'd need the decompress function to fix
that up.

Mind you, I'm not saying that this would represent good design;
datatype-specific functions that expect somebody else to have
detoasted their input seem pretty fragile.  But it might be how
cube's GIST support works.

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] Allow GiST opcalsses without compress\decompres functions

[Andrew Borodin]

28 мая 2017 г. 11:15 PM пользователь "Tom Lane"  написал:

Andrew Borodin  writes:
> 2017-05-28 22:22 GMT+05:00 Tom Lane :
>> 2. If compress/decompress are omitted, then we could support index-only
>> scans all the time, that is a no-op fetch function would work.  The
>> patch should cover that interaction too.

> I do not think so. Decompress have to get att to the state where
> consistency function can understand it. In theory. I've checked like a
> dozen of types and have not found places where fetch should differ
> from decompress.

But if compress is omitted, then we know the in-index representation
is the same as the original.  Therefore the fetch function would always
be a no-op and we can implement IOS whether or not the opclass designer
bothered to supply one.

regards, tom lane

True. If there is no code to "hash" original value, it is not hashed, it's
whole...

I'm not expert in toasting, cube's compress does nothing while cube
decomress does detoasting. Is this for reason?

Best regards, Andrey Borodin.

Re: [HACKERS] Allow GiST opcalsses without compress\decompres functions

[Tom Lane]

Andrew Borodin  writes:
> 2017-05-28 22:22 GMT+05:00 Tom Lane :
>> 2. If compress/decompress are omitted, then we could support index-only
>> scans all the time, that is a no-op fetch function would work.  The
>> patch should cover that interaction too.

> I do not think so. Decompress have to get att to the state where
> consistency function can understand it. In theory. I've checked like a
> dozen of types and have not found places where fetch should differ
> from decompress.

But if compress is omitted, then we know the in-index representation
is the same as the original.  Therefore the fetch function would always
be a no-op and we can implement IOS whether or not the opclass designer
bothered to supply one.

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] Allow GiST opcalsses without compress\decompres functions

[Andrew Borodin]

Thanks, Tom!

2017-05-28 22:22 GMT+05:00 Tom Lane :
> Andrew Borodin  writes:
>> Maybe we should make compress\decompress functions optional?
>
> 1. You'll need to adjust the documentation (gist.sgml) not just the code.
Sure, I'll check out where compression is mentioned and update docs.

> 2. If compress/decompress are omitted, then we could support index-only
> scans all the time, that is a no-op fetch function would work.  The
> patch should cover that interaction too.
I do not think so. Decompress have to get att to the state where
consistency function can understand it. In theory. I've checked like a
dozen of types and have not found places where fetch should differ
from decompress.

> 3. Personally I wouldn't bother with the separate compressed[] flags,
> just look at OidIsValid(giststate->compressFn[i].fn_oid).
That would save few bytes, but make compress\decompress code slightly
harder to read. Though some comments will help.

> 4. I don't think you thought very carefully about the combinations
> where only one of the two functions is supplied.  I can definitely
> see that compress + no decompress could be sensible.  Less sure
> about the other case, but why not allow it?  We could just say that
> an omitted function is taken to represent a no-op transformation.
Well, I thought only about the exclusion of this situation(when only
one function is supplied). But, Indeed, I've seen many opclasses where
compress was doing a work (like simplifying the data) while decompress
is just NOP.
I'll think more about it.
decompress-only opclass seems like having zero sense in adjusting
tuple on internal page. We decompress att, update it, then store it
back. Then, once upon a time, decompress it again. Looks odd. But this
does not look like a place where opcalss developer can introduce new
bugs, so I do not see reasons to forbid having only decompress
function.

> Please add this to the next commitfest.

OK.
Thank you for your comments. I'll address them and put a patch to the
commitfest.

Best regards, Andrey Borodin.


-- 
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] Allow GiST opcalsses without compress\decompres functions

[Tom Lane]

Andrew Borodin  writes:
> Maybe we should make compress\decompress functions optional?

1. You'll need to adjust the documentation (gist.sgml) not just the code.

2. If compress/decompress are omitted, then we could support index-only
scans all the time, that is a no-op fetch function would work.  The
patch should cover that interaction too.

3. Personally I wouldn't bother with the separate compressed[] flags,
just look at OidIsValid(giststate->compressFn[i].fn_oid).

4. I don't think you thought very carefully about the combinations
where only one of the two functions is supplied.  I can definitely
see that compress + no decompress could be sensible.  Less sure
about the other case, but why not allow it?  We could just say that
an omitted function is taken to represent a no-op transformation.

Please add this to the next commitfest.

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] Allow GiST opcalsses without compress\decompres functions

[Andrew Borodin]

Hi, hackers!

Currently, GiST stores each attribute in a compressed form.
That is, each time attribute is written it's calling compress
function, and when the attribute is accessed the decompress functions
is called.
Some types can't get any advantage out of this technique since the
context of one value is not enough for seeding effective compression
algorithm.
And we have some of the compress functions like this
Datum
gist_box_compress(PG_FUNCTION_ARGS)
{
PG_RETURN_POINTER(PG_GETARG_POINTER(0));
}

https://github.com/postgres/postgres/blob/master/src/backend/access/gist/gistproc.c#L195
https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/rangetypes_gist.c#L221
https://github.com/postgres/postgres/blob/master/contrib/seg/seg.c#L255
https://github.com/postgres/postgres/blob/master/contrib/cube/cube.c#L384

Maybe we should make compress\decompress functions optional?
Also, this brings some bit of performance.
For the attached test I observe 6% faster GiST build and 4% faster scans.
Not a big deal, but something. How do you think?

In some cases, there are strange things in the code of
compress\decompress. E.g. cube's decompress function is detoasting
entry twice, to be very sure :)



Best regards, Andrey Borodin, Octonica.
CREATE EXTENSION IF NOT EXISTS CUBE ;

DROP TABLE IF EXISTS TESTTABLE ;
DROP OPERATOR CLASS IF EXISTS gist_cube_ops_nocompress USING GIST;

CREATE OPERATOR CLASS gist_cube_ops_nocompress
FOR TYPE CUBE USING GIST AS
OPERATOR3   && ,
OPERATOR6   = ,
OPERATOR7   @> ,
OPERATOR8   <@ ,
OPERATOR13  @ ,
OPERATOR14  ~ ,

FUNCTION1   g_cube_consistent (INTERNAL, CUBE, SMALLINT, 
OID, INTERNAL),
FUNCTION2   g_cube_union (INTERNAL, INTERNAL),
--we do not have functions 3 and for (compress and decompress)
FUNCTION5   g_cube_penalty (INTERNAL, INTERNAL, INTERNAL),
FUNCTION6   g_cube_picksplit (INTERNAL, INTERNAL),
FUNCTION7   g_cube_same (CUBE, CUBE, INTERNAL),
FUNCTION8   g_cube_distance (INTERNAL, CUBE, SMALLINT, OID, 
INTERNAL),
FUNCTION9   g_cube_decompress (INTERNAL);--fetch function, 
not for compression



BEGIN;
SELECT SETSEED(0.5);

CREATE TABLE TESTTABLE AS SELECT CUBE(RANDOM()) C FROM 
GENERATE_SERIES(1,50) I;

\timing

--testing time to create compressed index
CREATE INDEX COMPRESSED ON TESTTABLE USING GIST(C gist_cube_ops);
--testing time to create uncompressed index
CREATE INDEX UNCOMPRESSED ON TESTTABLE USING GIST(C gist_cube_ops_nocompress);

\timing
set enable_bitmapscan = false;

UPDATE PG_INDEX SET INDISVALID = FALSE WHERE INDEXRELID = 
'UNCOMPRESSED'::REGCLASS;
UPDATE PG_INDEX SET INDISVALID = TRUE WHERE INDEXRELID = 'COMPRESSED'::REGCLASS;

EXPLAIN ANALYZE select (SELECT COUNT(*) FROM TESTTABLE WHERE C <@ CUBE(-b,1)) 
from generate_series(1,50) b;
EXPLAIN ANALYZE select (SELECT COUNT(*) FROM TESTTABLE WHERE C <@ CUBE(-b,1)) 
from generate_series(1,50) b;

UPDATE PG_INDEX SET INDISVALID = TRUE WHERE INDEXRELID = 
'UNCOMPRESSED'::REGCLASS;
UPDATE PG_INDEX SET INDISVALID = FALSE WHERE INDEXRELID = 
'COMPRESSED'::REGCLASS;

EXPLAIN ANALYZE select (SELECT COUNT(*) FROM TESTTABLE WHERE C <@ CUBE(-b,1)) 
from generate_series(1,50) b;
EXPLAIN ANALYZE select (SELECT COUNT(*) FROM TESTTABLE WHERE C <@ CUBE(-b,1)) 
from generate_series(1,50) b;

COMMIT;





0001-Allow-uncompressed-GiST.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