Re: [HACKERS] ALTER INDEX .. SET STATISTICS ... behaviour
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
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
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
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
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
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 @@ -150,6
Re: [HACKERS] ALTER INDEX .. SET STATISTICS ... behaviour
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
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
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
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
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
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
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
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