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