Обсуждение: [HACKERS] Allow GiST opcalsses without compress\decompres functions

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.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andrew Borodin <borodin@octonica.com> 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

Thanks, Tom!

2017-05-28 22:22 GMT+05:00 Tom Lane <tgl@sss.pgh.pa.us>:
> Andrew Borodin <borodin@octonica.com> 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.

Andrew Borodin <borodin@octonica.com> writes:
> 2017-05-28 22:22 GMT+05:00 Tom Lane <tgl@sss.pgh.pa.us>:
>> 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

Andrew Borodin <borodin@octonica.com> 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

2017-05-29 0:00 GMT+05:00 Tom Lane <tgl@sss.pgh.pa.us>:
> 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.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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(&giststate->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(&giststate->compressFn[i],
     giststate->supportCollation[i],                                              PointerGetDatum(¢ry)));
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

> 11 сент. 2017 г., в 12:57, Dmitriy Sarafannikov <dsarafannikov@yandex.ru> написал(а):
> 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.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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
       FUNCTION        1       g_cube_consistent (internal, cube, smallint, oid, internal),       FUNCTION        2
 g_cube_union (internal, internal),
 
-       FUNCTION        3       g_cube_compress (internal),
-       FUNCTION        4       g_cube_decompress (internal),       FUNCTION        5       g_cube_penalty (internal,
internal,internal),       FUNCTION        6       g_cube_picksplit (internal, internal),       FUNCTION        7
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=5loops=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

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

> 14 сент. 2017 г., в 18:42, Alexander Korotkov <a.korotkov@postgrespro.ru> написал(а):
> It would be good if someone would write patch for removing useless compress/decompress methods from builtin and
contribGiST 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
YandexData 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.



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andrey Borodin <x4mmm@yandex-team.ru> 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

Hello Tom! Thanks for committing the patch!

> 20 сент. 2017 г., в 8:38, Tom Lane <tgl@sss.pgh.pa.us> написал(а):
>
> Andrey Borodin <x4mmm@yandex-team.ru> 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

Andrey Borodin <x4mmm@yandex-team.ru> 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

Alexander Korotkov <a.korotkov@postgrespro.ru> writes:
> On Wed, Sep 20, 2017 at 4:25 PM, Tom Lane <tgl@sss.pgh.pa.us> 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

Hi, Tom!
> 20 сент. 2017 г., в 8:38, Tom Lane <tgl@sss.pgh.pa.us> написал(а):
>
> Andrey Borodin <x4mmm@yandex-team.ru> 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
withoutcompress\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.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andrey Borodin <x4mmm@yandex-team.ru> 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_dependwhere 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

> 22 окт. 2017 г., в 21:21, Tom Lane <tgl@sss.pgh.pa.us> написал(а):
>
> Andrey Borodin <x4mmm@yandex-team.ru> 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
couldbe 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