pgsql: Indexes with INCLUDE columns and their support in B-tree

On 2018-04-07 22:01, Teodor Sigaev wrote:

Indexes with INCLUDE columns and their support in B-tree

Author: Anastasia Lubennikova with contribition by Alexander Korotkov
and me
Reviewed by: Peter Geoghegan, Tomas Vondra, Antonin Houska, Jeff Janes,
David Rowley, Alexander Korotkov

I've put quite some time in testing this patch, and although I don't
really mind not being mentioned, I think it's better to err on the side
of including too many names than too few.

Erik Rijkers

Ooops, sorry, if it possible, I'd like to change list of reviewers to
add you, but I don't know how do it.

Nevertheless, thank you very much for your work

BTW, I miss Andrey Borodin in that list too...

Erik Rijkers wrote:

On 2018-04-07 22:01, Teodor Sigaev wrote:

Indexes with INCLUDE columns and their support in B-tree

Author: Anastasia Lubennikova with contribition by Alexander Korotkov
and me
Reviewed by: Peter Geoghegan, Tomas Vondra, Antonin Houska, Jeff Janes,
О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫О©╫ David Rowley, Alexander Korotkov

I've put quite some time in testing this patch, and although I don't
really mind not being mentioned, I think it's better to err on the side
of including too many names than too few.

Erik Rijkers

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

On 2018-04-07 22:51, Teodor Sigaev wrote:

Ooops, sorry, if it possible, I'd like to change list of reviewers to
add you, but I don't know how do it.

No problem, really.

Thanks, great feature! I'm glad it got in.

8 апр. 2018 г., в 1:51, Teodor Sigaev <teodor@sigaev.ru> написал(а):

Ooops, sorry, if it possible, I'd like to change list of reviewers to add you, but I don't know how do it.

Nevertheless, thank you very much for your work

BTW, I miss Andrey Borodin in that list too...

No problem :) Thanks for pushing this!

Best regards, Andrey Borodin.

Teodor Sigaev <teodor@sigaev.ru> writes:

Indexes with INCLUDE columns and their support in B-tree

The valgrind-using animals seem to think this broke stuff.
There are at least two different problems here:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lousyjack&amp;dt=2018-04-07%2020%3A03%3A02

regards, tom lane

On Sun, Apr 8, 2018 at 5:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Teodor Sigaev <teodor@sigaev.ru> writes:

Indexes with INCLUDE columns and their support in B-tree

The valgrind-using animals seem to think this broke stuff.
There are at least two different problems here:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=
lousyjack&dt=2018-04-07%2020%3A03%3A02

I think attached patch should fix one of those problems.
We should match clauses to only key columns of index.
I will further investigate the problem with CheckIndexCompatible().

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

Thank you, pushed

Alexander Korotkov wrote:

On Sun, Apr 8, 2018 at 5:41 PM, Tom Lane <tgl@sss.pgh.pa.us
<mailto:tgl@sss.pgh.pa.us>> wrote:

Teodor Sigaev <teodor@sigaev.ru <mailto:teodor@sigaev.ru>> writes:

Indexes with INCLUDE columns and their support in B-tree

The valgrind-using animals seem to think this broke stuff.
There are at least two different problems here:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lousyjack&amp;dt=2018-04-07%2020%3A03%3A02
<https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lousyjack&amp;dt=2018-04-07%2020%3A03%3A02&gt;

I think attached patch should fix one of those problems.
We should match clauses to only key columns of index.
I will further investigate the problem with CheckIndexCompatible().

------
Alexander Korotkov
Postgres Professional:http://www.postgrespro.com
<http://www.postgrespro.com/&gt;
The Russian Postgres Company

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

On Sun, Apr 8, 2018 at 7:58 PM, Teodor Sigaev <teodor@sigaev.ru> wrote:

Thank you, pushed

Thank you!

There is also patch for valgrind error in CheckIndexCompatible().
It appears that this function didn't initialize ii_NumIndexAttrs and
ii_NumIndexKeyAttrs before calling ComputeIndexAttrs(). This
variables weren't used before, but now they're used to determine
whether index column is key. Also, this patch makes ComputeIndexAttrs()
initialize classOidP elements with InvalidOid for non-key attributes.

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

Alexander Korotkov <a.korotkov@postgrespro.ru> writes:

There is also patch for valgrind error in CheckIndexCompatible().
It appears that this function didn't initialize ii_NumIndexAttrs and
ii_NumIndexKeyAttrs before calling ComputeIndexAttrs(). This
variables weren't used before, but now they're used to determine
whether index column is key. Also, this patch makes ComputeIndexAttrs()
initialize classOidP elements with InvalidOid for non-key attributes.

You didn't test this quite enough, because when I tried it I got a batch
of warnings about memory chunk inconsistencies. The reason was that
DefineIndex only allocated a classObjectId array large enough for the
key columns, so that the change in ComputeIndexAttrs made it write past
the end of that array. But I think it's a good idea to make those output
arrays all the same length, so I kept that change and changed DefineIndex.

Pushed; I've not checked to see if this makes valgrind happy, but
the buildfarm will tell us.

regards, tom lane

On Mon, Apr 9, 2018 at 12:27 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Alexander Korotkov <a.korotkov@postgrespro.ru> writes:

There is also patch for valgrind error in CheckIndexCompatible().
It appears that this function didn't initialize ii_NumIndexAttrs and
ii_NumIndexKeyAttrs before calling ComputeIndexAttrs(). This
variables weren't used before, but now they're used to determine
whether index column is key. Also, this patch makes ComputeIndexAttrs()
initialize classOidP elements with InvalidOid for non-key attributes.

You didn't test this quite enough, because when I tried it I got a batch
of warnings about memory chunk inconsistencies. The reason was that
DefineIndex only allocated a classObjectId array large enough for the
key columns, so that the change in ComputeIndexAttrs made it write past
the end of that array.

Sorry, my bad.

But I think it's a good idea to make those output

arrays all the same length, so I kept that change and changed DefineIndex.

Pushed; I've not checked to see if this makes valgrind happy, but
the buildfarm will tell us.

Thank you!

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

On 4/7/18 16:01, Teodor Sigaev wrote:

Indexes with INCLUDE columns and their support in B-tree

pg_dump from <8.2 servers is now failing.

The last branch in getIndexes() is not producing the required
indnkeyatts column.

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

Thank you, will see

Peter Eisentraut wrote:

On 4/7/18 16:01, Teodor Sigaev wrote:

Indexes with INCLUDE columns and their support in B-tree

pg_dump from <8.2 servers is now failing.

The last branch in getIndexes() is not producing the required
indnkeyatts column.

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