Make name optional in CREATE STATISTICS
Currently, CREATE STATS requires you to think of a name for each stats
object, which is fairly painful, so users would prefer an
automatically assigned name.
Attached patch allows this, which turns out to be very simple, since a
name assignment function already exists.
The generated name is simple, but that's exactly what users do anyway,
so it is not too bad.
Tests, docs included.
--
Simon Riggs http://www.EnterpriseDB.com/
Attachments:
create_stats_opt_name.v3.patchapplication/octet-stream; name=create_stats_opt_name.v3.patchDownload
diff --git a/doc/src/sgml/ref/create_statistics.sgml b/doc/src/sgml/ref/create_statistics.sgml
index 9a8c904c08..a7a55553e1 100644
--- a/doc/src/sgml/ref/create_statistics.sgml
+++ b/doc/src/sgml/ref/create_statistics.sgml
@@ -21,11 +21,11 @@ PostgreSQL documentation
<refsynopsisdiv>
<synopsis>
-CREATE STATISTICS [ IF NOT EXISTS ] <replaceable class="parameter">statistics_name</replaceable>
+CREATE STATISTICS [ [ IF NOT EXISTS ] <replaceable class="parameter">statistics_name</replaceable>]
ON ( <replaceable class="parameter">expression</replaceable> )
FROM <replaceable class="parameter">table_name</replaceable>
-CREATE STATISTICS [ IF NOT EXISTS ] <replaceable class="parameter">statistics_name</replaceable>
+CREATE STATISTICS [ [ IF NOT EXISTS ] <replaceable class="parameter">statistics_name</replaceable>]
[ ( <replaceable class="parameter">statistics_kind</replaceable> [, ... ] ) ]
ON { <replaceable class="parameter">column_name</replaceable> | ( <replaceable class="parameter">expression</replaceable> ) }, { <replaceable class="parameter">column_name</replaceable> | ( <replaceable class="parameter">expression</replaceable> ) } [, ...]
FROM <replaceable class="parameter">table_name</replaceable>
@@ -60,8 +60,8 @@ CREATE STATISTICS [ IF NOT EXISTS ] <replaceable class="parameter">statistics_na
If a schema name is given (for example, <literal>CREATE STATISTICS
myschema.mystat ...</literal>) then the statistics object is created in the
specified schema. Otherwise it is created in the current schema.
- The name of the statistics object must be distinct from the name of any
- other statistics object in the same schema.
+ If given, the name of the statistics object must be distinct from the name
+ of any other statistics object in the same schema.
</para>
</refsect1>
@@ -78,6 +78,7 @@ CREATE STATISTICS [ IF NOT EXISTS ] <replaceable class="parameter">statistics_na
exists. A notice is issued in this case. Note that only the name of
the statistics object is considered here, not the details of its
definition.
+ Statistics name is required when <literal>IF NOT EXISTS</literal> is specified.
</para>
</listitem>
</varlistentry>
@@ -88,6 +89,9 @@ CREATE STATISTICS [ IF NOT EXISTS ] <replaceable class="parameter">statistics_na
<para>
The name (optionally schema-qualified) of the statistics object to be
created.
+ If the name is omitted, <productname>PostgreSQL</productname> chooses a
+ suitable name based on the parent table's name and the defined column
+ name(s) and/or expression(s).
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index c9941d9cb4..484935b264 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -4362,6 +4362,18 @@ CreateStatsStmt:
n->if_not_exists = true;
$$ = (Node *)n;
}
+ | CREATE STATISTICS
+ opt_name_list ON stats_params FROM from_list
+ {
+ CreateStatsStmt *n = makeNode(CreateStatsStmt);
+ n->defnames = NULL;
+ n->stat_types = $3;
+ n->exprs = $5;
+ n->relations = $7;
+ n->stxcomment = NULL;
+ n->if_not_exists = false;
+ $$ = (Node *)n;
+ }
;
/*
diff --git a/src/test/regress/expected/stats_ext.out b/src/test/regress/expected/stats_ext.out
index 042316aeed..8f5fd546eb 100644
--- a/src/test/regress/expected/stats_ext.out
+++ b/src/test/regress/expected/stats_ext.out
@@ -3062,11 +3062,11 @@ CREATE STATISTICS tststats.priv_test_stats (mcv) ON a, b
ANALYZE tststats.priv_test_tbl;
-- Check printing info about extended statistics by \dX
create table stts_t1 (a int, b int);
-create statistics stts_1 (ndistinct) on a, b from stts_t1;
-create statistics stts_2 (ndistinct, dependencies) on a, b from stts_t1;
-create statistics stts_3 (ndistinct, dependencies, mcv) on a, b from stts_t1;
+create statistics (ndistinct) on a, b from stts_t1;
+create statistics (ndistinct, dependencies) on a, b from stts_t1;
+create statistics (ndistinct, dependencies, mcv) on a, b from stts_t1;
create table stts_t2 (a int, b int, c int);
-create statistics stts_4 on b, c from stts_t2;
+create statistics on b, c from stts_t2;
create table stts_t3 (col1 int, col2 int, col3 int);
create statistics stts_hoge on col1, col2, col3 from stts_t3;
create schema stts_s1;
@@ -3084,24 +3084,24 @@ set search_path to public, stts_s1, stts_s2, tststats;
public | mcv_lists_arrays_stats | a, b, c FROM mcv_lists_arrays | | | defined
public | mcv_lists_bool_stats | a, b, c FROM mcv_lists_bool | | | defined
public | mcv_lists_stats | a, b, d FROM mcv_lists | | | defined
- public | stts_1 | a, b FROM stts_t1 | defined | |
- public | stts_2 | a, b FROM stts_t1 | defined | defined |
- public | stts_3 | a, b FROM stts_t1 | defined | defined | defined
- public | stts_4 | b, c FROM stts_t2 | defined | defined | defined
public | stts_hoge | col1, col2, col3 FROM stts_t3 | defined | defined | defined
+ public | stts_t1_a_b_stat | a, b FROM stts_t1 | defined | |
+ public | stts_t1_a_b_stat1 | a, b FROM stts_t1 | defined | defined |
+ public | stts_t1_a_b_stat2 | a, b FROM stts_t1 | defined | defined | defined
+ public | stts_t2_b_c_stat | b, c FROM stts_t2 | defined | defined | defined
stts_s1 | stts_foo | col1, col2 FROM stts_t3 | defined | defined | defined
stts_s2 | stts_yama | col1, col3 FROM stts_t3 | | defined | defined
tststats | priv_test_stats | a, b FROM priv_test_tbl | | | defined
(12 rows)
-\dX stts_?
- List of extended statistics
- Schema | Name | Definition | Ndistinct | Dependencies | MCV
---------+--------+-------------------+-----------+--------------+---------
- public | stts_1 | a, b FROM stts_t1 | defined | |
- public | stts_2 | a, b FROM stts_t1 | defined | defined |
- public | stts_3 | a, b FROM stts_t1 | defined | defined | defined
- public | stts_4 | b, c FROM stts_t2 | defined | defined | defined
+\dX stts_t*
+ List of extended statistics
+ Schema | Name | Definition | Ndistinct | Dependencies | MCV
+--------+-------------------+-------------------+-----------+--------------+---------
+ public | stts_t1_a_b_stat | a, b FROM stts_t1 | defined | |
+ public | stts_t1_a_b_stat1 | a, b FROM stts_t1 | defined | defined |
+ public | stts_t1_a_b_stat2 | a, b FROM stts_t1 | defined | defined | defined
+ public | stts_t2_b_c_stat | b, c FROM stts_t2 | defined | defined | defined
(4 rows)
\dX *stts_hoge
@@ -3119,24 +3119,24 @@ set search_path to public, stts_s1, stts_s2, tststats;
public | mcv_lists_arrays_stats | a, b, c FROM mcv_lists_arrays | | | defined
public | mcv_lists_bool_stats | a, b, c FROM mcv_lists_bool | | | defined
public | mcv_lists_stats | a, b, d FROM mcv_lists | | | defined
- public | stts_1 | a, b FROM stts_t1 | defined | |
- public | stts_2 | a, b FROM stts_t1 | defined | defined |
- public | stts_3 | a, b FROM stts_t1 | defined | defined | defined
- public | stts_4 | b, c FROM stts_t2 | defined | defined | defined
public | stts_hoge | col1, col2, col3 FROM stts_t3 | defined | defined | defined
+ public | stts_t1_a_b_stat | a, b FROM stts_t1 | defined | |
+ public | stts_t1_a_b_stat1 | a, b FROM stts_t1 | defined | defined |
+ public | stts_t1_a_b_stat2 | a, b FROM stts_t1 | defined | defined | defined
+ public | stts_t2_b_c_stat | b, c FROM stts_t2 | defined | defined | defined
stts_s1 | stts_foo | col1, col2 FROM stts_t3 | defined | defined | defined
stts_s2 | stts_yama | col1, col3 FROM stts_t3 | | defined | defined
tststats | priv_test_stats | a, b FROM priv_test_tbl | | | defined
(12 rows)
-\dX+ stts_?
- List of extended statistics
- Schema | Name | Definition | Ndistinct | Dependencies | MCV
---------+--------+-------------------+-----------+--------------+---------
- public | stts_1 | a, b FROM stts_t1 | defined | |
- public | stts_2 | a, b FROM stts_t1 | defined | defined |
- public | stts_3 | a, b FROM stts_t1 | defined | defined | defined
- public | stts_4 | b, c FROM stts_t2 | defined | defined | defined
+\dX+ stts_t*
+ List of extended statistics
+ Schema | Name | Definition | Ndistinct | Dependencies | MCV
+--------+-------------------+-------------------+-----------+--------------+---------
+ public | stts_t1_a_b_stat | a, b FROM stts_t1 | defined | |
+ public | stts_t1_a_b_stat1 | a, b FROM stts_t1 | defined | defined |
+ public | stts_t1_a_b_stat2 | a, b FROM stts_t1 | defined | defined | defined
+ public | stts_t2_b_c_stat | b, c FROM stts_t2 | defined | defined | defined
(4 rows)
\dX+ *stts_hoge
@@ -3153,6 +3153,21 @@ set search_path to public, stts_s1, stts_s2, tststats;
stts_s2 | stts_yama | col1, col3 FROM stts_t3 | | defined | defined
(1 row)
+create statistics (mcv) ON a, b, (a+b), (a-b) FROM stts_t1;
+create statistics (mcv) ON a, b, (a+b), (a-b) FROM stts_t1;
+create statistics (mcv) ON (a+b), (a-b) FROM stts_t1;
+\dX stts_t*expr*
+ List of extended statistics
+ Schema | Name | Definition | Ndistinct | Dependencies | MCV
+--------+-----------------------------+-------------------------------------+-----------+--------------+---------
+ public | stts_t1_a_b_expr_expr_stat | a, b, (a + b), (a - b) FROM stts_t1 | | | defined
+ public | stts_t1_a_b_expr_expr_stat1 | a, b, (a + b), (a - b) FROM stts_t1 | | | defined
+ public | stts_t1_expr_expr_stat | (a + b), (a - b) FROM stts_t1 | | | defined
+(3 rows)
+
+drop statistics stts_t1_a_b_expr_expr_stat;
+drop statistics stts_t1_a_b_expr_expr_stat1;
+drop statistics stts_t1_expr_expr_stat;
set search_path to public, stts_s1;
\dX
List of extended statistics
@@ -3162,11 +3177,11 @@ set search_path to public, stts_s1;
public | mcv_lists_arrays_stats | a, b, c FROM mcv_lists_arrays | | | defined
public | mcv_lists_bool_stats | a, b, c FROM mcv_lists_bool | | | defined
public | mcv_lists_stats | a, b, d FROM mcv_lists | | | defined
- public | stts_1 | a, b FROM stts_t1 | defined | |
- public | stts_2 | a, b FROM stts_t1 | defined | defined |
- public | stts_3 | a, b FROM stts_t1 | defined | defined | defined
- public | stts_4 | b, c FROM stts_t2 | defined | defined | defined
public | stts_hoge | col1, col2, col3 FROM stts_t3 | defined | defined | defined
+ public | stts_t1_a_b_stat | a, b FROM stts_t1 | defined | |
+ public | stts_t1_a_b_stat1 | a, b FROM stts_t1 | defined | defined |
+ public | stts_t1_a_b_stat2 | a, b FROM stts_t1 | defined | defined | defined
+ public | stts_t2_b_c_stat | b, c FROM stts_t2 | defined | defined | defined
stts_s1 | stts_foo | col1, col2 FROM stts_t3 | defined | defined | defined
(10 rows)
@@ -3180,11 +3195,11 @@ set role regress_stats_ext;
public | mcv_lists_arrays_stats | a, b, c FROM mcv_lists_arrays | | | defined
public | mcv_lists_bool_stats | a, b, c FROM mcv_lists_bool | | | defined
public | mcv_lists_stats | a, b, d FROM mcv_lists | | | defined
- public | stts_1 | a, b FROM stts_t1 | defined | |
- public | stts_2 | a, b FROM stts_t1 | defined | defined |
- public | stts_3 | a, b FROM stts_t1 | defined | defined | defined
- public | stts_4 | b, c FROM stts_t2 | defined | defined | defined
public | stts_hoge | col1, col2, col3 FROM stts_t3 | defined | defined | defined
+ public | stts_t1_a_b_stat | a, b FROM stts_t1 | defined | |
+ public | stts_t1_a_b_stat1 | a, b FROM stts_t1 | defined | defined |
+ public | stts_t1_a_b_stat2 | a, b FROM stts_t1 | defined | defined | defined
+ public | stts_t2_b_c_stat | b, c FROM stts_t2 | defined | defined | defined
(9 rows)
reset role;
diff --git a/src/test/regress/sql/stats_ext.sql b/src/test/regress/sql/stats_ext.sql
index 6b954c9e50..5fd865f509 100644
--- a/src/test/regress/sql/stats_ext.sql
+++ b/src/test/regress/sql/stats_ext.sql
@@ -1555,12 +1555,12 @@ ANALYZE tststats.priv_test_tbl;
-- Check printing info about extended statistics by \dX
create table stts_t1 (a int, b int);
-create statistics stts_1 (ndistinct) on a, b from stts_t1;
-create statistics stts_2 (ndistinct, dependencies) on a, b from stts_t1;
-create statistics stts_3 (ndistinct, dependencies, mcv) on a, b from stts_t1;
+create statistics (ndistinct) on a, b from stts_t1;
+create statistics (ndistinct, dependencies) on a, b from stts_t1;
+create statistics (ndistinct, dependencies, mcv) on a, b from stts_t1;
create table stts_t2 (a int, b int, c int);
-create statistics stts_4 on b, c from stts_t2;
+create statistics on b, c from stts_t2;
create table stts_t3 (col1 int, col2 int, col3 int);
create statistics stts_hoge on col1, col2, col3 from stts_t3;
@@ -1575,13 +1575,21 @@ analyze stts_t1;
set search_path to public, stts_s1, stts_s2, tststats;
\dX
-\dX stts_?
+\dX stts_t*
\dX *stts_hoge
\dX+
-\dX+ stts_?
+\dX+ stts_t*
\dX+ *stts_hoge
\dX+ stts_s2.stts_yama
+create statistics (mcv) ON a, b, (a+b), (a-b) FROM stts_t1;
+create statistics (mcv) ON a, b, (a+b), (a-b) FROM stts_t1;
+create statistics (mcv) ON (a+b), (a-b) FROM stts_t1;
+\dX stts_t*expr*
+drop statistics stts_t1_a_b_expr_expr_stat;
+drop statistics stts_t1_a_b_expr_expr_stat1;
+drop statistics stts_t1_expr_expr_stat;
+
set search_path to public, stts_s1;
\dX
ne 15. 5. 2022 v 14:20 odesílatel Simon Riggs <simon.riggs@enterprisedb.com>
napsal:
Currently, CREATE STATS requires you to think of a name for each stats
object, which is fairly painful, so users would prefer an
automatically assigned name.Attached patch allows this, which turns out to be very simple, since a
name assignment function already exists.The generated name is simple, but that's exactly what users do anyway,
so it is not too bad.
good idea
Pavel
Show quoted text
Tests, docs included.
--
Simon Riggs http://www.EnterpriseDB.com/
On Sun, 15 May 2022 at 14:20, Simon Riggs <simon.riggs@enterprisedb.com> wrote:
Currently, CREATE STATS requires you to think of a name for each stats
object, which is fairly painful, so users would prefer an
automatically assigned name.Attached patch allows this, which turns out to be very simple, since a
name assignment function already exists.The generated name is simple, but that's exactly what users do anyway,
so it is not too bad.
Cool.
Tests, docs included.
Something I noticed is that this grammar change is quite different
from how create index specifies its optional name. Because we already
have a seperate statement sections for with and without IF NOT EXISTS,
adding another branch will add even more duplication. Using a new
opt_name production (potentially renamed from opt_index_name?) would
probably reduce the amount of duplication in the grammar.
We might be able to use opt_if_not_exists to fully remove the
duplicated grammars, but I don't think we would be able to keep the
"CREATE STATISTICS IF NOT EXISTS <<no name>> ON col1, col2 FROM table"
syntax illegal.
Please also update the comment in gram.y above the updated section
that details the expected grammar for CREATE STATISTICS, as you seem
to have overlooked that copy of grammar documentation.
Apart from these two small issues, this passes tests and seems complete.
Kind regards,
Matthias van de Meent
On Wed, 6 Jul 2022 at 19:35, Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
On Sun, 15 May 2022 at 14:20, Simon Riggs <simon.riggs@enterprisedb.com> wrote:
Currently, CREATE STATS requires you to think of a name for each stats
object, which is fairly painful, so users would prefer an
automatically assigned name.Attached patch allows this, which turns out to be very simple, since a
name assignment function already exists.The generated name is simple, but that's exactly what users do anyway,
so it is not too bad.Cool.
Thanks for your review.
Tests, docs included.
Something I noticed is that this grammar change is quite different
from how create index specifies its optional name. Because we already
have a seperate statement sections for with and without IF NOT EXISTS,
adding another branch will add even more duplication. Using a new
opt_name production (potentially renamed from opt_index_name?) would
probably reduce the amount of duplication in the grammar.
There are various other ways of doing this and, yes, we could refactor
other parts of the grammar to make this work. There is a specific
guideline about patch submission that says the best way to get a patch
rejected is to include unnecessary changes. With that in mind, let's
keep the patch simple and exactly aimed at the original purpose.
I'll leave it for committers to decide whether other refactoring is wanted.
We might be able to use opt_if_not_exists to fully remove the
duplicated grammars, but I don't think we would be able to keep the
"CREATE STATISTICS IF NOT EXISTS <<no name>> ON col1, col2 FROM table"
syntax illegal.Please also update the comment in gram.y above the updated section
that details the expected grammar for CREATE STATISTICS, as you seem
to have overlooked that copy of grammar documentation.
I have made the comment show that the name is optional, thank you.
Apart from these two small issues, this passes tests and seems complete.
Patch v4 attached
--
Simon Riggs http://www.EnterpriseDB.com/
Attachments:
create_stats_opt_name.v4.patchapplication/octet-stream; name=create_stats_opt_name.v4.patchDownload
diff --git a/doc/src/sgml/ref/create_statistics.sgml b/doc/src/sgml/ref/create_statistics.sgml
index 9a8c904c08..a7a55553e1 100644
--- a/doc/src/sgml/ref/create_statistics.sgml
+++ b/doc/src/sgml/ref/create_statistics.sgml
@@ -21,11 +21,11 @@ PostgreSQL documentation
<refsynopsisdiv>
<synopsis>
-CREATE STATISTICS [ IF NOT EXISTS ] <replaceable class="parameter">statistics_name</replaceable>
+CREATE STATISTICS [ [ IF NOT EXISTS ] <replaceable class="parameter">statistics_name</replaceable>]
ON ( <replaceable class="parameter">expression</replaceable> )
FROM <replaceable class="parameter">table_name</replaceable>
-CREATE STATISTICS [ IF NOT EXISTS ] <replaceable class="parameter">statistics_name</replaceable>
+CREATE STATISTICS [ [ IF NOT EXISTS ] <replaceable class="parameter">statistics_name</replaceable>]
[ ( <replaceable class="parameter">statistics_kind</replaceable> [, ... ] ) ]
ON { <replaceable class="parameter">column_name</replaceable> | ( <replaceable class="parameter">expression</replaceable> ) }, { <replaceable class="parameter">column_name</replaceable> | ( <replaceable class="parameter">expression</replaceable> ) } [, ...]
FROM <replaceable class="parameter">table_name</replaceable>
@@ -60,8 +60,8 @@ CREATE STATISTICS [ IF NOT EXISTS ] <replaceable class="parameter">statistics_na
If a schema name is given (for example, <literal>CREATE STATISTICS
myschema.mystat ...</literal>) then the statistics object is created in the
specified schema. Otherwise it is created in the current schema.
- The name of the statistics object must be distinct from the name of any
- other statistics object in the same schema.
+ If given, the name of the statistics object must be distinct from the name
+ of any other statistics object in the same schema.
</para>
</refsect1>
@@ -78,6 +78,7 @@ CREATE STATISTICS [ IF NOT EXISTS ] <replaceable class="parameter">statistics_na
exists. A notice is issued in this case. Note that only the name of
the statistics object is considered here, not the details of its
definition.
+ Statistics name is required when <literal>IF NOT EXISTS</literal> is specified.
</para>
</listitem>
</varlistentry>
@@ -88,6 +89,9 @@ CREATE STATISTICS [ IF NOT EXISTS ] <replaceable class="parameter">statistics_na
<para>
The name (optionally schema-qualified) of the statistics object to be
created.
+ If the name is omitted, <productname>PostgreSQL</productname> chooses a
+ suitable name based on the parent table's name and the defined column
+ name(s) and/or expression(s).
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 969c9c158f..7f4fdd120b 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -4523,7 +4523,7 @@ ExistingIndex: USING INDEX name { $$ = $3; }
/*****************************************************************************
*
* QUERY :
- * CREATE STATISTICS [IF NOT EXISTS] stats_name [(stat types)]
+ * CREATE STATISTICS [IF NOT EXISTS] [stats_name] [(stat types)]
* ON expression-list FROM from_list
*
* Note: the expectation here is that the clauses after ON are a subset of
@@ -4561,6 +4561,18 @@ CreateStatsStmt:
n->if_not_exists = true;
$$ = (Node *) n;
}
+ | CREATE STATISTICS
+ opt_name_list ON stats_params FROM from_list
+ {
+ CreateStatsStmt *n = makeNode(CreateStatsStmt);
+ n->defnames = NULL;
+ n->stat_types = $3;
+ n->exprs = $5;
+ n->relations = $7;
+ n->stxcomment = NULL;
+ n->if_not_exists = false;
+ $$ = (Node *) n;
+ }
;
/*
diff --git a/src/test/regress/expected/stats_ext.out b/src/test/regress/expected/stats_ext.out
index 042316aeed..8f5fd546eb 100644
--- a/src/test/regress/expected/stats_ext.out
+++ b/src/test/regress/expected/stats_ext.out
@@ -3062,11 +3062,11 @@ CREATE STATISTICS tststats.priv_test_stats (mcv) ON a, b
ANALYZE tststats.priv_test_tbl;
-- Check printing info about extended statistics by \dX
create table stts_t1 (a int, b int);
-create statistics stts_1 (ndistinct) on a, b from stts_t1;
-create statistics stts_2 (ndistinct, dependencies) on a, b from stts_t1;
-create statistics stts_3 (ndistinct, dependencies, mcv) on a, b from stts_t1;
+create statistics (ndistinct) on a, b from stts_t1;
+create statistics (ndistinct, dependencies) on a, b from stts_t1;
+create statistics (ndistinct, dependencies, mcv) on a, b from stts_t1;
create table stts_t2 (a int, b int, c int);
-create statistics stts_4 on b, c from stts_t2;
+create statistics on b, c from stts_t2;
create table stts_t3 (col1 int, col2 int, col3 int);
create statistics stts_hoge on col1, col2, col3 from stts_t3;
create schema stts_s1;
@@ -3084,24 +3084,24 @@ set search_path to public, stts_s1, stts_s2, tststats;
public | mcv_lists_arrays_stats | a, b, c FROM mcv_lists_arrays | | | defined
public | mcv_lists_bool_stats | a, b, c FROM mcv_lists_bool | | | defined
public | mcv_lists_stats | a, b, d FROM mcv_lists | | | defined
- public | stts_1 | a, b FROM stts_t1 | defined | |
- public | stts_2 | a, b FROM stts_t1 | defined | defined |
- public | stts_3 | a, b FROM stts_t1 | defined | defined | defined
- public | stts_4 | b, c FROM stts_t2 | defined | defined | defined
public | stts_hoge | col1, col2, col3 FROM stts_t3 | defined | defined | defined
+ public | stts_t1_a_b_stat | a, b FROM stts_t1 | defined | |
+ public | stts_t1_a_b_stat1 | a, b FROM stts_t1 | defined | defined |
+ public | stts_t1_a_b_stat2 | a, b FROM stts_t1 | defined | defined | defined
+ public | stts_t2_b_c_stat | b, c FROM stts_t2 | defined | defined | defined
stts_s1 | stts_foo | col1, col2 FROM stts_t3 | defined | defined | defined
stts_s2 | stts_yama | col1, col3 FROM stts_t3 | | defined | defined
tststats | priv_test_stats | a, b FROM priv_test_tbl | | | defined
(12 rows)
-\dX stts_?
- List of extended statistics
- Schema | Name | Definition | Ndistinct | Dependencies | MCV
---------+--------+-------------------+-----------+--------------+---------
- public | stts_1 | a, b FROM stts_t1 | defined | |
- public | stts_2 | a, b FROM stts_t1 | defined | defined |
- public | stts_3 | a, b FROM stts_t1 | defined | defined | defined
- public | stts_4 | b, c FROM stts_t2 | defined | defined | defined
+\dX stts_t*
+ List of extended statistics
+ Schema | Name | Definition | Ndistinct | Dependencies | MCV
+--------+-------------------+-------------------+-----------+--------------+---------
+ public | stts_t1_a_b_stat | a, b FROM stts_t1 | defined | |
+ public | stts_t1_a_b_stat1 | a, b FROM stts_t1 | defined | defined |
+ public | stts_t1_a_b_stat2 | a, b FROM stts_t1 | defined | defined | defined
+ public | stts_t2_b_c_stat | b, c FROM stts_t2 | defined | defined | defined
(4 rows)
\dX *stts_hoge
@@ -3119,24 +3119,24 @@ set search_path to public, stts_s1, stts_s2, tststats;
public | mcv_lists_arrays_stats | a, b, c FROM mcv_lists_arrays | | | defined
public | mcv_lists_bool_stats | a, b, c FROM mcv_lists_bool | | | defined
public | mcv_lists_stats | a, b, d FROM mcv_lists | | | defined
- public | stts_1 | a, b FROM stts_t1 | defined | |
- public | stts_2 | a, b FROM stts_t1 | defined | defined |
- public | stts_3 | a, b FROM stts_t1 | defined | defined | defined
- public | stts_4 | b, c FROM stts_t2 | defined | defined | defined
public | stts_hoge | col1, col2, col3 FROM stts_t3 | defined | defined | defined
+ public | stts_t1_a_b_stat | a, b FROM stts_t1 | defined | |
+ public | stts_t1_a_b_stat1 | a, b FROM stts_t1 | defined | defined |
+ public | stts_t1_a_b_stat2 | a, b FROM stts_t1 | defined | defined | defined
+ public | stts_t2_b_c_stat | b, c FROM stts_t2 | defined | defined | defined
stts_s1 | stts_foo | col1, col2 FROM stts_t3 | defined | defined | defined
stts_s2 | stts_yama | col1, col3 FROM stts_t3 | | defined | defined
tststats | priv_test_stats | a, b FROM priv_test_tbl | | | defined
(12 rows)
-\dX+ stts_?
- List of extended statistics
- Schema | Name | Definition | Ndistinct | Dependencies | MCV
---------+--------+-------------------+-----------+--------------+---------
- public | stts_1 | a, b FROM stts_t1 | defined | |
- public | stts_2 | a, b FROM stts_t1 | defined | defined |
- public | stts_3 | a, b FROM stts_t1 | defined | defined | defined
- public | stts_4 | b, c FROM stts_t2 | defined | defined | defined
+\dX+ stts_t*
+ List of extended statistics
+ Schema | Name | Definition | Ndistinct | Dependencies | MCV
+--------+-------------------+-------------------+-----------+--------------+---------
+ public | stts_t1_a_b_stat | a, b FROM stts_t1 | defined | |
+ public | stts_t1_a_b_stat1 | a, b FROM stts_t1 | defined | defined |
+ public | stts_t1_a_b_stat2 | a, b FROM stts_t1 | defined | defined | defined
+ public | stts_t2_b_c_stat | b, c FROM stts_t2 | defined | defined | defined
(4 rows)
\dX+ *stts_hoge
@@ -3153,6 +3153,21 @@ set search_path to public, stts_s1, stts_s2, tststats;
stts_s2 | stts_yama | col1, col3 FROM stts_t3 | | defined | defined
(1 row)
+create statistics (mcv) ON a, b, (a+b), (a-b) FROM stts_t1;
+create statistics (mcv) ON a, b, (a+b), (a-b) FROM stts_t1;
+create statistics (mcv) ON (a+b), (a-b) FROM stts_t1;
+\dX stts_t*expr*
+ List of extended statistics
+ Schema | Name | Definition | Ndistinct | Dependencies | MCV
+--------+-----------------------------+-------------------------------------+-----------+--------------+---------
+ public | stts_t1_a_b_expr_expr_stat | a, b, (a + b), (a - b) FROM stts_t1 | | | defined
+ public | stts_t1_a_b_expr_expr_stat1 | a, b, (a + b), (a - b) FROM stts_t1 | | | defined
+ public | stts_t1_expr_expr_stat | (a + b), (a - b) FROM stts_t1 | | | defined
+(3 rows)
+
+drop statistics stts_t1_a_b_expr_expr_stat;
+drop statistics stts_t1_a_b_expr_expr_stat1;
+drop statistics stts_t1_expr_expr_stat;
set search_path to public, stts_s1;
\dX
List of extended statistics
@@ -3162,11 +3177,11 @@ set search_path to public, stts_s1;
public | mcv_lists_arrays_stats | a, b, c FROM mcv_lists_arrays | | | defined
public | mcv_lists_bool_stats | a, b, c FROM mcv_lists_bool | | | defined
public | mcv_lists_stats | a, b, d FROM mcv_lists | | | defined
- public | stts_1 | a, b FROM stts_t1 | defined | |
- public | stts_2 | a, b FROM stts_t1 | defined | defined |
- public | stts_3 | a, b FROM stts_t1 | defined | defined | defined
- public | stts_4 | b, c FROM stts_t2 | defined | defined | defined
public | stts_hoge | col1, col2, col3 FROM stts_t3 | defined | defined | defined
+ public | stts_t1_a_b_stat | a, b FROM stts_t1 | defined | |
+ public | stts_t1_a_b_stat1 | a, b FROM stts_t1 | defined | defined |
+ public | stts_t1_a_b_stat2 | a, b FROM stts_t1 | defined | defined | defined
+ public | stts_t2_b_c_stat | b, c FROM stts_t2 | defined | defined | defined
stts_s1 | stts_foo | col1, col2 FROM stts_t3 | defined | defined | defined
(10 rows)
@@ -3180,11 +3195,11 @@ set role regress_stats_ext;
public | mcv_lists_arrays_stats | a, b, c FROM mcv_lists_arrays | | | defined
public | mcv_lists_bool_stats | a, b, c FROM mcv_lists_bool | | | defined
public | mcv_lists_stats | a, b, d FROM mcv_lists | | | defined
- public | stts_1 | a, b FROM stts_t1 | defined | |
- public | stts_2 | a, b FROM stts_t1 | defined | defined |
- public | stts_3 | a, b FROM stts_t1 | defined | defined | defined
- public | stts_4 | b, c FROM stts_t2 | defined | defined | defined
public | stts_hoge | col1, col2, col3 FROM stts_t3 | defined | defined | defined
+ public | stts_t1_a_b_stat | a, b FROM stts_t1 | defined | |
+ public | stts_t1_a_b_stat1 | a, b FROM stts_t1 | defined | defined |
+ public | stts_t1_a_b_stat2 | a, b FROM stts_t1 | defined | defined | defined
+ public | stts_t2_b_c_stat | b, c FROM stts_t2 | defined | defined | defined
(9 rows)
reset role;
diff --git a/src/test/regress/sql/stats_ext.sql b/src/test/regress/sql/stats_ext.sql
index 6b954c9e50..5fd865f509 100644
--- a/src/test/regress/sql/stats_ext.sql
+++ b/src/test/regress/sql/stats_ext.sql
@@ -1555,12 +1555,12 @@ ANALYZE tststats.priv_test_tbl;
-- Check printing info about extended statistics by \dX
create table stts_t1 (a int, b int);
-create statistics stts_1 (ndistinct) on a, b from stts_t1;
-create statistics stts_2 (ndistinct, dependencies) on a, b from stts_t1;
-create statistics stts_3 (ndistinct, dependencies, mcv) on a, b from stts_t1;
+create statistics (ndistinct) on a, b from stts_t1;
+create statistics (ndistinct, dependencies) on a, b from stts_t1;
+create statistics (ndistinct, dependencies, mcv) on a, b from stts_t1;
create table stts_t2 (a int, b int, c int);
-create statistics stts_4 on b, c from stts_t2;
+create statistics on b, c from stts_t2;
create table stts_t3 (col1 int, col2 int, col3 int);
create statistics stts_hoge on col1, col2, col3 from stts_t3;
@@ -1575,13 +1575,21 @@ analyze stts_t1;
set search_path to public, stts_s1, stts_s2, tststats;
\dX
-\dX stts_?
+\dX stts_t*
\dX *stts_hoge
\dX+
-\dX+ stts_?
+\dX+ stts_t*
\dX+ *stts_hoge
\dX+ stts_s2.stts_yama
+create statistics (mcv) ON a, b, (a+b), (a-b) FROM stts_t1;
+create statistics (mcv) ON a, b, (a+b), (a-b) FROM stts_t1;
+create statistics (mcv) ON (a+b), (a-b) FROM stts_t1;
+\dX stts_t*expr*
+drop statistics stts_t1_a_b_expr_expr_stat;
+drop statistics stts_t1_a_b_expr_expr_stat1;
+drop statistics stts_t1_expr_expr_stat;
+
set search_path to public, stts_s1;
\dX
On Thu, 7 Jul 2022 at 12:55, Simon Riggs <simon.riggs@enterprisedb.com> wrote:
There are various other ways of doing this and, yes, we could refactor
other parts of the grammar to make this work. There is a specific
guideline about patch submission that says the best way to get a patch
rejected is to include unnecessary changes. With that in mind, let's
keep the patch simple and exactly aimed at the original purpose.I'll leave it for committers to decide whether other refactoring is wanted.
Fair enough.
I have made the comment show that the name is optional, thank you.
The updated comment implies that IF NOT EXISTS is allowed without a
defined name, which is false:
+ * CREATE STATISTICS [IF NOT EXISTS] [stats_name] [(stat types)]
A more correct version would be
+ * CREATE STATISTICS [ [IF NOT EXISTS] stats_name ]
[(stat types)]
Patch v4 attached
Thanks for working on this!
Kind regards,
Matthias van de Meent
On Thu, 7 Jul 2022 at 11:58, Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
On Thu, 7 Jul 2022 at 12:55, Simon Riggs <simon.riggs@enterprisedb.com> wrote:
There are various other ways of doing this and, yes, we could refactor
other parts of the grammar to make this work. There is a specific
guideline about patch submission that says the best way to get a patch
rejected is to include unnecessary changes. With that in mind, let's
keep the patch simple and exactly aimed at the original purpose.I'll leave it for committers to decide whether other refactoring is wanted.
Fair enough.
I have made the comment show that the name is optional, thank you.
The updated comment implies that IF NOT EXISTS is allowed without a
defined name, which is false:+ * CREATE STATISTICS [IF NOT EXISTS] [stats_name] [(stat types)]
A more correct version would be
+ * CREATE STATISTICS [ [IF NOT EXISTS] stats_name ]
[(stat types)]
There you go
--
Simon Riggs http://www.EnterpriseDB.com/
Attachments:
create_stats_opt_name.v5.patchapplication/octet-stream; name=create_stats_opt_name.v5.patchDownload
diff --git a/doc/src/sgml/ref/create_statistics.sgml b/doc/src/sgml/ref/create_statistics.sgml
index 9a8c904c08..a7a55553e1 100644
--- a/doc/src/sgml/ref/create_statistics.sgml
+++ b/doc/src/sgml/ref/create_statistics.sgml
@@ -21,11 +21,11 @@ PostgreSQL documentation
<refsynopsisdiv>
<synopsis>
-CREATE STATISTICS [ IF NOT EXISTS ] <replaceable class="parameter">statistics_name</replaceable>
+CREATE STATISTICS [ [ IF NOT EXISTS ] <replaceable class="parameter">statistics_name</replaceable>]
ON ( <replaceable class="parameter">expression</replaceable> )
FROM <replaceable class="parameter">table_name</replaceable>
-CREATE STATISTICS [ IF NOT EXISTS ] <replaceable class="parameter">statistics_name</replaceable>
+CREATE STATISTICS [ [ IF NOT EXISTS ] <replaceable class="parameter">statistics_name</replaceable>]
[ ( <replaceable class="parameter">statistics_kind</replaceable> [, ... ] ) ]
ON { <replaceable class="parameter">column_name</replaceable> | ( <replaceable class="parameter">expression</replaceable> ) }, { <replaceable class="parameter">column_name</replaceable> | ( <replaceable class="parameter">expression</replaceable> ) } [, ...]
FROM <replaceable class="parameter">table_name</replaceable>
@@ -60,8 +60,8 @@ CREATE STATISTICS [ IF NOT EXISTS ] <replaceable class="parameter">statistics_na
If a schema name is given (for example, <literal>CREATE STATISTICS
myschema.mystat ...</literal>) then the statistics object is created in the
specified schema. Otherwise it is created in the current schema.
- The name of the statistics object must be distinct from the name of any
- other statistics object in the same schema.
+ If given, the name of the statistics object must be distinct from the name
+ of any other statistics object in the same schema.
</para>
</refsect1>
@@ -78,6 +78,7 @@ CREATE STATISTICS [ IF NOT EXISTS ] <replaceable class="parameter">statistics_na
exists. A notice is issued in this case. Note that only the name of
the statistics object is considered here, not the details of its
definition.
+ Statistics name is required when <literal>IF NOT EXISTS</literal> is specified.
</para>
</listitem>
</varlistentry>
@@ -88,6 +89,9 @@ CREATE STATISTICS [ IF NOT EXISTS ] <replaceable class="parameter">statistics_na
<para>
The name (optionally schema-qualified) of the statistics object to be
created.
+ If the name is omitted, <productname>PostgreSQL</productname> chooses a
+ suitable name based on the parent table's name and the defined column
+ name(s) and/or expression(s).
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 969c9c158f..b566787ff1 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -4523,7 +4523,7 @@ ExistingIndex: USING INDEX name { $$ = $3; }
/*****************************************************************************
*
* QUERY :
- * CREATE STATISTICS [IF NOT EXISTS] stats_name [(stat types)]
+ * CREATE STATISTICS [ [IF NOT EXISTS] stats_name] [(stat types)]
* ON expression-list FROM from_list
*
* Note: the expectation here is that the clauses after ON are a subset of
@@ -4561,6 +4561,18 @@ CreateStatsStmt:
n->if_not_exists = true;
$$ = (Node *) n;
}
+ | CREATE STATISTICS
+ opt_name_list ON stats_params FROM from_list
+ {
+ CreateStatsStmt *n = makeNode(CreateStatsStmt);
+ n->defnames = NULL;
+ n->stat_types = $3;
+ n->exprs = $5;
+ n->relations = $7;
+ n->stxcomment = NULL;
+ n->if_not_exists = false;
+ $$ = (Node *) n;
+ }
;
/*
diff --git a/src/test/regress/expected/stats_ext.out b/src/test/regress/expected/stats_ext.out
index 042316aeed..8f5fd546eb 100644
--- a/src/test/regress/expected/stats_ext.out
+++ b/src/test/regress/expected/stats_ext.out
@@ -3062,11 +3062,11 @@ CREATE STATISTICS tststats.priv_test_stats (mcv) ON a, b
ANALYZE tststats.priv_test_tbl;
-- Check printing info about extended statistics by \dX
create table stts_t1 (a int, b int);
-create statistics stts_1 (ndistinct) on a, b from stts_t1;
-create statistics stts_2 (ndistinct, dependencies) on a, b from stts_t1;
-create statistics stts_3 (ndistinct, dependencies, mcv) on a, b from stts_t1;
+create statistics (ndistinct) on a, b from stts_t1;
+create statistics (ndistinct, dependencies) on a, b from stts_t1;
+create statistics (ndistinct, dependencies, mcv) on a, b from stts_t1;
create table stts_t2 (a int, b int, c int);
-create statistics stts_4 on b, c from stts_t2;
+create statistics on b, c from stts_t2;
create table stts_t3 (col1 int, col2 int, col3 int);
create statistics stts_hoge on col1, col2, col3 from stts_t3;
create schema stts_s1;
@@ -3084,24 +3084,24 @@ set search_path to public, stts_s1, stts_s2, tststats;
public | mcv_lists_arrays_stats | a, b, c FROM mcv_lists_arrays | | | defined
public | mcv_lists_bool_stats | a, b, c FROM mcv_lists_bool | | | defined
public | mcv_lists_stats | a, b, d FROM mcv_lists | | | defined
- public | stts_1 | a, b FROM stts_t1 | defined | |
- public | stts_2 | a, b FROM stts_t1 | defined | defined |
- public | stts_3 | a, b FROM stts_t1 | defined | defined | defined
- public | stts_4 | b, c FROM stts_t2 | defined | defined | defined
public | stts_hoge | col1, col2, col3 FROM stts_t3 | defined | defined | defined
+ public | stts_t1_a_b_stat | a, b FROM stts_t1 | defined | |
+ public | stts_t1_a_b_stat1 | a, b FROM stts_t1 | defined | defined |
+ public | stts_t1_a_b_stat2 | a, b FROM stts_t1 | defined | defined | defined
+ public | stts_t2_b_c_stat | b, c FROM stts_t2 | defined | defined | defined
stts_s1 | stts_foo | col1, col2 FROM stts_t3 | defined | defined | defined
stts_s2 | stts_yama | col1, col3 FROM stts_t3 | | defined | defined
tststats | priv_test_stats | a, b FROM priv_test_tbl | | | defined
(12 rows)
-\dX stts_?
- List of extended statistics
- Schema | Name | Definition | Ndistinct | Dependencies | MCV
---------+--------+-------------------+-----------+--------------+---------
- public | stts_1 | a, b FROM stts_t1 | defined | |
- public | stts_2 | a, b FROM stts_t1 | defined | defined |
- public | stts_3 | a, b FROM stts_t1 | defined | defined | defined
- public | stts_4 | b, c FROM stts_t2 | defined | defined | defined
+\dX stts_t*
+ List of extended statistics
+ Schema | Name | Definition | Ndistinct | Dependencies | MCV
+--------+-------------------+-------------------+-----------+--------------+---------
+ public | stts_t1_a_b_stat | a, b FROM stts_t1 | defined | |
+ public | stts_t1_a_b_stat1 | a, b FROM stts_t1 | defined | defined |
+ public | stts_t1_a_b_stat2 | a, b FROM stts_t1 | defined | defined | defined
+ public | stts_t2_b_c_stat | b, c FROM stts_t2 | defined | defined | defined
(4 rows)
\dX *stts_hoge
@@ -3119,24 +3119,24 @@ set search_path to public, stts_s1, stts_s2, tststats;
public | mcv_lists_arrays_stats | a, b, c FROM mcv_lists_arrays | | | defined
public | mcv_lists_bool_stats | a, b, c FROM mcv_lists_bool | | | defined
public | mcv_lists_stats | a, b, d FROM mcv_lists | | | defined
- public | stts_1 | a, b FROM stts_t1 | defined | |
- public | stts_2 | a, b FROM stts_t1 | defined | defined |
- public | stts_3 | a, b FROM stts_t1 | defined | defined | defined
- public | stts_4 | b, c FROM stts_t2 | defined | defined | defined
public | stts_hoge | col1, col2, col3 FROM stts_t3 | defined | defined | defined
+ public | stts_t1_a_b_stat | a, b FROM stts_t1 | defined | |
+ public | stts_t1_a_b_stat1 | a, b FROM stts_t1 | defined | defined |
+ public | stts_t1_a_b_stat2 | a, b FROM stts_t1 | defined | defined | defined
+ public | stts_t2_b_c_stat | b, c FROM stts_t2 | defined | defined | defined
stts_s1 | stts_foo | col1, col2 FROM stts_t3 | defined | defined | defined
stts_s2 | stts_yama | col1, col3 FROM stts_t3 | | defined | defined
tststats | priv_test_stats | a, b FROM priv_test_tbl | | | defined
(12 rows)
-\dX+ stts_?
- List of extended statistics
- Schema | Name | Definition | Ndistinct | Dependencies | MCV
---------+--------+-------------------+-----------+--------------+---------
- public | stts_1 | a, b FROM stts_t1 | defined | |
- public | stts_2 | a, b FROM stts_t1 | defined | defined |
- public | stts_3 | a, b FROM stts_t1 | defined | defined | defined
- public | stts_4 | b, c FROM stts_t2 | defined | defined | defined
+\dX+ stts_t*
+ List of extended statistics
+ Schema | Name | Definition | Ndistinct | Dependencies | MCV
+--------+-------------------+-------------------+-----------+--------------+---------
+ public | stts_t1_a_b_stat | a, b FROM stts_t1 | defined | |
+ public | stts_t1_a_b_stat1 | a, b FROM stts_t1 | defined | defined |
+ public | stts_t1_a_b_stat2 | a, b FROM stts_t1 | defined | defined | defined
+ public | stts_t2_b_c_stat | b, c FROM stts_t2 | defined | defined | defined
(4 rows)
\dX+ *stts_hoge
@@ -3153,6 +3153,21 @@ set search_path to public, stts_s1, stts_s2, tststats;
stts_s2 | stts_yama | col1, col3 FROM stts_t3 | | defined | defined
(1 row)
+create statistics (mcv) ON a, b, (a+b), (a-b) FROM stts_t1;
+create statistics (mcv) ON a, b, (a+b), (a-b) FROM stts_t1;
+create statistics (mcv) ON (a+b), (a-b) FROM stts_t1;
+\dX stts_t*expr*
+ List of extended statistics
+ Schema | Name | Definition | Ndistinct | Dependencies | MCV
+--------+-----------------------------+-------------------------------------+-----------+--------------+---------
+ public | stts_t1_a_b_expr_expr_stat | a, b, (a + b), (a - b) FROM stts_t1 | | | defined
+ public | stts_t1_a_b_expr_expr_stat1 | a, b, (a + b), (a - b) FROM stts_t1 | | | defined
+ public | stts_t1_expr_expr_stat | (a + b), (a - b) FROM stts_t1 | | | defined
+(3 rows)
+
+drop statistics stts_t1_a_b_expr_expr_stat;
+drop statistics stts_t1_a_b_expr_expr_stat1;
+drop statistics stts_t1_expr_expr_stat;
set search_path to public, stts_s1;
\dX
List of extended statistics
@@ -3162,11 +3177,11 @@ set search_path to public, stts_s1;
public | mcv_lists_arrays_stats | a, b, c FROM mcv_lists_arrays | | | defined
public | mcv_lists_bool_stats | a, b, c FROM mcv_lists_bool | | | defined
public | mcv_lists_stats | a, b, d FROM mcv_lists | | | defined
- public | stts_1 | a, b FROM stts_t1 | defined | |
- public | stts_2 | a, b FROM stts_t1 | defined | defined |
- public | stts_3 | a, b FROM stts_t1 | defined | defined | defined
- public | stts_4 | b, c FROM stts_t2 | defined | defined | defined
public | stts_hoge | col1, col2, col3 FROM stts_t3 | defined | defined | defined
+ public | stts_t1_a_b_stat | a, b FROM stts_t1 | defined | |
+ public | stts_t1_a_b_stat1 | a, b FROM stts_t1 | defined | defined |
+ public | stts_t1_a_b_stat2 | a, b FROM stts_t1 | defined | defined | defined
+ public | stts_t2_b_c_stat | b, c FROM stts_t2 | defined | defined | defined
stts_s1 | stts_foo | col1, col2 FROM stts_t3 | defined | defined | defined
(10 rows)
@@ -3180,11 +3195,11 @@ set role regress_stats_ext;
public | mcv_lists_arrays_stats | a, b, c FROM mcv_lists_arrays | | | defined
public | mcv_lists_bool_stats | a, b, c FROM mcv_lists_bool | | | defined
public | mcv_lists_stats | a, b, d FROM mcv_lists | | | defined
- public | stts_1 | a, b FROM stts_t1 | defined | |
- public | stts_2 | a, b FROM stts_t1 | defined | defined |
- public | stts_3 | a, b FROM stts_t1 | defined | defined | defined
- public | stts_4 | b, c FROM stts_t2 | defined | defined | defined
public | stts_hoge | col1, col2, col3 FROM stts_t3 | defined | defined | defined
+ public | stts_t1_a_b_stat | a, b FROM stts_t1 | defined | |
+ public | stts_t1_a_b_stat1 | a, b FROM stts_t1 | defined | defined |
+ public | stts_t1_a_b_stat2 | a, b FROM stts_t1 | defined | defined | defined
+ public | stts_t2_b_c_stat | b, c FROM stts_t2 | defined | defined | defined
(9 rows)
reset role;
diff --git a/src/test/regress/sql/stats_ext.sql b/src/test/regress/sql/stats_ext.sql
index 6b954c9e50..5fd865f509 100644
--- a/src/test/regress/sql/stats_ext.sql
+++ b/src/test/regress/sql/stats_ext.sql
@@ -1555,12 +1555,12 @@ ANALYZE tststats.priv_test_tbl;
-- Check printing info about extended statistics by \dX
create table stts_t1 (a int, b int);
-create statistics stts_1 (ndistinct) on a, b from stts_t1;
-create statistics stts_2 (ndistinct, dependencies) on a, b from stts_t1;
-create statistics stts_3 (ndistinct, dependencies, mcv) on a, b from stts_t1;
+create statistics (ndistinct) on a, b from stts_t1;
+create statistics (ndistinct, dependencies) on a, b from stts_t1;
+create statistics (ndistinct, dependencies, mcv) on a, b from stts_t1;
create table stts_t2 (a int, b int, c int);
-create statistics stts_4 on b, c from stts_t2;
+create statistics on b, c from stts_t2;
create table stts_t3 (col1 int, col2 int, col3 int);
create statistics stts_hoge on col1, col2, col3 from stts_t3;
@@ -1575,13 +1575,21 @@ analyze stts_t1;
set search_path to public, stts_s1, stts_s2, tststats;
\dX
-\dX stts_?
+\dX stts_t*
\dX *stts_hoge
\dX+
-\dX+ stts_?
+\dX+ stts_t*
\dX+ *stts_hoge
\dX+ stts_s2.stts_yama
+create statistics (mcv) ON a, b, (a+b), (a-b) FROM stts_t1;
+create statistics (mcv) ON a, b, (a+b), (a-b) FROM stts_t1;
+create statistics (mcv) ON (a+b), (a-b) FROM stts_t1;
+\dX stts_t*expr*
+drop statistics stts_t1_a_b_expr_expr_stat;
+drop statistics stts_t1_a_b_expr_expr_stat1;
+drop statistics stts_t1_expr_expr_stat;
+
set search_path to public, stts_s1;
\dX
On Wed, 13 Jul 2022 at 08:07, Simon Riggs <simon.riggs@enterprisedb.com> wrote:
On Thu, 7 Jul 2022 at 11:58, Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:A more correct version would be
+ * CREATE STATISTICS [ [IF NOT EXISTS] stats_name ]
[(stat types)]There you go
Thanks!
I think this is ready for a committer, so I've marked it as such.
Kind regards,
Matthias van de Meent
On Wed, 20 Jul 2022 at 12:01, Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
On Wed, 13 Jul 2022 at 08:07, Simon Riggs <simon.riggs@enterprisedb.com> wrote:
+ * CREATE STATISTICS [ [IF NOT EXISTS] stats_name ]
I think this is ready for a committer, so I've marked it as such.
Picking this up...
I tend to agree with Matthias' earlier point about avoiding code
duplication in the grammar. Without going off and refactoring other
parts of the grammar not related to this patch, it's still a slightly
smaller, simpler change, and less code duplication, to do this using a
new opt_stats_name production in the grammar, as in the attached.
I also noticed a comment in CreateStatistics() that needed updating.
Barring any further comments, I'll push this shortly.
Regards,
Dean
Attachments:
0001-Make-the-name-optional-in-CREATE-STATISTICS.patchtext/x-patch; charset=US-ASCII; name=0001-Make-the-name-optional-in-CREATE-STATISTICS.patchDownload
From 8963355b2d8451be8f71a3bd2890e99e31f7d3ff Mon Sep 17 00:00:00 2001
From: Dean Rasheed <dean.a.rasheed@gmail.com>
Date: Thu, 21 Jul 2022 14:48:28 +0100
Subject: [PATCH] Make the name optional in CREATE STATISTICS.
This allows users to omit the statistics name in a CREATE STATISTICS
command, letting the system auto-generate a sensible, unique name,
putting the statistics object in the same schema as the table.
Simon Riggs, reviewed by Matthias van de Meent.
Discussion: https://postgr.es/m/CANbhV-FGD2d_C3zFTfT2aRfX_TaPSgOeKES58RLZx5XzQp5NhA@mail.gmail.com
---
doc/src/sgml/ref/create_statistics.sgml | 12 ++--
src/backend/commands/statscmds.c | 7 +-
src/backend/parser/gram.y | 13 +++-
src/test/regress/expected/stats_ext.out | 87 +++++++++++++++----------
src/test/regress/sql/stats_ext.sql | 20 ++++--
5 files changed, 86 insertions(+), 53 deletions(-)
diff --git a/doc/src/sgml/ref/create_statistics.sgml b/doc/src/sgml/ref/create_statistics.sgml
index 9a8c904c08..b847944f37 100644
--- a/doc/src/sgml/ref/create_statistics.sgml
+++ b/doc/src/sgml/ref/create_statistics.sgml
@@ -21,11 +21,11 @@ PostgreSQL documentation
<refsynopsisdiv>
<synopsis>
-CREATE STATISTICS [ IF NOT EXISTS ] <replaceable class="parameter">statistics_name</replaceable>
+CREATE STATISTICS [ [ IF NOT EXISTS ] <replaceable class="parameter">statistics_name</replaceable> ]
ON ( <replaceable class="parameter">expression</replaceable> )
FROM <replaceable class="parameter">table_name</replaceable>
-CREATE STATISTICS [ IF NOT EXISTS ] <replaceable class="parameter">statistics_name</replaceable>
+CREATE STATISTICS [ [ IF NOT EXISTS ] <replaceable class="parameter">statistics_name</replaceable> ]
[ ( <replaceable class="parameter">statistics_kind</replaceable> [, ... ] ) ]
ON { <replaceable class="parameter">column_name</replaceable> | ( <replaceable class="parameter">expression</replaceable> ) }, { <replaceable class="parameter">column_name</replaceable> | ( <replaceable class="parameter">expression</replaceable> ) } [, ...]
FROM <replaceable class="parameter">table_name</replaceable>
@@ -60,8 +60,8 @@ CREATE STATISTICS [ IF NOT EXISTS ] <replaceable class="parameter">statistics_na
If a schema name is given (for example, <literal>CREATE STATISTICS
myschema.mystat ...</literal>) then the statistics object is created in the
specified schema. Otherwise it is created in the current schema.
- The name of the statistics object must be distinct from the name of any
- other statistics object in the same schema.
+ If given, the name of the statistics object must be distinct from the name
+ of any other statistics object in the same schema.
</para>
</refsect1>
@@ -78,6 +78,7 @@ CREATE STATISTICS [ IF NOT EXISTS ] <replaceable class="parameter">statistics_na
exists. A notice is issued in this case. Note that only the name of
the statistics object is considered here, not the details of its
definition.
+ Statistics name is required when <literal>IF NOT EXISTS</literal> is specified.
</para>
</listitem>
</varlistentry>
@@ -88,6 +89,9 @@ CREATE STATISTICS [ IF NOT EXISTS ] <replaceable class="parameter">statistics_na
<para>
The name (optionally schema-qualified) of the statistics object to be
created.
+ If the name is omitted, <productname>PostgreSQL</productname> chooses a
+ suitable name based on the parent table's name and the defined column
+ name(s) and/or expression(s).
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/commands/statscmds.c b/src/backend/commands/statscmds.c
index cd5e2f2b6b..415016969d 100644
--- a/src/backend/commands/statscmds.c
+++ b/src/backend/commands/statscmds.c
@@ -155,10 +155,9 @@ CreateStatistics(CreateStatsStmt *stmt)
/*
* If the node has a name, split it up and determine creation namespace.
- * If not (a possibility not considered by the grammar, but one which can
- * occur via the "CREATE TABLE ... (LIKE)" command), then we put the
- * object in the same namespace as the relation, and cons up a name for
- * it.
+ * If not, put the object in the same namespace as the relation, and cons
+ * up a name for it. (This can happen either via "CREATE STATISTICS ..."
+ * or via "CREATE TABLE ... (LIKE)".)
*/
if (stmt->defnames)
namespaceId = QualifiedNameGetCreationNamespace(stmt->defnames,
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index d649a1b8d1..0a874a04aa 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -434,7 +434,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
old_aggr_definition old_aggr_list
oper_argtypes RuleActionList RuleActionMulti
opt_column_list columnList opt_name_list
- sort_clause opt_sort_clause sortby_list index_params stats_params
+ sort_clause opt_sort_clause sortby_list index_params
+ opt_stats_name stats_params
opt_include opt_c_include index_including_params
name_list role_list from_clause from_list opt_array_bounds
qualified_name_list any_name any_name_list type_name_list
@@ -4533,7 +4534,7 @@ ExistingIndex: USING INDEX name { $$ = $3; }
/*****************************************************************************
*
* QUERY :
- * CREATE STATISTICS [IF NOT EXISTS] stats_name [(stat types)]
+ * CREATE STATISTICS [[IF NOT EXISTS] stats_name] [(stat types)]
* ON expression-list FROM from_list
*
* Note: the expectation here is that the clauses after ON are a subset of
@@ -4545,7 +4546,7 @@ ExistingIndex: USING INDEX name { $$ = $3; }
*****************************************************************************/
CreateStatsStmt:
- CREATE STATISTICS any_name
+ CREATE STATISTICS opt_stats_name
opt_name_list ON stats_params FROM from_list
{
CreateStatsStmt *n = makeNode(CreateStatsStmt);
@@ -4573,6 +4574,12 @@ CreateStatsStmt:
}
;
+/* Statistics name is optional unless IF NOT EXISTS is specified */
+opt_stats_name:
+ any_name { $$ = $1; }
+ | /*EMPTY*/ { $$ = NULL; }
+ ;
+
/*
* Statistics attributes can be either simple column references, or arbitrary
* expressions in parens. For compatibility with index attributes permitted
diff --git a/src/test/regress/expected/stats_ext.out b/src/test/regress/expected/stats_ext.out
index 042316aeed..8f5fd546eb 100644
--- a/src/test/regress/expected/stats_ext.out
+++ b/src/test/regress/expected/stats_ext.out
@@ -3062,11 +3062,11 @@ CREATE STATISTICS tststats.priv_test_stats (mcv) ON a, b
ANALYZE tststats.priv_test_tbl;
-- Check printing info about extended statistics by \dX
create table stts_t1 (a int, b int);
-create statistics stts_1 (ndistinct) on a, b from stts_t1;
-create statistics stts_2 (ndistinct, dependencies) on a, b from stts_t1;
-create statistics stts_3 (ndistinct, dependencies, mcv) on a, b from stts_t1;
+create statistics (ndistinct) on a, b from stts_t1;
+create statistics (ndistinct, dependencies) on a, b from stts_t1;
+create statistics (ndistinct, dependencies, mcv) on a, b from stts_t1;
create table stts_t2 (a int, b int, c int);
-create statistics stts_4 on b, c from stts_t2;
+create statistics on b, c from stts_t2;
create table stts_t3 (col1 int, col2 int, col3 int);
create statistics stts_hoge on col1, col2, col3 from stts_t3;
create schema stts_s1;
@@ -3084,24 +3084,24 @@ set search_path to public, stts_s1, stts_s2, tststats;
public | mcv_lists_arrays_stats | a, b, c FROM mcv_lists_arrays | | | defined
public | mcv_lists_bool_stats | a, b, c FROM mcv_lists_bool | | | defined
public | mcv_lists_stats | a, b, d FROM mcv_lists | | | defined
- public | stts_1 | a, b FROM stts_t1 | defined | |
- public | stts_2 | a, b FROM stts_t1 | defined | defined |
- public | stts_3 | a, b FROM stts_t1 | defined | defined | defined
- public | stts_4 | b, c FROM stts_t2 | defined | defined | defined
public | stts_hoge | col1, col2, col3 FROM stts_t3 | defined | defined | defined
+ public | stts_t1_a_b_stat | a, b FROM stts_t1 | defined | |
+ public | stts_t1_a_b_stat1 | a, b FROM stts_t1 | defined | defined |
+ public | stts_t1_a_b_stat2 | a, b FROM stts_t1 | defined | defined | defined
+ public | stts_t2_b_c_stat | b, c FROM stts_t2 | defined | defined | defined
stts_s1 | stts_foo | col1, col2 FROM stts_t3 | defined | defined | defined
stts_s2 | stts_yama | col1, col3 FROM stts_t3 | | defined | defined
tststats | priv_test_stats | a, b FROM priv_test_tbl | | | defined
(12 rows)
-\dX stts_?
- List of extended statistics
- Schema | Name | Definition | Ndistinct | Dependencies | MCV
---------+--------+-------------------+-----------+--------------+---------
- public | stts_1 | a, b FROM stts_t1 | defined | |
- public | stts_2 | a, b FROM stts_t1 | defined | defined |
- public | stts_3 | a, b FROM stts_t1 | defined | defined | defined
- public | stts_4 | b, c FROM stts_t2 | defined | defined | defined
+\dX stts_t*
+ List of extended statistics
+ Schema | Name | Definition | Ndistinct | Dependencies | MCV
+--------+-------------------+-------------------+-----------+--------------+---------
+ public | stts_t1_a_b_stat | a, b FROM stts_t1 | defined | |
+ public | stts_t1_a_b_stat1 | a, b FROM stts_t1 | defined | defined |
+ public | stts_t1_a_b_stat2 | a, b FROM stts_t1 | defined | defined | defined
+ public | stts_t2_b_c_stat | b, c FROM stts_t2 | defined | defined | defined
(4 rows)
\dX *stts_hoge
@@ -3119,24 +3119,24 @@ set search_path to public, stts_s1, stts_s2, tststats;
public | mcv_lists_arrays_stats | a, b, c FROM mcv_lists_arrays | | | defined
public | mcv_lists_bool_stats | a, b, c FROM mcv_lists_bool | | | defined
public | mcv_lists_stats | a, b, d FROM mcv_lists | | | defined
- public | stts_1 | a, b FROM stts_t1 | defined | |
- public | stts_2 | a, b FROM stts_t1 | defined | defined |
- public | stts_3 | a, b FROM stts_t1 | defined | defined | defined
- public | stts_4 | b, c FROM stts_t2 | defined | defined | defined
public | stts_hoge | col1, col2, col3 FROM stts_t3 | defined | defined | defined
+ public | stts_t1_a_b_stat | a, b FROM stts_t1 | defined | |
+ public | stts_t1_a_b_stat1 | a, b FROM stts_t1 | defined | defined |
+ public | stts_t1_a_b_stat2 | a, b FROM stts_t1 | defined | defined | defined
+ public | stts_t2_b_c_stat | b, c FROM stts_t2 | defined | defined | defined
stts_s1 | stts_foo | col1, col2 FROM stts_t3 | defined | defined | defined
stts_s2 | stts_yama | col1, col3 FROM stts_t3 | | defined | defined
tststats | priv_test_stats | a, b FROM priv_test_tbl | | | defined
(12 rows)
-\dX+ stts_?
- List of extended statistics
- Schema | Name | Definition | Ndistinct | Dependencies | MCV
---------+--------+-------------------+-----------+--------------+---------
- public | stts_1 | a, b FROM stts_t1 | defined | |
- public | stts_2 | a, b FROM stts_t1 | defined | defined |
- public | stts_3 | a, b FROM stts_t1 | defined | defined | defined
- public | stts_4 | b, c FROM stts_t2 | defined | defined | defined
+\dX+ stts_t*
+ List of extended statistics
+ Schema | Name | Definition | Ndistinct | Dependencies | MCV
+--------+-------------------+-------------------+-----------+--------------+---------
+ public | stts_t1_a_b_stat | a, b FROM stts_t1 | defined | |
+ public | stts_t1_a_b_stat1 | a, b FROM stts_t1 | defined | defined |
+ public | stts_t1_a_b_stat2 | a, b FROM stts_t1 | defined | defined | defined
+ public | stts_t2_b_c_stat | b, c FROM stts_t2 | defined | defined | defined
(4 rows)
\dX+ *stts_hoge
@@ -3153,6 +3153,21 @@ set search_path to public, stts_s1, stts_s2, tststats;
stts_s2 | stts_yama | col1, col3 FROM stts_t3 | | defined | defined
(1 row)
+create statistics (mcv) ON a, b, (a+b), (a-b) FROM stts_t1;
+create statistics (mcv) ON a, b, (a+b), (a-b) FROM stts_t1;
+create statistics (mcv) ON (a+b), (a-b) FROM stts_t1;
+\dX stts_t*expr*
+ List of extended statistics
+ Schema | Name | Definition | Ndistinct | Dependencies | MCV
+--------+-----------------------------+-------------------------------------+-----------+--------------+---------
+ public | stts_t1_a_b_expr_expr_stat | a, b, (a + b), (a - b) FROM stts_t1 | | | defined
+ public | stts_t1_a_b_expr_expr_stat1 | a, b, (a + b), (a - b) FROM stts_t1 | | | defined
+ public | stts_t1_expr_expr_stat | (a + b), (a - b) FROM stts_t1 | | | defined
+(3 rows)
+
+drop statistics stts_t1_a_b_expr_expr_stat;
+drop statistics stts_t1_a_b_expr_expr_stat1;
+drop statistics stts_t1_expr_expr_stat;
set search_path to public, stts_s1;
\dX
List of extended statistics
@@ -3162,11 +3177,11 @@ set search_path to public, stts_s1;
public | mcv_lists_arrays_stats | a, b, c FROM mcv_lists_arrays | | | defined
public | mcv_lists_bool_stats | a, b, c FROM mcv_lists_bool | | | defined
public | mcv_lists_stats | a, b, d FROM mcv_lists | | | defined
- public | stts_1 | a, b FROM stts_t1 | defined | |
- public | stts_2 | a, b FROM stts_t1 | defined | defined |
- public | stts_3 | a, b FROM stts_t1 | defined | defined | defined
- public | stts_4 | b, c FROM stts_t2 | defined | defined | defined
public | stts_hoge | col1, col2, col3 FROM stts_t3 | defined | defined | defined
+ public | stts_t1_a_b_stat | a, b FROM stts_t1 | defined | |
+ public | stts_t1_a_b_stat1 | a, b FROM stts_t1 | defined | defined |
+ public | stts_t1_a_b_stat2 | a, b FROM stts_t1 | defined | defined | defined
+ public | stts_t2_b_c_stat | b, c FROM stts_t2 | defined | defined | defined
stts_s1 | stts_foo | col1, col2 FROM stts_t3 | defined | defined | defined
(10 rows)
@@ -3180,11 +3195,11 @@ set role regress_stats_ext;
public | mcv_lists_arrays_stats | a, b, c FROM mcv_lists_arrays | | | defined
public | mcv_lists_bool_stats | a, b, c FROM mcv_lists_bool | | | defined
public | mcv_lists_stats | a, b, d FROM mcv_lists | | | defined
- public | stts_1 | a, b FROM stts_t1 | defined | |
- public | stts_2 | a, b FROM stts_t1 | defined | defined |
- public | stts_3 | a, b FROM stts_t1 | defined | defined | defined
- public | stts_4 | b, c FROM stts_t2 | defined | defined | defined
public | stts_hoge | col1, col2, col3 FROM stts_t3 | defined | defined | defined
+ public | stts_t1_a_b_stat | a, b FROM stts_t1 | defined | |
+ public | stts_t1_a_b_stat1 | a, b FROM stts_t1 | defined | defined |
+ public | stts_t1_a_b_stat2 | a, b FROM stts_t1 | defined | defined | defined
+ public | stts_t2_b_c_stat | b, c FROM stts_t2 | defined | defined | defined
(9 rows)
reset role;
diff --git a/src/test/regress/sql/stats_ext.sql b/src/test/regress/sql/stats_ext.sql
index 6b954c9e50..5fd865f509 100644
--- a/src/test/regress/sql/stats_ext.sql
+++ b/src/test/regress/sql/stats_ext.sql
@@ -1555,12 +1555,12 @@ ANALYZE tststats.priv_test_tbl;
-- Check printing info about extended statistics by \dX
create table stts_t1 (a int, b int);
-create statistics stts_1 (ndistinct) on a, b from stts_t1;
-create statistics stts_2 (ndistinct, dependencies) on a, b from stts_t1;
-create statistics stts_3 (ndistinct, dependencies, mcv) on a, b from stts_t1;
+create statistics (ndistinct) on a, b from stts_t1;
+create statistics (ndistinct, dependencies) on a, b from stts_t1;
+create statistics (ndistinct, dependencies, mcv) on a, b from stts_t1;
create table stts_t2 (a int, b int, c int);
-create statistics stts_4 on b, c from stts_t2;
+create statistics on b, c from stts_t2;
create table stts_t3 (col1 int, col2 int, col3 int);
create statistics stts_hoge on col1, col2, col3 from stts_t3;
@@ -1575,13 +1575,21 @@ analyze stts_t1;
set search_path to public, stts_s1, stts_s2, tststats;
\dX
-\dX stts_?
+\dX stts_t*
\dX *stts_hoge
\dX+
-\dX+ stts_?
+\dX+ stts_t*
\dX+ *stts_hoge
\dX+ stts_s2.stts_yama
+create statistics (mcv) ON a, b, (a+b), (a-b) FROM stts_t1;
+create statistics (mcv) ON a, b, (a+b), (a-b) FROM stts_t1;
+create statistics (mcv) ON (a+b), (a-b) FROM stts_t1;
+\dX stts_t*expr*
+drop statistics stts_t1_a_b_expr_expr_stat;
+drop statistics stts_t1_a_b_expr_expr_stat1;
+drop statistics stts_t1_expr_expr_stat;
+
set search_path to public, stts_s1;
\dX
--
2.35.3
On Thu, 21 Jul 2022 at 15:12, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On Wed, 20 Jul 2022 at 12:01, Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:On Wed, 13 Jul 2022 at 08:07, Simon Riggs <simon.riggs@enterprisedb.com> wrote:
+ * CREATE STATISTICS [ [IF NOT EXISTS] stats_name ]
I think this is ready for a committer, so I've marked it as such.
Picking this up...
I tend to agree with Matthias' earlier point about avoiding code
duplication in the grammar. Without going off and refactoring other
parts of the grammar not related to this patch, it's still a slightly
smaller, simpler change, and less code duplication, to do this using a
new opt_stats_name production in the grammar, as in the attached.I also noticed a comment in CreateStatistics() that needed updating.
Barring any further comments, I'll push this shortly.
Nice change, please proceed.
--
Simon Riggs http://www.EnterpriseDB.com/
On 7/21/22 16:12, Dean Rasheed wrote:
On Wed, 20 Jul 2022 at 12:01, Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:On Wed, 13 Jul 2022 at 08:07, Simon Riggs <simon.riggs@enterprisedb.com> wrote:
+ * CREATE STATISTICS [ [IF NOT EXISTS] stats_name ]
I think this is ready for a committer, so I've marked it as such.
Picking this up...
I tend to agree with Matthias' earlier point about avoiding code
duplication in the grammar. Without going off and refactoring other
parts of the grammar not related to this patch, it's still a slightly
smaller, simpler change, and less code duplication, to do this using a
new opt_stats_name production in the grammar, as in the attached.I also noticed a comment in CreateStatistics() that needed updating.
Barring any further comments, I'll push this shortly.
+1
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 2022-Jul-21, Dean Rasheed wrote:
I tend to agree with Matthias' earlier point about avoiding code
duplication in the grammar. Without going off and refactoring other
parts of the grammar not related to this patch, it's still a slightly
smaller, simpler change, and less code duplication, to do this using a
new opt_stats_name production in the grammar, as in the attached.I also noticed a comment in CreateStatistics() that needed updating.
Barring any further comments, I'll push this shortly.
Thanks. I was looking at the recently modified REINDEX syntax and
noticed there another spot for taking an optional name. I ended up
reusing OptSchemaName for that, as in the attached patch. I think
adding production-specific additional productions is pointless and
probably bloats the grammar. So let me +1 your push of the patch you
posted, just to keep things moving forward, but in addition I propose to
later rename OptSchemaName to something more generic and use it in these
three places.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
Attachments:
0001-Rework-grammar-for-REINDEX.patchtext/x-diff; charset=us-asciiDownload
From 143467419c19aea6a79db46da461aae4d9afabac Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Thu, 21 Jul 2022 16:48:51 +0200
Subject: [PATCH] Rework grammar for REINDEX
---
src/backend/parser/gram.y | 80 +++++++++++-----------
src/test/regress/expected/create_index.out | 4 +-
2 files changed, 42 insertions(+), 42 deletions(-)
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index d649a1b8d1..85ab17ca5a 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -560,7 +560,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type <defelt> generic_option_elem alter_generic_option_elem
%type <list> generic_option_list alter_generic_option_list
-%type <ival> reindex_target_type reindex_target_multitable reindex_name_optional
+%type <ival> reindex_target_type reindex_target_type_multi
%type <node> copy_generic_opt_arg copy_generic_opt_arg_list_item
%type <defelt> copy_generic_opt_elem
@@ -9085,7 +9085,9 @@ DropTransformStmt: DROP TRANSFORM opt_if_exists FOR Typename LANGUAGE name opt_d
*
* QUERY:
*
- * REINDEX [ (options) ] type [CONCURRENTLY] <name>
+ * REINDEX [ (options) ] {TABLE | INDEX} [CONCURRENTLY] <name>
+ * REINDEX [ (options) ] SCHEMA [CONCURRENTLY] <name>
+ * REINDEX [ (options) ] {SYSTEM | DATABASE} [<name>]
*****************************************************************************/
ReindexStmt:
@@ -9102,37 +9104,6 @@ ReindexStmt:
makeDefElem("concurrently", NULL, @3));
$$ = (Node *) n;
}
- | REINDEX reindex_target_multitable opt_concurrently name
- {
- ReindexStmt *n = makeNode(ReindexStmt);
-
- n->kind = $2;
- n->name = $4;
- n->relation = NULL;
- n->params = NIL;
- if ($3)
- n->params = lappend(n->params,
- makeDefElem("concurrently", NULL, @3));
- $$ = (Node *) n;
- }
- | REINDEX reindex_name_optional
- {
- ReindexStmt *n = makeNode(ReindexStmt);
- n->kind = $2;
- n->name = NULL;
- n->relation = NULL;
- n->params = NIL;
- $$ = (Node *)n;
- }
- | REINDEX '(' utility_option_list ')' reindex_name_optional
- {
- ReindexStmt *n = makeNode(ReindexStmt);
- n->kind = $5;
- n->name = NULL;
- n->relation = NULL;
- n->params = $3;
- $$ = (Node *)n;
- }
| REINDEX '(' utility_option_list ')' reindex_target_type opt_concurrently qualified_name
{
ReindexStmt *n = makeNode(ReindexStmt);
@@ -9146,11 +9117,25 @@ ReindexStmt:
makeDefElem("concurrently", NULL, @6));
$$ = (Node *) n;
}
- | REINDEX '(' utility_option_list ')' reindex_target_multitable opt_concurrently name
+
+ | REINDEX SCHEMA opt_concurrently name
{
ReindexStmt *n = makeNode(ReindexStmt);
- n->kind = $5;
+ n->kind = REINDEX_OBJECT_SCHEMA;
+ n->name = $4;
+ n->relation = NULL;
+ n->params = NIL;
+ if ($3)
+ n->params = lappend(n->params,
+ makeDefElem("concurrently", NULL, @3));
+ $$ = (Node *) n;
+ }
+ | REINDEX '(' utility_option_list ')' SCHEMA opt_concurrently name
+ {
+ ReindexStmt *n = makeNode(ReindexStmt);
+
+ n->kind = REINDEX_OBJECT_SCHEMA;
n->name = $7;
n->relation = NULL;
n->params = $3;
@@ -9159,18 +9144,31 @@ ReindexStmt:
makeDefElem("concurrently", NULL, @6));
$$ = (Node *) n;
}
+ | REINDEX reindex_target_type_multi OptSchemaName
+ {
+ ReindexStmt *n = makeNode(ReindexStmt);
+ n->kind = $2;
+ n->name = NULL;
+ n->relation = NULL;
+ n->params = NIL;
+ $$ = (Node *)n;
+ }
+ | REINDEX '(' utility_option_list ')' reindex_target_type_multi OptSchemaName
+ {
+ ReindexStmt *n = makeNode(ReindexStmt);
+ n->kind = $5;
+ n->name = $6;
+ n->relation = NULL;
+ n->params = $3;
+ $$ = (Node *)n;
+ }
;
reindex_target_type:
INDEX { $$ = REINDEX_OBJECT_INDEX; }
| TABLE { $$ = REINDEX_OBJECT_TABLE; }
;
-reindex_target_multitable:
- SCHEMA { $$ = REINDEX_OBJECT_SCHEMA; }
- | SYSTEM_P { $$ = REINDEX_OBJECT_SYSTEM; }
- | DATABASE { $$ = REINDEX_OBJECT_DATABASE; }
- ;
/* For these options the name is optional */
-reindex_name_optional:
+reindex_target_type_multi:
SYSTEM_P { $$ = REINDEX_OBJECT_SYSTEM; }
| DATABASE { $$ = REINDEX_OBJECT_DATABASE; }
;
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index d55aec3a1d..d53af31753 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2521,7 +2521,9 @@ ERROR: cannot reindex system catalogs concurrently
REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1260_index; -- no catalog toast index
ERROR: cannot reindex system catalogs concurrently
REINDEX SYSTEM CONCURRENTLY postgres; -- not allowed for SYSTEM
-ERROR: cannot reindex system catalogs concurrently
+ERROR: syntax error at or near "CONCURRENTLY"
+LINE 1: REINDEX SYSTEM CONCURRENTLY postgres;
+ ^
-- Warns about catalog relations
REINDEX SCHEMA CONCURRENTLY pg_catalog;
WARNING: cannot reindex system catalogs concurrently, skipping all
--
2.30.2
On Thu, 21 Jul 2022 at 18:42, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
Thanks. I was looking at the recently modified REINDEX syntax and
noticed there another spot for taking an optional name. I ended up
reusing OptSchemaName for that, as in the attached patch. I think
adding production-specific additional productions is pointless and
probably bloats the grammar. So let me +1 your push of the patch you
posted, just to keep things moving forward, but in addition I propose to
later rename OptSchemaName to something more generic and use it in these
three places.
OK, pushed.
Before writing opt_stats_name, I went looking for anything else I
could use, but didn't see anything. The difference between this and
the index case, is that statistics objects can be schema-qualified, so
it might be tricky to get something that'll work for all 3 places.
Regards,
Dean
On Thu, Jul 21, 2022 at 07:42:12PM +0200, Alvaro Herrera wrote:
Thanks. I was looking at the recently modified REINDEX syntax and
noticed there another spot for taking an optional name. I ended up
reusing OptSchemaName for that, as in the attached patch. I think
adding production-specific additional productions is pointless and
probably bloats the grammar. So let me +1 your push of the patch you
posted, just to keep things moving forward, but in addition I propose to
later rename OptSchemaName to something more generic and use it in these
three places.
This slightly changes the behavior of the grammar, as CONCURRENTLY
was working on DATABASE as follows:
* On HEAD:
=# reindex database concurrently postgres;
REINDEX
=# reindex (concurrently) database concurrently postgres;
REINDEX
=# reindex (concurrently) database ;
REINDEX
=# reindex (concurrently) database postgres;
REINDEX
=# reindex database concurrently postgres;
REINDEX
=# reindex database concurrently;
ERROR: 42601: syntax error at or near ";"
And actually, even on HEAD, the last case is marked as supported by
the docs but we don't allow it in the parser. My mistake on this
one.
Now, with the patch, I get:
=# reindex (concurrently) database concurrently;
ERROR: 42601: syntax error at or near "concurrently"
LINE 1: reindex (concurrently) database concurrently postgres ;
=# reindex (concurrently) database concurrently postgres;
ERROR: 42601: syntax error at or near "concurrently"
LINE 1: reindex (concurrently) database concurrently postgres;
=# reindex (concurrently) database ;
REINDEX
=# reindex (concurrently) database postgres;
REINDEX
=# reindex database concurrently postgres;
ERROR: 42601: syntax error at or near "concurrently"
LINE 1: reindex database concurrently postgres;
=# reindex database concurrently;
ERROR: 42601: syntax error at or near "concurrently"
So this indeed has as effect to make possible the use of CONCURRENTLY
for DATABASE and SYSTEM only within the parenthesized grammar. Seeing
the simplifications this creates, I'd agree with dropping this part of
the grammar. I think that I would add the following queries to
create_index.sql to test this grammar, as we can rely on this code
path generating an error:
REINDEX (CONCURRENTLY) SYSTEM postgres;
REINDEX (CONCURRENTLY) SYSTEM;
At least it would validate the parsing for DATABASE.
This breaks reindexdb for the database case, because the query
generated in run_reindex_command() adds CONCURRENTLY only *after* the
database name, and we should be careful to generate something
backward-compatible in this tool, as well. The fix is simple: you
can add CONCURRENTLY within the section with TABLESPACE and VERBOSE
for a connection >= 14, and use it after the object name for <= 13.
--
Michael
On 2022-Jul-22, Michael Paquier wrote:
So this indeed has as effect to make possible the use of CONCURRENTLY
for DATABASE and SYSTEM only within the parenthesized grammar. Seeing
the simplifications this creates, I'd agree with dropping this part of
the grammar.
Actually, looking at the grammar again I realized that the '('options')'
part could be refactored; and with that, keeping an extra production for
REINDEX DATABASE CONCURRENTLY is short enough. It is removed from
REINDEX SYSTEM, but that's OK because that doesn't work anyway.
I added the new test lines you proposed and amended the docs; the result
is attached.
Initially I wanted to use the "optional list of options" for all
utilities that have similar constructions, (VACUUM, ANALYZE, CLUSTER,
EXPLAIN) but it is not possible because their alternative productions
accept different keywords, so it doesn't look possible.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"If you have nothing to say, maybe you need just the right tool to help you
not say it." (New York Times, about Microsoft PowerPoint)
Attachments:
v2-0001-Rework-grammar-for-REINDEX.patchtext/x-diff; charset=us-asciiDownload
From 1c5f90b475b94f623a91fe10514e69cd8ffcc38a Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Thu, 21 Jul 2022 16:48:51 +0200
Subject: [PATCH v2] Rework grammar for REINDEX
---
doc/src/sgml/ref/reindex.sgml | 3 +-
src/backend/parser/gram.y | 87 ++++++++--------------
src/test/regress/expected/create_index.out | 6 ++
src/test/regress/sql/create_index.sql | 2 +
4 files changed, 41 insertions(+), 57 deletions(-)
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index fcbda88149..6750eb6e47 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -22,7 +22,8 @@ PostgreSQL documentation
<refsynopsisdiv>
<synopsis>
REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { INDEX | TABLE | SCHEMA } [ CONCURRENTLY ] <replaceable class="parameter">name</replaceable>
-REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { DATABASE | SYSTEM } [ CONCURRENTLY ] [ <replaceable class="parameter">name</replaceable> ]
+REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] DATABASE [ CONCURRENTLY ] [ <replaceable class="parameter">name</replaceable> ]
+REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] SYSTEM [ <replaceable class="parameter">name</replaceable> ]
<phrase>where <replaceable class="parameter">option</replaceable> can be one of:</phrase>
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index b761a5b5d2..c6f74bf24e 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -564,7 +564,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type <defelt> generic_option_elem alter_generic_option_elem
%type <list> generic_option_list alter_generic_option_list
-%type <ival> reindex_target_type reindex_target_multitable reindex_name_optional
+%type <ival> reindex_target_type
+%type <list> opt_reindex_option_list
%type <node> copy_generic_opt_arg copy_generic_opt_arg_list_item
%type <defelt> copy_generic_opt_elem
@@ -9091,95 +9092,69 @@ DropTransformStmt: DROP TRANSFORM opt_if_exists FOR Typename LANGUAGE name opt_d
*
* QUERY:
*
- * REINDEX [ (options) ] type [CONCURRENTLY] <name>
+ * REINDEX [ (options) ] {TABLE | INDEX | SCHEMA} [CONCURRENTLY] <name>
+ * REINDEX [ (options) ] DATABASE [CONCURRENTLY] [<name>]
+ * REINDEX [ (options) ] SYSTEM [<name>]
*****************************************************************************/
ReindexStmt:
- REINDEX reindex_target_type opt_concurrently qualified_name
+ REINDEX opt_reindex_option_list reindex_target_type opt_concurrently qualified_name
{
ReindexStmt *n = makeNode(ReindexStmt);
- n->kind = $2;
- n->relation = $4;
+ n->kind = $3;
+ n->relation = $5;
n->name = NULL;
- n->params = NIL;
- if ($3)
+ n->params = $2;
+ if ($4)
n->params = lappend(n->params,
- makeDefElem("concurrently", NULL, @3));
+ makeDefElem("concurrently", NULL, @4));
$$ = (Node *) n;
}
- | REINDEX reindex_target_multitable opt_concurrently name
+ | REINDEX opt_reindex_option_list SCHEMA opt_concurrently name
{
ReindexStmt *n = makeNode(ReindexStmt);
- n->kind = $2;
- n->name = $4;
+ n->kind = REINDEX_OBJECT_SCHEMA;
+ n->name = $5;
n->relation = NULL;
- n->params = NIL;
- if ($3)
+ n->params = $2;
+ if ($4)
n->params = lappend(n->params,
- makeDefElem("concurrently", NULL, @3));
+ makeDefElem("concurrently", NULL, @4));
$$ = (Node *) n;
}
- | REINDEX reindex_name_optional
+ | REINDEX opt_reindex_option_list DATABASE opt_concurrently opt_single_name
{
ReindexStmt *n = makeNode(ReindexStmt);
- n->kind = $2;
+ n->kind = REINDEX_OBJECT_DATABASE;
n->name = NULL;
n->relation = NULL;
- n->params = NIL;
+ n->params = $2;
$$ = (Node *)n;
}
- | REINDEX '(' utility_option_list ')' reindex_name_optional
+ | REINDEX opt_reindex_option_list SYSTEM_P opt_single_name
{
ReindexStmt *n = makeNode(ReindexStmt);
- n->kind = $5;
+ n->kind = REINDEX_OBJECT_SYSTEM;
n->name = NULL;
n->relation = NULL;
- n->params = $3;
+ n->params = $2;
$$ = (Node *)n;
}
- | REINDEX '(' utility_option_list ')' reindex_target_type opt_concurrently qualified_name
- {
- ReindexStmt *n = makeNode(ReindexStmt);
-
- n->kind = $5;
- n->relation = $7;
- n->name = NULL;
- n->params = $3;
- if ($6)
- n->params = lappend(n->params,
- makeDefElem("concurrently", NULL, @6));
- $$ = (Node *) n;
- }
- | REINDEX '(' utility_option_list ')' reindex_target_multitable opt_concurrently name
- {
- ReindexStmt *n = makeNode(ReindexStmt);
-
- n->kind = $5;
- n->name = $7;
- n->relation = NULL;
- n->params = $3;
- if ($6)
- n->params = lappend(n->params,
- makeDefElem("concurrently", NULL, @6));
- $$ = (Node *) n;
- }
;
reindex_target_type:
INDEX { $$ = REINDEX_OBJECT_INDEX; }
| TABLE { $$ = REINDEX_OBJECT_TABLE; }
;
-reindex_target_multitable:
- SCHEMA { $$ = REINDEX_OBJECT_SCHEMA; }
- | SYSTEM_P { $$ = REINDEX_OBJECT_SYSTEM; }
- | DATABASE { $$ = REINDEX_OBJECT_DATABASE; }
- ;
-/* For these options the name is optional */
-reindex_name_optional:
- SYSTEM_P { $$ = REINDEX_OBJECT_SYSTEM; }
- | DATABASE { $$ = REINDEX_OBJECT_DATABASE; }
- ;
+opt_reindex_option_list:
+ '(' utility_option_list ')'
+ {
+ $$ = $2;
+ }
+ | /* EMPTY */
+ { $$ = NULL; }
+ ;
/*****************************************************************************
*
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index d55aec3a1d..a913a1846f 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2521,6 +2521,12 @@ ERROR: cannot reindex system catalogs concurrently
REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1260_index; -- no catalog toast index
ERROR: cannot reindex system catalogs concurrently
REINDEX SYSTEM CONCURRENTLY postgres; -- not allowed for SYSTEM
+ERROR: syntax error at or near "CONCURRENTLY"
+LINE 1: REINDEX SYSTEM CONCURRENTLY postgres;
+ ^
+REINDEX (CONCURRENTLY) SYSTEM postgres; -- ditto
+ERROR: cannot reindex system catalogs concurrently
+REINDEX (CONCURRENTLY) SYSTEM; -- ditto
ERROR: cannot reindex system catalogs concurrently
-- Warns about catalog relations
REINDEX SCHEMA CONCURRENTLY pg_catalog;
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index d8fded3d93..4b75790e47 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -1072,6 +1072,8 @@ REINDEX INDEX CONCURRENTLY pg_class_oid_index; -- no catalog index
REINDEX TABLE CONCURRENTLY pg_toast.pg_toast_1260; -- no catalog toast table
REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1260_index; -- no catalog toast index
REINDEX SYSTEM CONCURRENTLY postgres; -- not allowed for SYSTEM
+REINDEX (CONCURRENTLY) SYSTEM postgres; -- ditto
+REINDEX (CONCURRENTLY) SYSTEM; -- ditto
-- Warns about catalog relations
REINDEX SCHEMA CONCURRENTLY pg_catalog;
--
2.30.2
On Fri, Jul 22, 2022 at 03:06:46PM +0200, Alvaro Herrera wrote:
Actually, looking at the grammar again I realized that the '('options')'
part could be refactored; and with that, keeping an extra production for
REINDEX DATABASE CONCURRENTLY is short enough. It is removed from
REINDEX SYSTEM, but that's OK because that doesn't work anyway.
I have just looked at 83011ce, and got what you've done here. You
have thrown away reindex_target_multitable and added three parts for
SCHEMA, DATABASE and SYSTEM instead with their own options, enforcing
the restriction on CONCURRENTLY at the end of REINDEX SYSTEM in the
parser rather than indexcmds.c.
+ | REINDEX opt_reindex_option_list SYSTEM_P opt_single_name
{
ReindexStmt *n =
makeNode(ReindexStmt);
-
- n->kind = $5;
- n->name = $7;
+ n->kind = REINDEX_OBJECT_SYSTEM;
+ n->name = NULL;I think that there is a bug in this logic. ReindexStmt->name is
always set to NULL, meaning that a REINDEX command with any database
name would still pass, but I don't think that we should allow that.
For example, with something like these commands, we should complain
that the database name specified does not match with the database we
are connected to:
=# reindex system popo_foo_bar;
REINDEX
=# reindex database popo_foo_bar;
REINDEX
It may have been better to wait a bit if you wanted me to look at all
that, as our timezones are not compatible, especially on Fridays, but
that's fine :D.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
I have just looked at 83011ce, and got what you've done here. You
have thrown away reindex_target_multitable and added three parts for
SCHEMA, DATABASE and SYSTEM instead with their own options, enforcing
the restriction on CONCURRENTLY at the end of REINDEX SYSTEM in the
parser rather than indexcmds.c.
That does not seem like an improvement. In v15:
regression=# REINDEX SYSTEM CONCURRENTLY db;
ERROR: cannot reindex system catalogs concurrently
As of HEAD:
regression=# REINDEX SYSTEM CONCURRENTLY db;
ERROR: syntax error at or near "CONCURRENTLY"
LINE 1: REINDEX SYSTEM CONCURRENTLY db;
^
That is not a very helpful error, not even if the man page
doesn't show the syntax as legal.
regards, tom lane
On Fri, Jul 22, 2022 at 11:54:27PM -0400, Tom Lane wrote:
That does not seem like an improvement. In v15:
regression=# REINDEX SYSTEM CONCURRENTLY db;
ERROR: cannot reindex system catalogs concurrentlyAs of HEAD:
regression=# REINDEX SYSTEM CONCURRENTLY db;
ERROR: syntax error at or near "CONCURRENTLY"
LINE 1: REINDEX SYSTEM CONCURRENTLY db;
^That is not a very helpful error, not even if the man page
doesn't show the syntax as legal.
As the problem comes down to the fact that INDEX/TABLE, SCHEMA and
DATABASE/SYSTEM need to handle names for different object types each,
I think that we could do something like the attached, removing one
block on the way at the cost of an extra parser node.
By the way, it seems that 83011ce also broke the case of "REINDEX
DATABASE CONCURRENTLY", where the parser missed the addition of a
DefElem for "concurrently" in this case.
--
Michael
Attachments:
reindex_syntax_tweak.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index ace4fb5c77..dfb0d85d66 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -564,7 +564,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type <defelt> generic_option_elem alter_generic_option_elem
%type <list> generic_option_list alter_generic_option_list
-%type <ival> reindex_target_type
+%type <ival> reindex_target_relation reindex_target_all
%type <list> opt_reindex_option_list
%type <node> copy_generic_opt_arg copy_generic_opt_arg_list_item
@@ -9092,13 +9092,12 @@ DropTransformStmt: DROP TRANSFORM opt_if_exists FOR Typename LANGUAGE name opt_d
*
* QUERY:
*
- * REINDEX [ (options) ] {TABLE | INDEX | SCHEMA} [CONCURRENTLY] <name>
- * REINDEX [ (options) ] DATABASE [CONCURRENTLY] [<name>]
- * REINDEX [ (options) ] SYSTEM [<name>]
+ * REINDEX [ (options) ] {INDEX | TABLE | SCHEMA} [CONCURRENTLY] <name>
+ * REINDEX [ (options) ] {DATABASE | SYSTEM} [CONCURRENTLY] [<name>]
*****************************************************************************/
ReindexStmt:
- REINDEX opt_reindex_option_list reindex_target_type opt_concurrently qualified_name
+ REINDEX opt_reindex_option_list reindex_target_relation opt_concurrently qualified_name
{
ReindexStmt *n = makeNode(ReindexStmt);
@@ -9124,29 +9123,27 @@ ReindexStmt:
makeDefElem("concurrently", NULL, @4));
$$ = (Node *) n;
}
- | REINDEX opt_reindex_option_list DATABASE opt_concurrently opt_single_name
+ | REINDEX opt_reindex_option_list reindex_target_all opt_concurrently opt_single_name
{
ReindexStmt *n = makeNode(ReindexStmt);
- n->kind = REINDEX_OBJECT_DATABASE;
- n->name = NULL;
- n->relation = NULL;
- n->params = $2;
- $$ = (Node *) n;
- }
- | REINDEX opt_reindex_option_list SYSTEM_P opt_single_name
- {
- ReindexStmt *n = makeNode(ReindexStmt);
- n->kind = REINDEX_OBJECT_SYSTEM;
- n->name = NULL;
+ n->kind = $3;
+ n->name = $5;
n->relation = NULL;
n->params = $2;
+ if ($4)
+ n->params = lappend(n->params,
+ makeDefElem("concurrently", NULL, @4));
$$ = (Node *) n;
}
;
-reindex_target_type:
+reindex_target_relation:
INDEX { $$ = REINDEX_OBJECT_INDEX; }
| TABLE { $$ = REINDEX_OBJECT_TABLE; }
;
+reindex_target_all:
+ SYSTEM_P { $$ = REINDEX_OBJECT_SYSTEM; }
+ | DATABASE { $$ = REINDEX_OBJECT_DATABASE; }
+ ;
opt_reindex_option_list:
'(' utility_option_list ')' { $$ = $2; }
| /* EMPTY */ { $$ = NULL; }
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index a913a1846f..801b396398 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2521,9 +2521,7 @@ ERROR: cannot reindex system catalogs concurrently
REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1260_index; -- no catalog toast index
ERROR: cannot reindex system catalogs concurrently
REINDEX SYSTEM CONCURRENTLY postgres; -- not allowed for SYSTEM
-ERROR: syntax error at or near "CONCURRENTLY"
-LINE 1: REINDEX SYSTEM CONCURRENTLY postgres;
- ^
+ERROR: cannot reindex system catalogs concurrently
REINDEX (CONCURRENTLY) SYSTEM postgres; -- ditto
ERROR: cannot reindex system catalogs concurrently
REINDEX (CONCURRENTLY) SYSTEM; -- ditto
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 6750eb6e47..fcbda88149 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -22,8 +22,7 @@ PostgreSQL documentation
<refsynopsisdiv>
<synopsis>
REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { INDEX | TABLE | SCHEMA } [ CONCURRENTLY ] <replaceable class="parameter">name</replaceable>
-REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] DATABASE [ CONCURRENTLY ] [ <replaceable class="parameter">name</replaceable> ]
-REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] SYSTEM [ <replaceable class="parameter">name</replaceable> ]
+REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { DATABASE | SYSTEM } [ CONCURRENTLY ] [ <replaceable class="parameter">name</replaceable> ]
<phrase>where <replaceable class="parameter">option</replaceable> can be one of:</phrase>
On 2022-Jul-23, Michael Paquier wrote:
As the problem comes down to the fact that INDEX/TABLE, SCHEMA and
DATABASE/SYSTEM need to handle names for different object types each,
I think that we could do something like the attached, removing one
block on the way at the cost of an extra parser node.
Yeah, looks good. I propose to also test the error for reindexing a
different database, which is currently uncovered, as attached.
By the way, it seems that 83011ce also broke the case of "REINDEX
DATABASE CONCURRENTLY", where the parser missed the addition of a
DefElem for "concurrently" in this case.
Wow.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Escucha y olvidarás; ve y recordarás; haz y entenderás" (Confucio)
Attachments:
tweak-reindex.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index dfb0d85d66..f9037761f9 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -9115,8 +9115,8 @@ ReindexStmt:
ReindexStmt *n = makeNode(ReindexStmt);
n->kind = REINDEX_OBJECT_SCHEMA;
- n->name = $5;
n->relation = NULL;
+ n->name = $5;
n->params = $2;
if ($4)
n->params = lappend(n->params,
@@ -9126,9 +9126,10 @@ ReindexStmt:
| REINDEX opt_reindex_option_list reindex_target_all opt_concurrently opt_single_name
{
ReindexStmt *n = makeNode(ReindexStmt);
+
n->kind = $3;
- n->name = $5;
n->relation = NULL;
+ n->name = $5;
n->params = $2;
if ($4)
n->params = lappend(n->params,
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 801b396398..6cd57e3eaa 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2529,6 +2529,9 @@ ERROR: cannot reindex system catalogs concurrently
-- Warns about catalog relations
REINDEX SCHEMA CONCURRENTLY pg_catalog;
WARNING: cannot reindex system catalogs concurrently, skipping all
+-- Not the current database
+REINDEX DATABASE not_current_database;
+ERROR: can only reindex the currently open database
-- Check the relation status, there should not be invalid indexes
\d concur_reindex_tab
Table "public.concur_reindex_tab"
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index 4b75790e47..a3738833b2 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -1076,6 +1076,8 @@ REINDEX (CONCURRENTLY) SYSTEM postgres; -- ditto
REINDEX (CONCURRENTLY) SYSTEM; -- ditto
-- Warns about catalog relations
REINDEX SCHEMA CONCURRENTLY pg_catalog;
+-- Not the current database
+REINDEX DATABASE not_current_database;
-- Check the relation status, there should not be invalid indexes
\d concur_reindex_tab
On Mon, Jul 25, 2022 at 11:49:50AM +0200, Alvaro Herrera wrote:
On 2022-Jul-23, Michael Paquier wrote:
As the problem comes down to the fact that INDEX/TABLE, SCHEMA and
DATABASE/SYSTEM need to handle names for different object types each,
I think that we could do something like the attached, removing one
block on the way at the cost of an extra parser node.Yeah, looks good. I propose to also test the error for reindexing a
different database, which is currently uncovered, as attached.
Good idea.
By the way, it seems that 83011ce also broke the case of "REINDEX
DATABASE CONCURRENTLY", where the parser missed the addition of a
DefElem for "concurrently" in this case.Wow.
For this one, we have a gap in the test, actually. It seems to me
that we'd better make sure that the OID of the indexes rebuilt
concurrently is changed. There is a REINDEX DATABASE CONCURRENTLY
already in the TAP tests, and the only thing that would be needed for
the job is an extra query that compares the OID saved before the
reindex with the one in the catalogs after the fact..
--
Michael
On 2022-Jul-25, Michael Paquier wrote:
On Mon, Jul 25, 2022 at 11:49:50AM +0200, Alvaro Herrera wrote:
On 2022-Jul-23, Michael Paquier wrote:
By the way, it seems that 83011ce also broke the case of "REINDEX
DATABASE CONCURRENTLY", where the parser missed the addition of a
DefElem for "concurrently" in this case.Wow.
For this one, we have a gap in the test, actually. It seems to me
that we'd better make sure that the OID of the indexes rebuilt
concurrently is changed. There is a REINDEX DATABASE CONCURRENTLY
already in the TAP tests, and the only thing that would be needed for
the job is an extra query that compares the OID saved before the
reindex with the one in the catalogs after the fact..
Agreed. I think you already have the query for that elsewhere in the
test, so it's just a matter of copying it from there.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"No tengo por qué estar de acuerdo con lo que pienso"
(Carlos Caszeli)
On Mon, Jul 25, 2022 at 12:55:54PM +0200, Alvaro Herrera wrote:
Agreed. I think you already have the query for that elsewhere in the
test, so it's just a matter of copying it from there.
I actually already wrote most of it in 2cbc3c1, and I just needed to
extend things a bit to detect the OID changes :)
So, applied, with all the extra tests.
--
Michael
On 2022-Jul-26, Michael Paquier wrote:
On Mon, Jul 25, 2022 at 12:55:54PM +0200, Alvaro Herrera wrote:
Agreed. I think you already have the query for that elsewhere in the
test, so it's just a matter of copying it from there.I actually already wrote most of it in 2cbc3c1, and I just needed to
extend things a bit to detect the OID changes :)So, applied, with all the extra tests.
Thank you!
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"I am amazed at [the pgsql-sql] mailing list for the wonderful support, and
lack of hesitasion in answering a lost soul's question, I just wished the rest
of the mailing list could be like this." (Fotis)
(http://archives.postgresql.org/pgsql-sql/2006-06/msg00265.php)