Allow GiST opcalsses without compress\decompres functions

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

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

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.

--
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:

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

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

28 мая 2017 г. 11:15 PM пользователь "Tom Lane" <tgl@sss.pgh.pa.us> написал:

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

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.

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

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

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.

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(&centry)));
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.

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=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

On Thu, Sep 14, 2017 at 2:20 PM, Dmitriy Sarafannikov <
dsarafannikov@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

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=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

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 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.

On Fri, Sep 15, 2017 at 3:36 PM, Andrey Borodin <x4mmm@yandex-team.ru>
wrote:

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 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

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

On Wed, Sep 20, 2017 at 12:43 PM, Andrey Borodin <x4mmm@yandex-team.ru>
wrote:

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.

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

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

On Wed, Sep 20, 2017 at 4:25 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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().

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

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

On Wed, Sep 20, 2017 at 5:02 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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.

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