ALTER INDEX .. SET STATISTICS ... behaviour

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

I've discovered that PostgreSQL is able to run following kind of queries in
order to change statistic-gathering target for an indexed expression.

ALTER INDEX index_name ALTER COLUMN expr SET STATISTICS stat_target;

It's been previously discussed in [1].

I think this should be fixed not just in docs. This is why I've started
thread in pgsql-hackers. For me usage of internal column names "expr",
"expr1", "expr2" etc. looks weird. And I think we should replace it with a
better syntax. What do you think about these options?

ALTER INDEX index_name ALTER EXPRESSION 0 SET STATISTICS stat_target; --
Refer expression by its number
ALTER INDEX index_name ALTER EXPRESSION (x + y) SET STATISTICS stat_target;
-- Refer expression by its definition

Don't like either of those particularly, but what about just targeting
a column by column number, independently of whether it's an expression?

ALTER INDEX myindex ALTER COLUMN 4 SET STATISTICS 1000;

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, May 31, 2017 at 6:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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

I've discovered that PostgreSQL is able to run following kind of queries

in

order to change statistic-gathering target for an indexed expression.

ALTER INDEX index_name ALTER COLUMN expr SET STATISTICS stat_target;

It's been previously discussed in [1].

I think this should be fixed not just in docs. This is why I've started
thread in pgsql-hackers. For me usage of internal column names "expr",
"expr1", "expr2" etc. looks weird. And I think we should replace it

with a

better syntax. What do you think about these options?

ALTER INDEX index_name ALTER EXPRESSION 0 SET STATISTICS stat_target; --
Refer expression by its number
ALTER INDEX index_name ALTER EXPRESSION (x + y) SET STATISTICS

stat_target;

-- Refer expression by its definition

Don't like either of those particularly, but what about just targeting
a column by column number, independently of whether it's an expression?

ALTER INDEX myindex ALTER COLUMN 4 SET STATISTICS 1000;

I completely agree with that.
If no objections, I will write a patch for that.

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

On Wed, May 31, 2017 at 7:18 PM, Alexander Korotkov <
a.korotkov@postgrespro.ru> wrote:

On Wed, May 31, 2017 at 6:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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

I've discovered that PostgreSQL is able to run following kind of

queries in

order to change statistic-gathering target for an indexed expression.

ALTER INDEX index_name ALTER COLUMN expr SET STATISTICS stat_target;

It's been previously discussed in [1].

I think this should be fixed not just in docs. This is why I've started
thread in pgsql-hackers. For me usage of internal column names "expr",
"expr1", "expr2" etc. looks weird. And I think we should replace it

with a

better syntax. What do you think about these options?

ALTER INDEX index_name ALTER EXPRESSION 0 SET STATISTICS stat_target; --
Refer expression by its number
ALTER INDEX index_name ALTER EXPRESSION (x + y) SET STATISTICS

stat_target;

-- Refer expression by its definition

Don't like either of those particularly, but what about just targeting
a column by column number, independently of whether it's an expression?

ALTER INDEX myindex ALTER COLUMN 4 SET STATISTICS 1000;

I completely agree with that.
If no objections, I will write a patch for that.

Please, find it in attached patch.

I decided to forbid referencing columns by numbers for tables, because
those numbers could contain gaps. Also, I forbid altering statistics
target for non-expression index columns, because it has no effect.

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

On Thu, Jun 1, 2017 at 6:40 PM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:

On Wed, May 31, 2017 at 7:18 PM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:

Don't like either of those particularly, but what about just targeting
a column by column number, independently of whether it's an expression?

ALTER INDEX myindex ALTER COLUMN 4 SET STATISTICS 1000;

I completely agree with that.
If no objections, I will write a patch for that.

Please, find it in attached patch.

I decided to forbid referencing columns by numbers for tables, because those
numbers could contain gaps. Also, I forbid altering statistics target for
non-expression index columns, because it has no effect.

In order to avoid losing track of this patch, I think it is better to
add it in open items list for 10.

--
With Regards,
Amit Kapila.
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

Amit Kapila <amit.kapila16@gmail.com> writes:

In order to avoid losing track of this patch, I think it is better to
add it in open items list for 10.

This is an entirely new feature, not a bug fix, and thus certainly not an
open item for v10. Please stick it in the next commitfest, instead.

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 Sun, Jun 4, 2017 at 10:11 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Amit Kapila <amit.kapila16@gmail.com> writes:

In order to avoid losing track of this patch, I think it is better to
add it in open items list for 10.

This is an entirely new feature, not a bug fix, and thus certainly not an
open item for v10. Please stick it in the next commitfest, instead.

Okay, that makes sense. Sorry, I missed the point in the first read
of the mail.

--
With Regards,
Amit Kapila.
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 Thu, Jun 1, 2017 at 4:10 PM, Alexander Korotkov <
a.korotkov@postgrespro.ru> wrote:

On Wed, May 31, 2017 at 7:18 PM, Alexander Korotkov <
a.korotkov@postgrespro.ru> wrote:

On Wed, May 31, 2017 at 6:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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

I've discovered that PostgreSQL is able to run following kind of

queries in

order to change statistic-gathering target for an indexed expression.

ALTER INDEX index_name ALTER COLUMN expr SET STATISTICS stat_target;

It's been previously discussed in [1].

I think this should be fixed not just in docs. This is why I've

started

thread in pgsql-hackers. For me usage of internal column names "expr",
"expr1", "expr2" etc. looks weird. And I think we should replace it

with a

better syntax. What do you think about these options?

ALTER INDEX index_name ALTER EXPRESSION 0 SET STATISTICS stat_target;

--

Refer expression by its number
ALTER INDEX index_name ALTER EXPRESSION (x + y) SET STATISTICS

stat_target;

-- Refer expression by its definition

Don't like either of those particularly, but what about just targeting
a column by column number, independently of whether it's an expression?

ALTER INDEX myindex ALTER COLUMN 4 SET STATISTICS 1000;

I completely agree with that.
If no objections, I will write a patch for that.

Please, find it in attached patch.

I decided to forbid referencing columns by numbers for tables, because
those numbers could contain gaps. Also, I forbid altering statistics
target for non-expression index columns, because it has no effect.

Patch rebased to current master is attached.

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

On 08/30/2017 02:26 PM, Alexander Korotkov wrote:

Patch rebased to current master is attached.

Hello,

I reviewed this patch. It works as expected but I have few remarks :

* There is a warning during compilation :

gram.y: In function ‘base_yyparse’:
gram.y:2090:6: warning: ISO C90 forbids mixed declarations and code
[-Wdeclaration-after-statement]
AlterTableCmd *n = makeNode(AlterTableCmd);
^~~~~~~~~~~~~

If I understand we should declare AlterTableCmd *n [...] before "if"?

* I propose to add "Stats target" information in psql verbose output command
\d+ (patch attached with test)

\d+ tmp_idx
Index "public.tmp_idx"
Column | Type | Definition | Storage | Stats target
--------+------------------+------------+---------+--------------
a | integer | a | plain |
expr | double precision | (d + e) | plain | 1000
b | cstring | b | plain |
btree, for table "public.tmp"

* psql completion is missing (added to previous patch)

Regards,

--
Adrien NAYRAT

http://dalibo.com - http://dalibo.org

Hi, Adrien!

On Mon, Sep 4, 2017 at 3:57 PM, Adrien Nayrat <adrien.nayrat@dalibo.com>
wrote:

On 08/30/2017 02:26 PM, Alexander Korotkov wrote:
I reviewed this patch. It works as expected but I have few remarks :

Thank you for reviewing my patch!.

* There is a warning during compilation :

gram.y: In function ‘base_yyparse’:
gram.y:2090:6: warning: ISO C90 forbids mixed declarations and code
[-Wdeclaration-after-statement]
AlterTableCmd *n = makeNode(AlterTableCmd);
^~~~~~~~~~~~~

Fixed.

If I understand we should declare AlterTableCmd *n [...] before "if"?

* I propose to add "Stats target" information in psql verbose output
command
\d+ (patch attached with test)

\d+ tmp_idx
Index "public.tmp_idx"
Column | Type | Definition | Storage | Stats target
--------+------------------+------------+---------+--------------
a | integer | a | plain |
expr | double precision | (d + e) | plain | 1000
b | cstring | b | plain |
btree, for table "public.tmp"

* psql completion is missing (added to previous patch)

Looks good for me. I've integrated those changes in the patch.
New revision is attached.

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

On 09/04/2017 06:16 PM, Alexander Korotkov wrote:

Looks good for me.  I've integrated those changes in the patch.
New revision is attached.

Thanks, I changed status to "ready for commiter".

--
Adrien NAYRAT

http://dalibo.com - http://dalibo.org

On 4 September 2017 at 10:30, Adrien Nayrat <adrien.nayrat@dalibo.com> wrote:

On 09/04/2017 06:16 PM, Alexander Korotkov wrote:

Looks good for me. I've integrated those changes in the patch.
New revision is attached.

Thanks, I changed status to "ready for commiter".

This looks useful and good to me.

I've changed this line of code to return NULL rather than return tuple
if (!HeapTupleIsValid(tuple))
return tuple;

Other than that, I'm good to commit.

Any last minute objections?

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

Simon Riggs <simon@2ndquadrant.com> writes:

Other than that, I'm good to commit.
Any last minute objections?

The docs and comments could stand a bit closer copy-editing by a
native English speaker. Other than that, it seemed sane in a
quick read-through.

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 6 September 2017 at 10:27, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Simon Riggs <simon@2ndquadrant.com> writes:

Other than that, I'm good to commit.
Any last minute objections?

The docs and comments could stand a bit closer copy-editing by a
native English speaker. Other than that, it seemed sane in a
quick read-through.

Will do

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