Обсуждение: [HACKERS] [PATCH] Pageinspect - add functions on GIN and GiST indexes fromgevel

Hi all,

the following patch transfers functionality from gevel module
(http://www.sai.msu.su/~megera/wiki/Gevel) which provides functions for
analyzing GIN and GiST indexes to pageinspect. Gevel was originally
designed by Oleg Bartunov, and Teodor Sigaev for developers of GiST and
GIN indexes.

Functions added:
  - gist_stat(text) - shows statistics on GiST Tree
  - gist_tree(text) - shows GiST tree
  - gist_tree(text, int4) - shows GiST tree up to MAXLEVEL
  - gist_print(text) - prints objects stored in GiST tree
  - spgist_stat(text) - shows statistics on SP-GiST
  - spgist_print(text) - prints objects stored in index
  - gin_value_count() - originally gin_stat(text) - prints estimated counts
for index values
  - gin_stats() - originally gin_statpage(text) - shows statistics
  - gin_count_estimate(text, tsquery) - shows number of indexed rows matched
query

Tests also transferred, docs for new functions are added. I run pgindent
over the code, but the result is different from those I expected, so I leave
pgindented one.
The patch is applicable to the commit 
866f4a7c210857aa342bf901558d170325094dde.

-- 
Alexey Chernyshov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


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

On Fri, Jul 21, 2017 at 8:05 AM, Alexey Chernyshov
<a.chernyshov@postgrespro.ru> wrote:
> the following patch transfers functionality from gevel module
> (http://www.sai.msu.su/~megera/wiki/Gevel) which provides functions for
> analyzing GIN and GiST indexes to pageinspect. Gevel was originally
> designed by Oleg Bartunov, and Teodor Sigaev for developers of GiST and
> GIN indexes.

It's not clear from the web site in question that the relevant code is
released under the PostgreSQL license.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Jul 24, 2017 at 11:38 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Jul 21, 2017 at 8:05 AM, Alexey Chernyshov
> <a.chernyshov@postgrespro.ru> wrote:
>> the following patch transfers functionality from gevel module
>> (http://www.sai.msu.su/~megera/wiki/Gevel) which provides functions for
>> analyzing GIN and GiST indexes to pageinspect. Gevel was originally
>> designed by Oleg Bartunov, and Teodor Sigaev for developers of GiST and
>> GIN indexes.
>
> It's not clear from the web site in question that the relevant code is
> released under the PostgreSQL license.

git clone git://sigaev.ru/gevel

from README.gevel

License

Stable version, included into PostgreSQL distribution, released under
BSD license. Development version, available from this site, released under
the GNU General Public License, version 2 (June 1991)

We would be happy to write anything community likes :)

>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

>
> It's not clear from the web site in question that the relevant code is
> released under the PostgreSQL license.
>

As author I allow to use this code in PostgreSQL and under its license.

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

On Tue, Jul 25, 2017 at 5:57 AM, Teodor Sigaev <teodor@sigaev.ru> wrote:
>> It's not clear from the web site in question that the relevant code is
>> released under the PostgreSQL license.
>
> As author I allow to use this code in PostgreSQL and under its license.

Great.  As long as all authors feel that way, we're fine.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sat, Jul 22, 2017 at 12:05 AM, Alexey Chernyshov
<a.chernyshov@postgrespro.ru> wrote:
> the following patch transfers functionality from gevel module
> (http://www.sai.msu.su/~megera/wiki/Gevel) which provides functions for
> analyzing GIN and GiST indexes to pageinspect. Gevel was originally
> designed by Oleg Bartunov, and Teodor Sigaev for developers of GiST and
> GIN indexes.
>
> Functions added:
>  - gist_stat(text) - shows statistics on GiST Tree
>  - gist_tree(text) - shows GiST tree
>  - gist_tree(text, int4) - shows GiST tree up to MAXLEVEL
>  - gist_print(text) - prints objects stored in GiST tree
>  - spgist_stat(text) - shows statistics on SP-GiST
>  - spgist_print(text) - prints objects stored in index
>  - gin_value_count() - originally gin_stat(text) - prints estimated counts
> for index values
>  - gin_stats() - originally gin_statpage(text) - shows statistics
>  - gin_count_estimate(text, tsquery) - shows number of indexed rows matched
> query
>
> Tests also transferred, docs for new functions are added. I run pgindent
> over the code, but the result is different from those I expected, so I leave
> pgindented one.
> The patch is applicable to the commit
> 866f4a7c210857aa342bf901558d170325094dde.

Hi Alexey,

This patch still applies but doesn't build after commits 2cd70845 and c6293249.

ginfuncs.c:91:47: error: invalid type argument of ‘->’ (have
‘FormData_pg_attribute’)          st->index->rd_att->attrs[st->attnum]->attbyval,
...several similar errors...

For example, that expression needs to be changed to:
          TupleDescAttr(st->index->rd_att, attnum)->attbyval

Here is some superficial review since I'm here:

+/*
+ * process_tuples
+ * Retrieves the number of TIDs in a tuple.
+ */
+static void
+process_tuple(FuncCallContext *funcctx, GinStatState * st, IndexTuple itup)

"process_tuples" vs "process_tuple"

+ /* do no distiguish various null category */

"do not distinguish various null categories"

+ st->nulls[0] = (st->category == GIN_CAT_NORM_KEY) ? false : true;

That's a long way to write (st->category != GIN_CAT_NORM_KEY)!

+ * We scaned the whole page, so we should take right page

"scanned"

+/*
+ * refind_position
+ * Refinds a previois position, at returns it has correctly
+ * set offset and buffer is locked.
+ */

"previous", s/, at returns/.  On return/

+ memset(st->nulls, false, 2 * sizeof(*st->nulls));

"false" looks strange here where an int is expected.  The size
expression is weird.  I think you should just write:

st->nulls[0] = false;
st->nulls[1] = false;

Investigating the types more closely, I see that 'nulls' is declared
like this (in struct GinStatState):

+ char nulls[2];

Then later you do this:

+ htuple = heap_form_tuple(funcctx->attinmeta->tupdesc,
+     st->dvalues,
+     st->nulls);

But that's not OK, heap_form_tuple takes bool *.  bool and char are
not interchangeable (they may have different sizes on some platforms,
among other problems, even though usually they are both 1 byte).  So I
think you should declare it as "bool nulls[2]".

Meanwhile in TypeStorage you have a member "bool *nulls", which you
then initialise like so:

+ st->nulls = (char *) palloc((tupdesc->natts + 2) * sizeof(*st->nulls));

That cast is wrong.

+/*
+ * gin_count_estimate
+ * Outputs number of indexed rows matched query. It doesn't touch heap at
+ * all.

Maybe "... number of indexed rows matching query."?

+ if (attnum < 0 || attnum >= st->index->rd_att->natts)
+     elog(ERROR, "Wrong column's number");
+ st->attnum = attnum;

Maybe "invalid column number" or "invalid attribute number"?

+ elog(ERROR, "Column type is not a tsvector");

s/C/c/ (according to convention).

+ * Prints various stat about index internals.

s/stat/stats/

+ elog(ERROR, "Relation %s.%s is not an index",

s/R/r/

+ elog(ERROR, "Index %s.%s has wrong type",

s/I/i/

+      <function>gin_value_count</function> prints estimated counts for each
+      indexed values, single-argument function will return result for a first
+      column of index. For example:

"... for each indexed value.  The single argument version will return
results for the first column of an index.  For example:"

+      <function>gin_count_estimate</function> outputs number of indexed
+     rows matched query. It doesn't touch heap at all. For example:

"... outputs the number of indexed rows matched by a query. ..."

+      <function>gist_print</function> prints objects stored in
+      <acronym>GiST</acronym> tree, works only if objects in index have
+      textual representation (<function>type_out</function> functions should be
+      implemented for the given object type). It's known to work with R-tree
+      <acronym>GiST</acronym> based index. Note, in example below, objects are
+      of type box. For example:

"... prints objects stored in a GiST tree.  it works only if the
objects in the index have textual representation (type_out functions
should be implemented for the given object type).  It's known to work
with R-tree GiST-based indexes.  Note: in the example below, objects
are of type box.  For example:"

+      <function>gin_value_count</function> prints estimated counts for each
+      indexed value for a given column. For example:

Maybe "for a given column (starting from zero)"?

+      <function>spgist_print</function> prints objects stored in
+      <acronym>SP-GiST</acronym> tree, works only if objects in index have
+      textual representation (<function>type_out</function> functions should
+      be implemented for the given object type).

"... tree.  It works only if ..."

+         Note 1. in example below we used quad_point_ops which uses point
+         for leaf and prefix value, but doesn't use node_label at all.
+         Use type  'int' as dummy type for prefix or/and node_label.

"... in the example ..."

A few random whitespace changes:

- (errmsg("must be superuser to use raw page functions"))));
+ (errmsg("must be superuser to use raw page functions")))
+ );

-   opaq->flags, GIN_META)));
+   opaq->flags, GIN_META))
+ );

- flags[nflags++] = DirectFunctionCall1(to_hex32, Int32GetDatum(flagbits));
+ flags[nflags++] = DirectFunctionCall1(to_hex32,
+  Int32GetDatum(flagbits));

-   (GIN_DATA | GIN_LEAF | GIN_COMPRESSED))));
+   (GIN_DATA | GIN_LEAF | GIN_COMPRESSED)))
+ );

--
Thomas Munro
http://www.enterprisedb.com

On Wed, 6 Sep 2017 22:56:45 +1200
Thomas Munro <thomas.munro@enterprisedb.com> wrote:

Hi Thomas,

Thanks a lot for your careful review.

> 
> This patch still applies but doesn't build after commits 2cd70845 and
> c6293249.
> 

Updated patch is attached.

> 
> Here is some superficial review since I'm here:
> 

Thank you very much, I hope I corrected the most part of mistakes.

-- 
Alexey Chernyshov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

On 07/21/2017 03:40 PM, Alexander Korotkov wrote:
> Hi, Alexey!
> 
> On Fri, Jul 21, 2017 at 3:05 PM, Alexey Chernyshov
> <a.chernyshov@postgrespro.ru <mailto:a.chernyshov@postgrespro.ru>> wrote:
> 
>     the following patch transfers functionality from gevel module
>     (http://www.sai.msu.su/~megera/wiki/Gevel
>     <http://www.sai.msu.su/~megera/wiki/Gevel>) which provides functions for
>     analyzing GIN and GiST indexes to pageinspect. Gevel was originally
>     designed by Oleg Bartunov, and Teodor Sigaev for developers of GiST and
>     GIN indexes.
> 
> 
> It's very good that you've picked up this work!  pageinspect is lacking
> of this functionality.  
> 
>     Functions added:
>      - gist_stat(text) - shows statistics on GiST Tree
>      - gist_tree(text) - shows GiST tree
>      - gist_tree(text, int4) - shows GiST tree up to MAXLEVEL
>      - gist_print(text) - prints objects stored in GiST tree
>      - spgist_stat(text) - shows statistics on SP-GiST
>      - spgist_print(text) - prints objects stored in index
>      - gin_value_count() - originally gin_stat(text) - prints estimated
>     counts
>     for index values
>      - gin_stats() - originally gin_statpage(text) - shows statistics
>      - gin_count_estimate(text, tsquery) - shows number of indexed rows
>     matched
>     query
> 
>     Tests also transferred, docs for new functions are added. I run pgindent
>     over the code, but the result is different from those I expected, so
>     I leave
>     pgindented one.
>     The patch is applicable to the commit
>     866f4a7c210857aa342bf901558d170325094dde.
> 
> 
> As we can see, gevel contains functions which analyze the whole index.
>  pageinspect is written in another manner: it gives you functionality to
> analyze individual pages, tuples and so on.
> Thus, we probably shouldn't try to move gevel functions to pageinspect
> "as is".  They should be rewritten in more granular manner as well as
> other pageinspact functions are.  Any other opinions?
>  

I agree with Alexander that pageinspect is written in a very different
way - as the extension name suggests, it's used to inspect pages. The
proposed patch uses a very different approach, reading the whole index,
not individual pages. Why should it be part of pageinspect?

For example we have pgstattuple extension, which seems like a better
match. Or just create a new extension - if the code is valuable, surely
we can live one more extension instead of smuggling it in inside
pageinspect.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Sep 8, 2017 at 3:38 AM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> Hi,
>
> On 07/21/2017 03:40 PM, Alexander Korotkov wrote:
>> Hi, Alexey!
>>
>> On Fri, Jul 21, 2017 at 3:05 PM, Alexey Chernyshov
>> <a.chernyshov@postgrespro.ru <mailto:a.chernyshov@postgrespro.ru>> wrote:
>>
>>     the following patch transfers functionality from gevel module
>>     (http://www.sai.msu.su/~megera/wiki/Gevel
>>     <http://www.sai.msu.su/~megera/wiki/Gevel>) which provides functions for
>>     analyzing GIN and GiST indexes to pageinspect. Gevel was originally
>>     designed by Oleg Bartunov, and Teodor Sigaev for developers of GiST and
>>     GIN indexes.
>>
>>
>> It's very good that you've picked up this work!  pageinspect is lacking
>> of this functionality.
>>
>>     Functions added:
>>      - gist_stat(text) - shows statistics on GiST Tree
>>      - gist_tree(text) - shows GiST tree
>>      - gist_tree(text, int4) - shows GiST tree up to MAXLEVEL
>>      - gist_print(text) - prints objects stored in GiST tree
>>      - spgist_stat(text) - shows statistics on SP-GiST
>>      - spgist_print(text) - prints objects stored in index
>>      - gin_value_count() - originally gin_stat(text) - prints estimated
>>     counts
>>     for index values
>>      - gin_stats() - originally gin_statpage(text) - shows statistics
>>      - gin_count_estimate(text, tsquery) - shows number of indexed rows
>>     matched
>>     query
>>
>>     Tests also transferred, docs for new functions are added. I run pgindent
>>     over the code, but the result is different from those I expected, so
>>     I leave
>>     pgindented one.
>>     The patch is applicable to the commit
>>     866f4a7c210857aa342bf901558d170325094dde.
>>
>>
>> As we can see, gevel contains functions which analyze the whole index.
>>  pageinspect is written in another manner: it gives you functionality to
>> analyze individual pages, tuples and so on.
>> Thus, we probably shouldn't try to move gevel functions to pageinspect
>> "as is".  They should be rewritten in more granular manner as well as
>> other pageinspact functions are.  Any other opinions?
>>
>
> I agree with Alexander that pageinspect is written in a very different
> way - as the extension name suggests, it's used to inspect pages. The
> proposed patch uses a very different approach, reading the whole index,
> not individual pages. Why should it be part of pageinspect?
>
> For example we have pgstattuple extension, which seems like a better
> match. Or just create a new extension - if the code is valuable, surely
> we can live one more extension instead of smuggling it in inside
> pageinspect.
>

Finally, i got some time to look into this patch and surprisingly I
didn't find any function returning information at page level instead
all the SQL functions are returning information at index level.
Therefore, i too feel that it should be either integrated with
pgstattuple which could a better match as Tomas also mentioned or may
be add a new contrib module itself. I think, adding a new contrib
module looks like a better option. The reason being, it doesn't simply
include the function for showing index level statistics (for e.g..
index size, no of rows, values..e.t.c) like pgstattuple does but,
also, displays information contained in a page for e.g. the object
stored in the page,  it's tid e.t.c. So, more or less, I would say
that, it's the mixture of pageinspect and pgstattuple module and
therefore, i feel, adding it as a new extension would be a better
choice. Thought?

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com


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

On Sat, 9 Sep 2017 13:53:35 +0530
Ashutosh Sharma <ashu.coek88@gmail.com> wrote:

> 
> Finally, i got some time to look into this patch and surprisingly I
> didn't find any function returning information at page level instead
> all the SQL functions are returning information at index level.
> Therefore, i too feel that it should be either integrated with
> pgstattuple which could a better match as Tomas also mentioned or may
> be add a new contrib module itself. I think, adding a new contrib
> module looks like a better option. The reason being, it doesn't simply
> include the function for showing index level statistics (for e.g..
> index size, no of rows, values..e.t.c) like pgstattuple does but,
> also, displays information contained in a page for e.g. the object
> stored in the page,  it's tid e.t.c. So, more or less, I would say
> that, it's the mixture of pageinspect and pgstattuple module and
> therefore, i feel, adding it as a new extension would be a better
> choice. Thought?

Thank you for your interest, I will add a new contrib module named,
say, indexstat. I think we can add statistics on other indexes in the
future.

-- 
Alexey Chernyshov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


-- 
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 13, 2017 at 1:15 PM, Alexey Chernyshov
<a.chernyshov@postgrespro.ru> wrote:
> On Sat, 9 Sep 2017 13:53:35 +0530
> Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
>>
>> Finally, i got some time to look into this patch and surprisingly I
>> didn't find any function returning information at page level instead
>> all the SQL functions are returning information at index level.
>> Therefore, i too feel that it should be either integrated with
>> pgstattuple which could a better match as Tomas also mentioned or may
>> be add a new contrib module itself. I think, adding a new contrib
>> module looks like a better option. The reason being, it doesn't simply
>> include the function for showing index level statistics (for e.g..
>> index size, no of rows, values..e.t.c) like pgstattuple does but,
>> also, displays information contained in a page for e.g. the object
>> stored in the page,  it's tid e.t.c. So, more or less, I would say
>> that, it's the mixture of pageinspect and pgstattuple module and
>> therefore, i feel, adding it as a new extension would be a better
>> choice. Thought?
>
> Thank you for your interest, I will add a new contrib module named,
> say, indexstat. I think we can add statistics on other indexes in the
> future.
>

I think we should wait for experts opinion and then take a call. I am
not expert. I just gave my opinion as i have worked in this area
earlier when working for hash index. Thanks.

-- 
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com


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

On 9/13/17 03:45, Alexey Chernyshov wrote:
> On Sat, 9 Sep 2017 13:53:35 +0530
> Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> 
>>
>> Finally, i got some time to look into this patch and surprisingly I
>> didn't find any function returning information at page level instead
>> all the SQL functions are returning information at index level.
>> Therefore, i too feel that it should be either integrated with
>> pgstattuple which could a better match as Tomas also mentioned or may
>> be add a new contrib module itself. I think, adding a new contrib
>> module looks like a better option. The reason being, it doesn't simply
>> include the function for showing index level statistics (for e.g..
>> index size, no of rows, values..e.t.c) like pgstattuple does but,
>> also, displays information contained in a page for e.g. the object
>> stored in the page,  it's tid e.t.c. So, more or less, I would say
>> that, it's the mixture of pageinspect and pgstattuple module and
>> therefore, i feel, adding it as a new extension would be a better
>> choice. Thought?
> 
> Thank you for your interest, I will add a new contrib module named,
> say, indexstat. I think we can add statistics on other indexes in the
> future.

A few thoughts on this current patch:

The patch no longer compiles, because of changes in the way the tuple
descriptors are accessed (I think).

I agree with the conclusion so far that this should be a new extension,
not being a good fit for an existing extension.

This kind of thing doesn't look like good design:

+<screen>
+test=# SELECT spgist_stats('spgist_idx');
+           spgist_stats
+----------------------------------
+ totalPages:        21           +
+ deletedPages:      0            +
+ innerPages:        3            +
+ leafPages:         18           +
+ emptyPages:        1            +
+ usedSpace:         121.27 kbytes+
+ freeSpace:         46.07 kbytes +
+ fillRatio:         72.47%       +
+ leafTuples:        3669         +
+ innerTuples:       20           +
+ innerAllTheSame:   0            +
+ leafPlaceholders:  569          +
+ innerPlaceholders: 0            +
+ leafRedirects:     0            +
+ innerRedirects:    0            +
+</screen>

This function should return a row with columns for each of these pieces
of information.

So to summarize, I think there is interest in this functionality, but
the patch needs some work.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
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, Nov 2, 2017 at 3:47 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> So to summarize, I think there is interest in this functionality, but
> the patch needs some work.

This patch has been waiting for input from its author for three weeks,
so I am marking it as returned with feedback.
-- 
Michael