Re: [HACKERS] [PATCH] Pageinspect - add functions on GIN and GiST indexes from gevel

[Peter Eisentraut]

On 9/13/17 03:45, Alexey Chernyshov wrote:
> On Sat, 9 Sep 2017 13:53:35 +0530
> Ashutosh Sharma  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:

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

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

Re: [HACKERS] [PATCH] Pageinspect - add functions on GIN and GiST indexes from gevel

[Alexander Korotkov]

On Wed, Sep 13, 2017 at 10:57 AM, Ashutosh Sharma 
wrote:

> On Wed, Sep 13, 2017 at 1:15 PM, Alexey Chernyshov
>  wrote:
> > On Sat, 9 Sep 2017 13:53:35 +0530
> > Ashutosh Sharma  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.


I'm not sure that we should port these functions from gevel directly.  We
could try to re-implement similar functionality which fits pageinspect
approach better.  In particular, we can implement some low-level functions
whose show detailed information at page level.  And on top of them we can
implement analogues of gevel functions in SQL or PL/pgSQL.  Is it feasible?

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

Re: [HACKERS] [PATCH] Pageinspect - add functions on GIN and GiST indexes from gevel

[Ashutosh Sharma]

On Wed, Sep 13, 2017 at 1:15 PM, Alexey Chernyshov
 wrote:
> On Sat, 9 Sep 2017 13:53:35 +0530
> Ashutosh Sharma  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

Re: [HACKERS] [PATCH] Pageinspect - add functions on GIN and GiST indexes from gevel

[Alexey Chernyshov]

On Sat, 9 Sep 2017 13:53:35 +0530
Ashutosh Sharma  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

Re: [HACKERS] [PATCH] Pageinspect - add functions on GIN and GiST indexes from gevel

[Ashutosh Sharma]

On Fri, Sep 8, 2017 at 3:38 AM, Tomas Vondra
 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
>> mailto:a.chernys...@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 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

Re: [HACKERS] [PATCH] Pageinspect - add functions on GIN and GiST indexes from gevel

[Tomas Vondra]

Hi,

On 07/21/2017 03:40 PM, Alexander Korotkov wrote:
> Hi, Alexey!
> 
> On Fri, Jul 21, 2017 at 3:05 PM, Alexey Chernyshov
> mailto:a.chernys...@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 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


-- 
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] [PATCH] Pageinspect - add functions on GIN and GiST indexes from gevel

[Thomas Munro]

On Sat, Jul 22, 2017 at 12:05 AM, Alexey Chernyshov
 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/

+  gin_value_count 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:"

+  gin_count_estimate 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. ..."

+  gist_print prints objects stored in
+  GiST tree, works only if objects in 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 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:"

+  gin_value_count prints estimated counts fo

Re: [HACKERS] [PATCH] Pageinspect - add functions on GIN and GiST indexes from gevel

[Robert Haas]

On Tue, Jul 25, 2017 at 5:57 AM, Teodor Sigaev  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


-- 
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] [PATCH] Pageinspect - add functions on GIN and GiST indexes from gevel

[Teodor Sigaev]

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: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/


--
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] [PATCH] Pageinspect - add functions on GIN and GiST indexes from gevel

[Oleg Bartunov]

On Mon, Jul 24, 2017 at 11:38 PM, Robert Haas  wrote:
> On Fri, Jul 21, 2017 at 8:05 AM, Alexey Chernyshov
>  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


-- 
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] [PATCH] Pageinspect - add functions on GIN and GiST indexes from gevel

[Robert Haas]

On Fri, Jul 21, 2017 at 8:05 AM, Alexey Chernyshov
 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


-- 
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] [PATCH] Pageinspect - add functions on GIN and GiST indexes from gevel

[Alexander Korotkov]

Hi, Alexey!

On Fri, Jul 21, 2017 at 3:05 PM, Alexey Chernyshov <
a.chernys...@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 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 866f4a7c210857aa342bf901558d17
> 0325094dde.
>

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?

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