Subject: Re: ALTER INDEX .. SET STATISTICS ... behaviour

From: Alexander Korotkov <[email-masked]>
Date: Wed, 30 Aug 2017 15:26:36 +0300

On Thu, Jun 1, 2017 at 4:10 PM, Alexander Korotkov <
[email-masked]> wrote:

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

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

---

From: Adrien Nayrat <[email-masked]>
Date: Mon, 4 Sep 2017 14:57:08 +0200

> 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 <name> */
 	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 <name> ALTER COLUMN <colnum> */
+	else if (Matches6("ALTER", "INDEX", MatchAny, "ALTER", "COLUMN", MatchAny))
+		COMPLETE_WITH_CONST("SET STATISTICS");
 	/* ALTER INDEX <name> 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
@@ -150,6 +150,8 @@ ALTER INDEX tmp_idx ALTER COLUMN 1 SET STATISTICS 1000;
 
 ALTER INDEX tmp_idx ALTER COLUMN 2 SET STATISTICS 1000;
 
+\d+ tmp_idx
+
 ALTER INDEX tmp_idx ALTER COLUMN 3 SET STATISTICS 1000;
 
 ALTER INDEX tmp_idx ALTER COLUMN 4 SET STATISTICS 1000;

---

From: Alexander Korotkov <[email-masked]>
Date: Mon, 4 Sep 2017 19:16:00 +0300

Hi, Adrien!

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

Thank you for reviewing my patch!.

>       AlterTableCmd *n = makeNode(AlterTableCmd);

Fixed.

>  Column |       Type       | Definition | Storage | Stats target

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

------
Alexander Korotkov

---

From: Adrien Nayrat <[email-masked]>
Date: Mon, 4 Sep 2017 19:30:11 +0200

> New revision is attached.

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

-- 
Adrien NAYRAT

---

From: Simon Riggs <[email-masked]>
Date: Wed, 6 Sep 2017 09:59:20 -0700

> 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

---

From: Tom Lane <[email-masked]>
Date: Wed, 06 Sep 2017 13:27:36 -0400

> 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

---

From: Simon Riggs <[email-masked]>
Date: Wed, 6 Sep 2017 10:33:59 -0700

> native English speaker.  Other than that, it seemed sane in a

Will do