Re: [HACKERS] ALTER INDEX .. SET STATISTICS ... behaviour

[Simon Riggs]

On 6 September 2017 at 10:27, Tom Lane  wrote:
> Simon Riggs  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 Riggshttp://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] ALTER INDEX .. SET STATISTICS ... behaviour

[Tom Lane]

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

Re: [HACKERS] ALTER INDEX .. SET STATISTICS ... behaviour

[Simon Riggs]

On 4 September 2017 at 10:30, Adrien Nayrat  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 Riggshttp://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] ALTER INDEX .. SET STATISTICS ... behaviour

[Adrien Nayrat]

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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] ALTER INDEX .. SET STATISTICS ... behaviour

[Alexander Korotkov]

Hi, Adrien!

On Mon, Sep 4, 2017 at 3:57 PM, Adrien Nayrat 
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


alter-index-statistics-3.patch
Description: Binary data

-- 
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] ALTER INDEX .. SET STATISTICS ... behaviour

[Adrien Nayrat]

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
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index f6049cc9e5..6fb9bdd063 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -1742,6 +1742,7 @@ describeOneTableDetails(const char *schemaname,
 	{
 		headers[cols++] = gettext_noop("Storage");
 		if (tableinfo.relkind == RELKIND_RELATION ||
+			tableinfo.relkind == RELKIND_INDEX ||
 			tableinfo.relkind == RELKIND_MATVIEW ||
 			tableinfo.relkind == RELKIND_FOREIGN_TABLE ||
 			tableinfo.relkind == RELKIND_PARTITIONED_TABLE)
@@ -1841,6 +1842,7 @@ describeOneTableDetails(const char *schemaname,
 
 			/* Statistics target, if the relkind supports this feature */
 			if (tableinfo.relkind == RELKIND_RELATION ||
+tableinfo.relkind == RELKIND_INDEX ||
 tableinfo.relkind == RELKIND_MATVIEW ||
 tableinfo.relkind == RELKIND_FOREIGN_TABLE ||
 tableinfo.relkind == RELKIND_PARTITIONED_TABLE)
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 1583cfa998..baa739a8c0 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1644,7 +1644,10 @@ psql_completion(const char *text, int start, int end)
    "UNION SELECT 'ALL IN TABLESPACE'");
 	/* ALTER INDEX  */
 	else if (Matches3("ALTER", "INDEX", MatchAny))
-		COMPLETE_WITH_LIST4("OWNER TO", "RENAME TO", "SET", "RESET");
+		COMPLETE_WITH_LIST5("ALTER COLUMN", "OWNER TO", "RENAME TO", "SET", "RESET");
+	/* ALTER INDEX  ALTER COLUMN  */
+	else if (Matches6("ALTER", "INDEX", MatchAny, "ALTER", "COLUMN", MatchAny))
+		COMPLETE_WITH_CONST("SET STATISTICS");
 	/* ALTER INDEX  SET */
 	else if (Matches4("ALTER", "INDEX", MatchAny, "SET"))
 		COMPLETE_WITH_LIST2("(", "TABLESPACE");
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 64160a8b4d..0f36423163 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -103,6 +103,15 @@ ALTER INDEX tmp_idx ALTER COLUMN 1 SET STATISTICS 1000;
 ERROR:  cannot alter statistics on non-expression column "a" of index "tmp_idx"
 HINT:  Alter statistics on table column instead.
 ALTER INDEX tmp_idx ALTER COLUMN 2 SET STATISTICS 1000;
+\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"
+
 ALTER INDEX tmp_idx ALTER COLUMN 3 SET STATISTICS 1000;
 ERROR:  cannot alter statistics on non-expression column "b" of index "tmp_idx"
 HINT:  Alter statistics on table column instead.
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 064adb4640..8450f2463e 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2324,10 +2324,10 @@ DROP TABLE array_gin_test;
 CREATE INDEX gin_relopts_test ON array_index_op_test USING gin (i)
   WITH (FASTUPDATE=on, GIN_PENDING_LIST_LIMIT=128);
 \d+ gin_relopts_test
- Index "public.gin_relopts_test"
- Column |  Type   | Definition | Storage 
-+-++-
- i  | integer | i  | plain
+Index "public.gin_relopts_test"
+ Column |  Type   | Definition | Storage | Stats target 
++-++-+--
+ i  | integer | i  | plain   | 
 gin, for table "public.array_index_op_test"
 Options: fastupdate=on, gin_pending_list_limit=128
 
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 57f44c445d..e6f6669880 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ 

Re: [HACKERS] ALTER INDEX .. SET STATISTICS ... behaviour

[Alexander Korotkov]

On Thu, Jun 1, 2017 at 4:10 PM, Alexander Korotkov <
a.korot...@postgrespro.ru> wrote:

> On Wed, May 31, 2017 at 7:18 PM, Alexander Korotkov <
> a.korot...@postgrespro.ru> wrote:
>
>> On Wed, May 31, 2017 at 6:53 PM, Tom Lane  wrote:
>>
>>> Alexander Korotkov  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


alter-index-statistics-2.patch
Description: Binary data

-- 
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] ALTER INDEX .. SET STATISTICS ... behaviour

[Amit Kapila]

On Sun, Jun 4, 2017 at 10:11 AM, Tom Lane  wrote:
> Amit Kapila  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

Re: [HACKERS] ALTER INDEX .. SET STATISTICS ... behaviour

[Tom Lane]

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

Re: [HACKERS] ALTER INDEX .. SET STATISTICS ... behaviour

[Amit Kapila]

On Thu, Jun 1, 2017 at 6:40 PM, Alexander Korotkov
 wrote:
> On Wed, May 31, 2017 at 7:18 PM, Alexander Korotkov
>  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

Re: [HACKERS] ALTER INDEX .. SET STATISTICS ... behaviour

[Alexander Korotkov]

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

> On Wed, May 31, 2017 at 6:53 PM, Tom Lane  wrote:
>
>> Alexander Korotkov  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


alter-index-statistics-1.patch
Description: Binary data

-- 
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] ALTER INDEX .. SET STATISTICS ... behaviour

[Alexander Korotkov]

On Wed, May 31, 2017 at 6:53 PM, Tom Lane  wrote:

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

Re: [HACKERS] ALTER INDEX .. SET STATISTICS ... behaviour

[Tom Lane]

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

[HACKERS] ALTER INDEX .. SET STATISTICS ... behaviour

[Alexander Korotkov]

Hackers,

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

1.
https://www.postgresql.org/message-id/flat/20150716143149.GO2301%40postgresql.org

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