ALTER INDEX ... ALTER COLUMN not present in dump
Hello,
It seems like ALTER INDEX ... ALTER COLUMN statements (for setting specific
statistics targets on functional indexes, for example) are not part of a
pg_dump.
It is not easily noticed, since everything seems to work normally until a
sub-par plan is chosen because of an error in cardinality estimates.
Regards,
--
Ronan Dunklau
Hello
Can you give reproducible example?
I have ALTER TABLE ONLY (schema).(table) ALTER COLUMN (column) SET STATISTICS (target); in pg_dump output.
regards, Sergei
Hello
Can you give reproducible example?
I have ALTER TABLE ONLY (schema).(table) ALTER COLUMN (column) SET STATISTICS (target); in pg_dump output.regards, Sergei
Please note it is about ALTER INDEX, not ALTER TABLE.
Here is the reproducible example:
create table t1 (id int);
create index on t1 ((id + 2));
alter index t1 alter column t1_expr statistics 10000;
pg_dump output extract:
--
-- Name: t1_expr_idx; Type: INDEX; Schema: public; Owner: postgres
--
CREATE INDEX t1_expr_idx ON public.t1 USING btree (((id + 2)));
--
-- PostgreSQL database dump complete
--
Oh, i see.
I can reproduce and did not found any SET STATISTICS code for indexes in pg_dump source. Seems completely missed support for this clause.
regards, Sergei
On 11/15/18 12:20 PM, Sergei Kornilov wrote:
Oh, i see.
I can reproduce and did not found any SET STATISTICS code for indexes in pg_dump source. Seems completely missed support for this clause.regards, Sergei
It seems we missed something in :
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=5b6d13eec72b960eb0f78542199380e49c8583d4;hp=e09db94c0a5f3b440d96c5c9e8e6c1638d1ec39f
Sorry :/
On Thu, Nov 15, 2018 at 12:46:08PM +0100, Adrien NAYRAT wrote:
On 11/15/18 12:20 PM, Sergei Kornilov wrote:
I can reproduce and did not found any SET STATISTICS code for indexes
in pg_dump source. Seems completely missed support for this clause.It seems we missed something in :
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=5b6d13eec72b960eb0f78542199380e49c8583d4;hp=e09db94c0a5f3b440d96c5c9e8e6c1638d1ec39f
Yes, that's a bug, and something that we should try to fix in v11.
--
Michael
On Fri, Nov 16, 2018 at 07:46:01AM +0900, Michael Paquier wrote:
Yes, that's a bug, and something that we should try to fix in v11.
Okay, here are my notes. We need to do a couple of things here:
1) Add a new join to pg_attribute in getIndexes(), then add the
information for statistics and the associated column to IndxInfo after
parsing the gathered array using parsePGArray().
2) Add the extra ALTER INDEX commands to the queries creating the
objects in dumpIndex().
3) Add a test.
A good thing is that when ALTER INDEX .. SET STATISTICS is applied on an
index of a partitioned table, the statement is not cascaded to the
existing partitions. We may want in the future to support ONLY and make
the inheritance automatic. But that's another topic, and the fix for
v11 should be chirurgical.
--
Michael
On Fri, Nov 16, 2018 at 09:45:05AM +0900, Michael Paquier wrote:
A good thing is that when ALTER INDEX .. SET STATISTICS is applied on an
index of a partitioned table, the statement is not cascaded to the
existing partitions. We may want in the future to support ONLY and make
the inheritance automatic. But that's another topic, and the fix for
v11 should be chirurgical.
And here you go as attached. Looking closer, in v10 and older versions,
ALTER INDEX SET STATISTICS is able to work as it is an alias of ALTER
TABLE. The attached patch does not bother generating the ALTER INDEX
queries for v10 and older and feeds queries with empty strings. Perhaps
we should support that case? Or the lack of complains would be an
argument sufficient to care only about v11 and newer versions? I would
tend to think that supporting only v11 and above is enough. Thoughts
are welcome.
--
Michael
Attachments:
dump-alter-index-stats.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index c8d01ed4a4..5c1bee0cb7 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -6772,7 +6772,9 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
i_conoid,
i_condef,
i_tablespace,
- i_indreloptions;
+ i_indreloptions,
+ i_indstatcols,
+ i_indstatvals;
int ntups;
for (i = 0; i < numTables; i++)
@@ -6826,7 +6828,15 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
"c.oid AS conoid, "
"pg_catalog.pg_get_constraintdef(c.oid, false) AS condef, "
"(SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) AS tablespace, "
- "t.reloptions AS indreloptions "
+ "t.reloptions AS indreloptions, "
+ "(SELECT array_agg(attname ORDER BY attname::text COLLATE \"C\") "
+ " FROM pg_catalog.pg_attribute "
+ " WHERE attrelid = i.indexrelid AND "
+ " attstattarget >= 0) AS indstatcols,"
+ "(SELECT array_agg(attstattarget ORDER BY attname::text COLLATE \"C\") "
+ " FROM pg_catalog.pg_attribute "
+ " WHERE attrelid = i.indexrelid AND "
+ " attstattarget >= 0) AS indstatvals "
"FROM pg_catalog.pg_index i "
"JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) "
"JOIN pg_catalog.pg_class t2 ON (t2.oid = i.indrelid) "
@@ -6863,7 +6873,9 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
"c.oid AS conoid, "
"pg_catalog.pg_get_constraintdef(c.oid, false) AS condef, "
"(SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) AS tablespace, "
- "t.reloptions AS indreloptions "
+ "t.reloptions AS indreloptions, "
+ "'' AS indstatcols, "
+ "'' AS indstatvals "
"FROM pg_catalog.pg_index i "
"JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) "
"LEFT JOIN pg_catalog.pg_constraint c "
@@ -6896,7 +6908,9 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
"c.oid AS conoid, "
"pg_catalog.pg_get_constraintdef(c.oid, false) AS condef, "
"(SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) AS tablespace, "
- "t.reloptions AS indreloptions "
+ "t.reloptions AS indreloptions, "
+ "'' AS indstatcols, "
+ "'' AS indstatvals "
"FROM pg_catalog.pg_index i "
"JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) "
"LEFT JOIN pg_catalog.pg_constraint c "
@@ -6925,7 +6939,9 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
"c.oid AS conoid, "
"null AS condef, "
"(SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) AS tablespace, "
- "t.reloptions AS indreloptions "
+ "t.reloptions AS indreloptions, "
+ "'' AS indstatcols, "
+ "'' AS indstatvals "
"FROM pg_catalog.pg_index i "
"JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) "
"LEFT JOIN pg_catalog.pg_depend d "
@@ -6957,7 +6973,9 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
"c.oid AS conoid, "
"null AS condef, "
"(SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) AS tablespace, "
- "null AS indreloptions "
+ "null AS indreloptions, "
+ "'' AS indstatcols, "
+ "'' AS indstatvals "
"FROM pg_catalog.pg_index i "
"JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) "
"LEFT JOIN pg_catalog.pg_depend d "
@@ -6995,6 +7013,8 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
i_condef = PQfnumber(res, "condef");
i_tablespace = PQfnumber(res, "tablespace");
i_indreloptions = PQfnumber(res, "indreloptions");
+ i_indstatcols = PQfnumber(res, "indstatcols");
+ i_indstatvals = PQfnumber(res, "indstatvals");
tbinfo->indexes = indxinfo =
(IndxInfo *) pg_malloc(ntups * sizeof(IndxInfo));
@@ -7018,6 +7038,8 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
indxinfo[j].indnattrs = atoi(PQgetvalue(res, j, i_indnatts));
indxinfo[j].tablespace = pg_strdup(PQgetvalue(res, j, i_tablespace));
indxinfo[j].indreloptions = pg_strdup(PQgetvalue(res, j, i_indreloptions));
+ indxinfo[j].indstatcols = pg_strdup(PQgetvalue(res, j, i_indstatcols));
+ indxinfo[j].indstatvals = pg_strdup(PQgetvalue(res, j, i_indstatvals));
indxinfo[j].indkeys = (Oid *) pg_malloc(indxinfo[j].indnattrs * sizeof(Oid));
parseOidArray(PQgetvalue(res, j, i_indkey),
indxinfo[j].indkeys, indxinfo[j].indnattrs);
@@ -16207,6 +16229,13 @@ dumpIndex(Archive *fout, IndxInfo *indxinfo)
*/
if (!is_constraint)
{
+ char *indstatcols = indxinfo->indstatcols;
+ char *indstatvals = indxinfo->indstatvals;
+ char **indstatcolsarray = NULL;
+ char **indstatvalsarray = NULL;
+ int nstatcols;
+ int nstatvals;
+
if (dopt->binary_upgrade)
binary_upgrade_set_pg_class_oids(fout, q,
indxinfo->dobj.catId.oid, true);
@@ -16230,6 +16259,27 @@ dumpIndex(Archive *fout, IndxInfo *indxinfo)
qindxname);
}
+ /*
+ * If the index has any statistics on some of its columns,
+ * generate the associated ALTER INDEX queries.
+ */
+ if (parsePGArray(indstatcols, &indstatcolsarray, &nstatcols) &&
+ parsePGArray(indstatvals, &indstatvalsarray, &nstatvals) &&
+ nstatcols == nstatvals)
+ {
+ int j;
+
+ for (j = 0; j < nstatcols; j++)
+ {
+ appendPQExpBuffer(q, "ALTER INDEX %s ",
+ fmtQualifiedDumpable(indxinfo));
+ appendPQExpBuffer(q, "ALTER COLUMN %s ",
+ indstatcolsarray[j]);
+ appendPQExpBuffer(q, "SET STATISTICS %s;\n",
+ indstatvalsarray[j]);
+ }
+ }
+
/* If the index defines identity, we need to record that. */
if (indxinfo->indisreplident)
{
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index 685ad78669..58ca0f7d3d 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -361,6 +361,8 @@ typedef struct _indxInfo
char *indexdef;
char *tablespace; /* tablespace in which index is stored */
char *indreloptions; /* options specified by WITH (...) */
+ char *indstatcols; /* column names with statistics */
+ char *indstatvals; /* statistic values for columns */
int indnkeyattrs; /* number of index key attributes */
int indnattrs; /* total number of index attributes */
Oid *indkeys; /* In spite of the name 'indkeys' this field
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index ec751a7c23..94a2b404c8 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -2420,6 +2420,47 @@ my %tests = (
unlike => { exclude_dump_test_schema => 1, },
},
+ 'CREATE TABLE table_with_stats' => {
+ create_order => 98,
+ create_sql => 'CREATE TABLE dump_test.table_index_stats (
+ col1 int,
+ col2 int);
+ CREATE INDEX index_with_stats
+ ON dump_test.table_index_stats
+ ((col1 + 1), (col2 + 1));
+ ALTER INDEX dump_test.index_with_stats
+ ALTER COLUMN expr SET STATISTICS 1000;',
+ regexp => qr/^
+ \QALTER INDEX dump_test.index_with_stats ALTER COLUMN expr SET STATISTICS 1000;\E
+ /xm,
+ like => {
+ binary_upgrade => 1,
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ exclude_test_table => 1,
+ exclude_test_table_data => 1,
+ no_blobs => 1,
+ no_privs => 1,
+ no_owner => 1,
+ only_dump_test_schema => 1,
+ pg_dumpall_dbprivs => 1,
+ schema_only => 1,
+ section_post_data => 1,
+ test_schema_plus_blobs => 1,
+ with_oids => 1,
+ },
+ unlike => {
+ exclude_dump_test_schema => 1,
+ only_dump_test_table => 1,
+ pg_dumpall_globals => 1,
+ pg_dumpall_globals_clean => 1,
+ role => 1,
+ section_pre_data => 1,
+ },
+ },
+
'CREATE STATISTICS extended_stats_no_options' => {
create_order => 97,
create_sql => 'CREATE STATISTICS dump_test.test_ext_stats_no_options
On Fri, Nov 16, 2018 at 05:32:52PM +0900, Michael Paquier wrote:
And here you go as attached. Looking closer, in v10 and older versions,
ALTER INDEX SET STATISTICS is able to work as it is an alias of ALTER
TABLE. The attached patch does not bother generating the ALTER INDEX
queries for v10 and older and feeds queries with empty strings. Perhaps
we should support that case? Or the lack of complains would be an
argument sufficient to care only about v11 and newer versions? I would
tend to think that supporting only v11 and above is enough. Thoughts
are welcome.
+ appendPQExpBuffer(q, "ALTER COLUMN %s ",
+ indstatcolsarray[j])This forgot a wrapping with fmtId(). Friday hits hard..
--
Michael
Attachments:
dump-alter-index-stats-v2.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index c8d01ed4a4..767644cf49 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -6772,7 +6772,9 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
i_conoid,
i_condef,
i_tablespace,
- i_indreloptions;
+ i_indreloptions,
+ i_indstatcols,
+ i_indstatvals;
int ntups;
for (i = 0; i < numTables; i++)
@@ -6826,7 +6828,15 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
"c.oid AS conoid, "
"pg_catalog.pg_get_constraintdef(c.oid, false) AS condef, "
"(SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) AS tablespace, "
- "t.reloptions AS indreloptions "
+ "t.reloptions AS indreloptions, "
+ "(SELECT array_agg(attname ORDER BY attname::text COLLATE \"C\") "
+ " FROM pg_catalog.pg_attribute "
+ " WHERE attrelid = i.indexrelid AND "
+ " attstattarget >= 0) AS indstatcols,"
+ "(SELECT array_agg(attstattarget ORDER BY attname::text COLLATE \"C\") "
+ " FROM pg_catalog.pg_attribute "
+ " WHERE attrelid = i.indexrelid AND "
+ " attstattarget >= 0) AS indstatvals "
"FROM pg_catalog.pg_index i "
"JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) "
"JOIN pg_catalog.pg_class t2 ON (t2.oid = i.indrelid) "
@@ -6863,7 +6873,9 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
"c.oid AS conoid, "
"pg_catalog.pg_get_constraintdef(c.oid, false) AS condef, "
"(SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) AS tablespace, "
- "t.reloptions AS indreloptions "
+ "t.reloptions AS indreloptions, "
+ "'' AS indstatcols, "
+ "'' AS indstatvals "
"FROM pg_catalog.pg_index i "
"JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) "
"LEFT JOIN pg_catalog.pg_constraint c "
@@ -6896,7 +6908,9 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
"c.oid AS conoid, "
"pg_catalog.pg_get_constraintdef(c.oid, false) AS condef, "
"(SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) AS tablespace, "
- "t.reloptions AS indreloptions "
+ "t.reloptions AS indreloptions, "
+ "'' AS indstatcols, "
+ "'' AS indstatvals "
"FROM pg_catalog.pg_index i "
"JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) "
"LEFT JOIN pg_catalog.pg_constraint c "
@@ -6925,7 +6939,9 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
"c.oid AS conoid, "
"null AS condef, "
"(SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) AS tablespace, "
- "t.reloptions AS indreloptions "
+ "t.reloptions AS indreloptions, "
+ "'' AS indstatcols, "
+ "'' AS indstatvals "
"FROM pg_catalog.pg_index i "
"JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) "
"LEFT JOIN pg_catalog.pg_depend d "
@@ -6957,7 +6973,9 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
"c.oid AS conoid, "
"null AS condef, "
"(SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) AS tablespace, "
- "null AS indreloptions "
+ "null AS indreloptions, "
+ "'' AS indstatcols, "
+ "'' AS indstatvals "
"FROM pg_catalog.pg_index i "
"JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) "
"LEFT JOIN pg_catalog.pg_depend d "
@@ -6995,6 +7013,8 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
i_condef = PQfnumber(res, "condef");
i_tablespace = PQfnumber(res, "tablespace");
i_indreloptions = PQfnumber(res, "indreloptions");
+ i_indstatcols = PQfnumber(res, "indstatcols");
+ i_indstatvals = PQfnumber(res, "indstatvals");
tbinfo->indexes = indxinfo =
(IndxInfo *) pg_malloc(ntups * sizeof(IndxInfo));
@@ -7018,6 +7038,8 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
indxinfo[j].indnattrs = atoi(PQgetvalue(res, j, i_indnatts));
indxinfo[j].tablespace = pg_strdup(PQgetvalue(res, j, i_tablespace));
indxinfo[j].indreloptions = pg_strdup(PQgetvalue(res, j, i_indreloptions));
+ indxinfo[j].indstatcols = pg_strdup(PQgetvalue(res, j, i_indstatcols));
+ indxinfo[j].indstatvals = pg_strdup(PQgetvalue(res, j, i_indstatvals));
indxinfo[j].indkeys = (Oid *) pg_malloc(indxinfo[j].indnattrs * sizeof(Oid));
parseOidArray(PQgetvalue(res, j, i_indkey),
indxinfo[j].indkeys, indxinfo[j].indnattrs);
@@ -16207,6 +16229,13 @@ dumpIndex(Archive *fout, IndxInfo *indxinfo)
*/
if (!is_constraint)
{
+ char *indstatcols = indxinfo->indstatcols;
+ char *indstatvals = indxinfo->indstatvals;
+ char **indstatcolsarray = NULL;
+ char **indstatvalsarray = NULL;
+ int nstatcols;
+ int nstatvals;
+
if (dopt->binary_upgrade)
binary_upgrade_set_pg_class_oids(fout, q,
indxinfo->dobj.catId.oid, true);
@@ -16230,6 +16259,27 @@ dumpIndex(Archive *fout, IndxInfo *indxinfo)
qindxname);
}
+ /*
+ * If the index has any statistics on some of its columns,
+ * generate the associated ALTER INDEX queries.
+ */
+ if (parsePGArray(indstatcols, &indstatcolsarray, &nstatcols) &&
+ parsePGArray(indstatvals, &indstatvalsarray, &nstatvals) &&
+ nstatcols == nstatvals)
+ {
+ int j;
+
+ for (j = 0; j < nstatcols; j++)
+ {
+ appendPQExpBuffer(q, "ALTER INDEX %s ",
+ fmtQualifiedDumpable(indxinfo));
+ appendPQExpBuffer(q, "ALTER COLUMN %s ",
+ fmtId(indstatcolsarray[j]));
+ appendPQExpBuffer(q, "SET STATISTICS %s;\n",
+ indstatvalsarray[j]);
+ }
+ }
+
/* If the index defines identity, we need to record that. */
if (indxinfo->indisreplident)
{
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index 685ad78669..58ca0f7d3d 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -361,6 +361,8 @@ typedef struct _indxInfo
char *indexdef;
char *tablespace; /* tablespace in which index is stored */
char *indreloptions; /* options specified by WITH (...) */
+ char *indstatcols; /* column names with statistics */
+ char *indstatvals; /* statistic values for columns */
int indnkeyattrs; /* number of index key attributes */
int indnattrs; /* total number of index attributes */
Oid *indkeys; /* In spite of the name 'indkeys' this field
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index ec751a7c23..94a2b404c8 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -2420,6 +2420,47 @@ my %tests = (
unlike => { exclude_dump_test_schema => 1, },
},
+ 'CREATE TABLE table_with_stats' => {
+ create_order => 98,
+ create_sql => 'CREATE TABLE dump_test.table_index_stats (
+ col1 int,
+ col2 int);
+ CREATE INDEX index_with_stats
+ ON dump_test.table_index_stats
+ ((col1 + 1), (col2 + 1));
+ ALTER INDEX dump_test.index_with_stats
+ ALTER COLUMN expr SET STATISTICS 1000;',
+ regexp => qr/^
+ \QALTER INDEX dump_test.index_with_stats ALTER COLUMN expr SET STATISTICS 1000;\E
+ /xm,
+ like => {
+ binary_upgrade => 1,
+ clean => 1,
+ clean_if_exists => 1,
+ createdb => 1,
+ defaults => 1,
+ exclude_test_table => 1,
+ exclude_test_table_data => 1,
+ no_blobs => 1,
+ no_privs => 1,
+ no_owner => 1,
+ only_dump_test_schema => 1,
+ pg_dumpall_dbprivs => 1,
+ schema_only => 1,
+ section_post_data => 1,
+ test_schema_plus_blobs => 1,
+ with_oids => 1,
+ },
+ unlike => {
+ exclude_dump_test_schema => 1,
+ only_dump_test_table => 1,
+ pg_dumpall_globals => 1,
+ pg_dumpall_globals_clean => 1,
+ role => 1,
+ section_pre_data => 1,
+ },
+ },
+
'CREATE STATISTICS extended_stats_no_options' => {
create_order => 97,
create_sql => 'CREATE STATISTICS dump_test.test_ext_stats_no_options
On Fri, Nov 16, 2018 at 10:31:03PM +0900, Michael Paquier wrote:
On Fri, Nov 16, 2018 at 05:32:52PM +0900, Michael Paquier wrote:
And here you go as attached. Looking closer, in v10 and older versions,
ALTER INDEX SET STATISTICS is able to work as it is an alias of ALTER
TABLE. The attached patch does not bother generating the ALTER INDEX
queries for v10 and older and feeds queries with empty strings. Perhaps
we should support that case? Or the lack of complains would be an
argument sufficient to care only about v11 and newer versions? I would
tend to think that supporting only v11 and above is enough. Thoughts
are welcome.
So, any thoughts about this patch? I would still like to move on with
only supporting this set of queries only for v11 and above as that gets
only clearly documented on the ALTER INDEX page from that point.
--
Michael
On 11/21/18 1:04 AM, Michael Paquier wrote:
On Fri, Nov 16, 2018 at 10:31:03PM +0900, Michael Paquier wrote:
On Fri, Nov 16, 2018 at 05:32:52PM +0900, Michael Paquier wrote:
And here you go as attached. Looking closer, in v10 and older versions,
ALTER INDEX SET STATISTICS is able to work as it is an alias of ALTER
TABLE. The attached patch does not bother generating the ALTER INDEX
queries for v10 and older and feeds queries with empty strings. Perhaps
we should support that case? Or the lack of complains would be an
argument sufficient to care only about v11 and newer versions? I would
tend to think that supporting only v11 and above is enough. Thoughts
are welcome.So, any thoughts about this patch? I would still like to move on with
only supporting this set of queries only for v11 and above as that gets
only clearly documented on the ALTER INDEX page from that point.
--
Michael
Sorry, I will try to look at this soon, but it is relatively new for me.
At least, someone else with more knowledge should also look.
Thanks to address this issue.
On 11/21/18 9:09 AM, Adrien Nayrat wrote:
So, any thoughts about this patch? I would still like to move on with
only supporting this set of queries only for v11 and above as that gets
only clearly documented on the ALTER INDEX page from that point.
--
MichaelSorry, I will try to look at this soon, but it is relatively new for me.
At least, someone else with more knowledge should also look.Thanks to address this issue.
I done some tests and it look good to me. I took an eye on the code and
nothing hurt me, but I am not the most qualified to say that.
Thanks Michael for the fix!
On Wed, Nov 21, 2018 at 07:24:18PM +0100, Adrien NAYRAT wrote:
I done some tests and it look good to me. I took an eye on the code and
nothing hurt me, but I am not the most qualified to say that.
Thanks Adrien for the review. For now I have added it to the next
commit fest:
https://commitfest.postgresql.org/21/1884/
--
Michael
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested
dump-alter-index-stats-v2.patch looks pretty much reasonable to me, passing on committer.
The new status of this patch is: Ready for Committer
On Fri, Dec 14, 2018 at 08:08:45AM +0000, Amul Sul wrote:
dump-alter-index-stats-v2.patch looks pretty much reasonable to me, passing on committer.
The new status of this patch is: Ready for Committer
Thanks Amul for the review. I got the occasion to look again at this
patch, and I have read again the original thread which has added the new
grammar for ALTER INDEX SET STATISTICS:
/messages/by-id/CAPpHfdsSYo6xpt0F=ngAdqMPFJJhC7zApde9h1qwkdpHpwFisA@mail.gmail.com
As Alexander and others state on this thread, it looks a bit weird to
use internally-produced attribute names in those SQL queries, which is
why the new grammar has been added. At the same time, it looks more
solid to me to represent the dumps with those column names instead of
column numbers. Tom, Alexander, as you have commented on the original
thread, perhaps you have an opinion here to share?
For now, attached is an updated patch which has a simplified test list
in the TAP test. I have also added two free() calls for the arrays
getting allocated when statistics are present for an index.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
As Alexander and others state on this thread, it looks a bit weird to
use internally-produced attribute names in those SQL queries, which is
why the new grammar has been added. At the same time, it looks more
solid to me to represent the dumps with those column names instead of
column numbers. Tom, Alexander, as you have commented on the original
thread, perhaps you have an opinion here to share?
The problem is that there's no guarantee that the new server would
generate the same column name for an index column --- and I don't
want to try to lock things down so much that there would be such
a guarantee. So I'd go with the column-number form.
As an example:
regression=# create table foo (expr int, f1 int, f2 int);
CREATE TABLE
regression=# create index on foo ((f1+f2));
CREATE INDEX
regression=# create index on foo (expr, (f1+f2));
CREATE INDEX
regression=# \d foo
Table "public.foo"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
expr | integer | | |
f1 | integer | | |
f2 | integer | | |
Indexes:
"foo_expr_expr1_idx" btree (expr, (f1 + f2))
"foo_expr_idx" btree ((f1 + f2))
regression=# \d foo_expr_idx
Index "public.foo_expr_idx"
Column | Type | Key? | Definition
--------+---------+------+------------
expr | integer | yes | (f1 + f2)
btree, for table "public.foo"
regression=# \d foo_expr_expr1_idx
Index "public.foo_expr_expr1_idx"
Column | Type | Key? | Definition
--------+---------+------+------------
expr | integer | yes | expr
expr1 | integer | yes | (f1 + f2)
btree, for table "public.foo"
If we were to rename the "foo.expr" column at this point,
and then dump and reload, the expression column in the
second index would presumably acquire the name "expr"
not "expr1", because "expr" would no longer be taken.
So if pg_dump were to try to use that index column name
in ALTER ... SET STATISTICS, it'd fail.
regards, tom lane
On Mon, Dec 17, 2018 at 10:44 AM Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Dec 14, 2018 at 08:08:45AM +0000, Amul Sul wrote:
dump-alter-index-stats-v2.patch looks pretty much reasonable to me, passing on committer.
The new status of this patch is: Ready for Committer
Thanks Amul for the review. I got the occasion to look again at this
patch, and I have read again the original thread which has added the new
grammar for ALTER INDEX SET STATISTICS:
/messages/by-id/CAPpHfdsSYo6xpt0F=ngAdqMPFJJhC7zApde9h1qwkdpHpwFisA@mail.gmail.comAs Alexander and others state on this thread, it looks a bit weird to
use internally-produced attribute names in those SQL queries, which is
why the new grammar has been added. At the same time, it looks more
solid to me to represent the dumps with those column names instead of
column numbers. Tom, Alexander, as you have commented on the original
thread, perhaps you have an opinion here to share?
Oh I see -- understood the problem, I missed this discussion, thanks to
letting me know.
For now, attached is an updated patch which has a simplified test list
in the TAP test. I have also added two free() calls for the arrays
getting allocated when statistics are present for an index.
Patch is missing?
Regards,
Amul
On Mon, Dec 17, 2018 at 10:59:08AM +0530, amul sul wrote:
Patch is missing?
Here you go. The patch is still using atttribute names, which is a bad
idea ;)
--
Michael
Attachments:
dump-alter-index-stats-v3.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 637c79af48..09e90ea62c 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -6712,7 +6712,9 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
i_conoid,
i_condef,
i_tablespace,
- i_indreloptions;
+ i_indreloptions,
+ i_indstatcols,
+ i_indstatvals;
int ntups;
for (i = 0; i < numTables; i++)
@@ -6766,7 +6768,15 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
"c.oid AS conoid, "
"pg_catalog.pg_get_constraintdef(c.oid, false) AS condef, "
"(SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) AS tablespace, "
- "t.reloptions AS indreloptions "
+ "t.reloptions AS indreloptions, "
+ "(SELECT pg_catalog.array_agg(attname ORDER BY attname::text COLLATE \"C\") "
+ " FROM pg_catalog.pg_attribute "
+ " WHERE attrelid = i.indexrelid AND "
+ " attstattarget >= 0) AS indstatcols,"
+ "(SELECT pg_catalog.array_agg(attstattarget ORDER BY attname::text COLLATE \"C\") "
+ " FROM pg_catalog.pg_attribute "
+ " WHERE attrelid = i.indexrelid AND "
+ " attstattarget >= 0) AS indstatvals "
"FROM pg_catalog.pg_index i "
"JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) "
"JOIN pg_catalog.pg_class t2 ON (t2.oid = i.indrelid) "
@@ -6803,7 +6813,9 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
"c.oid AS conoid, "
"pg_catalog.pg_get_constraintdef(c.oid, false) AS condef, "
"(SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) AS tablespace, "
- "t.reloptions AS indreloptions "
+ "t.reloptions AS indreloptions, "
+ "'' AS indstatcols, "
+ "'' AS indstatvals "
"FROM pg_catalog.pg_index i "
"JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) "
"LEFT JOIN pg_catalog.pg_constraint c "
@@ -6836,7 +6848,9 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
"c.oid AS conoid, "
"pg_catalog.pg_get_constraintdef(c.oid, false) AS condef, "
"(SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) AS tablespace, "
- "t.reloptions AS indreloptions "
+ "t.reloptions AS indreloptions, "
+ "'' AS indstatcols, "
+ "'' AS indstatvals "
"FROM pg_catalog.pg_index i "
"JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) "
"LEFT JOIN pg_catalog.pg_constraint c "
@@ -6865,7 +6879,9 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
"c.oid AS conoid, "
"null AS condef, "
"(SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) AS tablespace, "
- "t.reloptions AS indreloptions "
+ "t.reloptions AS indreloptions, "
+ "'' AS indstatcols, "
+ "'' AS indstatvals "
"FROM pg_catalog.pg_index i "
"JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) "
"LEFT JOIN pg_catalog.pg_depend d "
@@ -6897,7 +6913,9 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
"c.oid AS conoid, "
"null AS condef, "
"(SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) AS tablespace, "
- "null AS indreloptions "
+ "null AS indreloptions, "
+ "'' AS indstatcols, "
+ "'' AS indstatvals "
"FROM pg_catalog.pg_index i "
"JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) "
"LEFT JOIN pg_catalog.pg_depend d "
@@ -6935,6 +6953,8 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
i_condef = PQfnumber(res, "condef");
i_tablespace = PQfnumber(res, "tablespace");
i_indreloptions = PQfnumber(res, "indreloptions");
+ i_indstatcols = PQfnumber(res, "indstatcols");
+ i_indstatvals = PQfnumber(res, "indstatvals");
tbinfo->indexes = indxinfo =
(IndxInfo *) pg_malloc(ntups * sizeof(IndxInfo));
@@ -6958,6 +6978,8 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
indxinfo[j].indnattrs = atoi(PQgetvalue(res, j, i_indnatts));
indxinfo[j].tablespace = pg_strdup(PQgetvalue(res, j, i_tablespace));
indxinfo[j].indreloptions = pg_strdup(PQgetvalue(res, j, i_indreloptions));
+ indxinfo[j].indstatcols = pg_strdup(PQgetvalue(res, j, i_indstatcols));
+ indxinfo[j].indstatvals = pg_strdup(PQgetvalue(res, j, i_indstatvals));
indxinfo[j].indkeys = (Oid *) pg_malloc(indxinfo[j].indnattrs * sizeof(Oid));
parseOidArray(PQgetvalue(res, j, i_indkey),
indxinfo[j].indkeys, indxinfo[j].indnattrs);
@@ -16148,6 +16170,13 @@ dumpIndex(Archive *fout, IndxInfo *indxinfo)
*/
if (!is_constraint)
{
+ char *indstatcols = indxinfo->indstatcols;
+ char *indstatvals = indxinfo->indstatvals;
+ char **indstatcolsarray = NULL;
+ char **indstatvalsarray = NULL;
+ int nstatcols;
+ int nstatvals;
+
if (dopt->binary_upgrade)
binary_upgrade_set_pg_class_oids(fout, q,
indxinfo->dobj.catId.oid, true);
@@ -16171,6 +16200,27 @@ dumpIndex(Archive *fout, IndxInfo *indxinfo)
qindxname);
}
+ /*
+ * If the index has any statistics on some of its columns,
+ * generate the associated ALTER INDEX queries.
+ */
+ if (parsePGArray(indstatcols, &indstatcolsarray, &nstatcols) &&
+ parsePGArray(indstatvals, &indstatvalsarray, &nstatvals) &&
+ nstatcols == nstatvals)
+ {
+ int j;
+
+ for (j = 0; j < nstatcols; j++)
+ {
+ appendPQExpBuffer(q, "ALTER INDEX %s ",
+ fmtQualifiedDumpable(indxinfo));
+ appendPQExpBuffer(q, "ALTER COLUMN %s ",
+ fmtId(indstatcolsarray[j]));
+ appendPQExpBuffer(q, "SET STATISTICS %s;\n",
+ indstatvalsarray[j]);
+ }
+ }
+
/* If the index defines identity, we need to record that. */
if (indxinfo->indisreplident)
{
@@ -16194,6 +16244,11 @@ dumpIndex(Archive *fout, IndxInfo *indxinfo)
q->data, delq->data, NULL,
NULL, 0,
NULL, NULL);
+
+ if (indstatcolsarray)
+ free(indstatcolsarray);
+ if (indstatvalsarray)
+ free(indstatvalsarray);
}
/* Dump Index Comments */
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index 789d6a24e2..527c233851 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -360,6 +360,8 @@ typedef struct _indxInfo
char *indexdef;
char *tablespace; /* tablespace in which index is stored */
char *indreloptions; /* options specified by WITH (...) */
+ char *indstatcols; /* column names with statistics */
+ char *indstatvals; /* statistic values for columns */
int indnkeyattrs; /* number of index key attributes */
int indnattrs; /* total number of index attributes */
Oid *indkeys; /* In spite of the name 'indkeys' this field
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 46dbb078cf..c5a2b7485e 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -2343,6 +2343,24 @@ my %tests = (
unlike => { exclude_dump_test_schema => 1, },
},
+ 'CREATE TABLE table_with_stats' => {
+ create_order => 98,
+ create_sql => 'CREATE TABLE dump_test.table_index_stats (
+ col1 int,
+ col2 int);
+ CREATE INDEX index_with_stats
+ ON dump_test.table_index_stats
+ ((col1 + 1), (col2 + 1));
+ ALTER INDEX dump_test.index_with_stats
+ ALTER COLUMN expr SET STATISTICS 1000;',
+ regexp => qr/^
+ \QALTER INDEX dump_test.index_with_stats ALTER COLUMN expr SET STATISTICS 1000;\E
+ /xm,
+ like =>
+ { %full_runs, %dump_test_schema_runs, section_post_data => 1, },
+ unlike => { exclude_dump_test_schema => 1, },
+ },
+
'CREATE STATISTICS extended_stats_no_options' => {
create_order => 97,
create_sql => 'CREATE STATISTICS dump_test.test_ext_stats_no_options
On Mon, Dec 17, 2018 at 12:24:15AM -0500, Tom Lane wrote:
If we were to rename the "foo.expr" column at this point,
and then dump and reload, the expression column in the
second index would presumably acquire the name "expr"
not "expr1", because "expr" would no longer be taken.
So if pg_dump were to try to use that index column name
in ALTER ... SET STATISTICS, it'd fail.
Good point, thanks! I did not think about the case where a table uses
an attribute name matching what would be generated for indexes.
So this settles the argument that we had better not do anything before
v11. Switching the dump code to use column numbers has not proved to be
complicated as only the query and some comments had to be tweaked.
Attached is an updated patch, and I am switching back the patch to
"Needs review" to have an extra pair of eyes look at that in case I
missed something.
--
Michael
Attachments:
dump-alter-index-stats-v4.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 637c79af48..a46dd4c8e6 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -6712,7 +6712,9 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
i_conoid,
i_condef,
i_tablespace,
- i_indreloptions;
+ i_indreloptions,
+ i_indstatcols,
+ i_indstatvals;
int ntups;
for (i = 0; i < numTables; i++)
@@ -6766,7 +6768,15 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
"c.oid AS conoid, "
"pg_catalog.pg_get_constraintdef(c.oid, false) AS condef, "
"(SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) AS tablespace, "
- "t.reloptions AS indreloptions "
+ "t.reloptions AS indreloptions, "
+ "(SELECT pg_catalog.array_agg(attnum ORDER BY attnum) "
+ " FROM pg_catalog.pg_attribute "
+ " WHERE attrelid = i.indexrelid AND "
+ " attstattarget >= 0) AS indstatcols,"
+ "(SELECT pg_catalog.array_agg(attstattarget ORDER BY attnum) "
+ " FROM pg_catalog.pg_attribute "
+ " WHERE attrelid = i.indexrelid AND "
+ " attstattarget >= 0) AS indstatvals "
"FROM pg_catalog.pg_index i "
"JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) "
"JOIN pg_catalog.pg_class t2 ON (t2.oid = i.indrelid) "
@@ -6803,7 +6813,9 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
"c.oid AS conoid, "
"pg_catalog.pg_get_constraintdef(c.oid, false) AS condef, "
"(SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) AS tablespace, "
- "t.reloptions AS indreloptions "
+ "t.reloptions AS indreloptions, "
+ "'' AS indstatcols, "
+ "'' AS indstatvals "
"FROM pg_catalog.pg_index i "
"JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) "
"LEFT JOIN pg_catalog.pg_constraint c "
@@ -6836,7 +6848,9 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
"c.oid AS conoid, "
"pg_catalog.pg_get_constraintdef(c.oid, false) AS condef, "
"(SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) AS tablespace, "
- "t.reloptions AS indreloptions "
+ "t.reloptions AS indreloptions, "
+ "'' AS indstatcols, "
+ "'' AS indstatvals "
"FROM pg_catalog.pg_index i "
"JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) "
"LEFT JOIN pg_catalog.pg_constraint c "
@@ -6865,7 +6879,9 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
"c.oid AS conoid, "
"null AS condef, "
"(SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) AS tablespace, "
- "t.reloptions AS indreloptions "
+ "t.reloptions AS indreloptions, "
+ "'' AS indstatcols, "
+ "'' AS indstatvals "
"FROM pg_catalog.pg_index i "
"JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) "
"LEFT JOIN pg_catalog.pg_depend d "
@@ -6897,7 +6913,9 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
"c.oid AS conoid, "
"null AS condef, "
"(SELECT spcname FROM pg_catalog.pg_tablespace s WHERE s.oid = t.reltablespace) AS tablespace, "
- "null AS indreloptions "
+ "null AS indreloptions, "
+ "'' AS indstatcols, "
+ "'' AS indstatvals "
"FROM pg_catalog.pg_index i "
"JOIN pg_catalog.pg_class t ON (t.oid = i.indexrelid) "
"LEFT JOIN pg_catalog.pg_depend d "
@@ -6935,6 +6953,8 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
i_condef = PQfnumber(res, "condef");
i_tablespace = PQfnumber(res, "tablespace");
i_indreloptions = PQfnumber(res, "indreloptions");
+ i_indstatcols = PQfnumber(res, "indstatcols");
+ i_indstatvals = PQfnumber(res, "indstatvals");
tbinfo->indexes = indxinfo =
(IndxInfo *) pg_malloc(ntups * sizeof(IndxInfo));
@@ -6958,6 +6978,8 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
indxinfo[j].indnattrs = atoi(PQgetvalue(res, j, i_indnatts));
indxinfo[j].tablespace = pg_strdup(PQgetvalue(res, j, i_tablespace));
indxinfo[j].indreloptions = pg_strdup(PQgetvalue(res, j, i_indreloptions));
+ indxinfo[j].indstatcols = pg_strdup(PQgetvalue(res, j, i_indstatcols));
+ indxinfo[j].indstatvals = pg_strdup(PQgetvalue(res, j, i_indstatvals));
indxinfo[j].indkeys = (Oid *) pg_malloc(indxinfo[j].indnattrs * sizeof(Oid));
parseOidArray(PQgetvalue(res, j, i_indkey),
indxinfo[j].indkeys, indxinfo[j].indnattrs);
@@ -16148,6 +16170,13 @@ dumpIndex(Archive *fout, IndxInfo *indxinfo)
*/
if (!is_constraint)
{
+ char *indstatcols = indxinfo->indstatcols;
+ char *indstatvals = indxinfo->indstatvals;
+ char **indstatcolsarray = NULL;
+ char **indstatvalsarray = NULL;
+ int nstatcols;
+ int nstatvals;
+
if (dopt->binary_upgrade)
binary_upgrade_set_pg_class_oids(fout, q,
indxinfo->dobj.catId.oid, true);
@@ -16171,6 +16200,31 @@ dumpIndex(Archive *fout, IndxInfo *indxinfo)
qindxname);
}
+ /*
+ * If the index has any statistics on some of its columns,
+ * generate the associated ALTER INDEX queries.
+ */
+ if (parsePGArray(indstatcols, &indstatcolsarray, &nstatcols) &&
+ parsePGArray(indstatvals, &indstatvalsarray, &nstatvals) &&
+ nstatcols == nstatvals)
+ {
+ int j;
+
+ for (j = 0; j < nstatcols; j++)
+ {
+ appendPQExpBuffer(q, "ALTER INDEX %s ",
+ fmtQualifiedDumpable(indxinfo));
+ /*
+ * Note that this is a column number, so no quotes should
+ * be used.
+ */
+ appendPQExpBuffer(q, "ALTER COLUMN %s ",
+ indstatcolsarray[j]);
+ appendPQExpBuffer(q, "SET STATISTICS %s;\n",
+ indstatvalsarray[j]);
+ }
+ }
+
/* If the index defines identity, we need to record that. */
if (indxinfo->indisreplident)
{
@@ -16194,6 +16248,11 @@ dumpIndex(Archive *fout, IndxInfo *indxinfo)
q->data, delq->data, NULL,
NULL, 0,
NULL, NULL);
+
+ if (indstatcolsarray)
+ free(indstatcolsarray);
+ if (indstatvalsarray)
+ free(indstatvalsarray);
}
/* Dump Index Comments */
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index 789d6a24e2..1d997fa36f 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -360,6 +360,8 @@ typedef struct _indxInfo
char *indexdef;
char *tablespace; /* tablespace in which index is stored */
char *indreloptions; /* options specified by WITH (...) */
+ char *indstatcols; /* column numbers with statistics */
+ char *indstatvals; /* statistic values for columns */
int indnkeyattrs; /* number of index key attributes */
int indnattrs; /* total number of index attributes */
Oid *indkeys; /* In spite of the name 'indkeys' this field
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 46dbb078cf..8fd283f601 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -2343,6 +2343,24 @@ my %tests = (
unlike => { exclude_dump_test_schema => 1, },
},
+ 'CREATE TABLE table_with_stats' => {
+ create_order => 98,
+ create_sql => 'CREATE TABLE dump_test.table_index_stats (
+ col1 int,
+ col2 int);
+ CREATE INDEX index_with_stats
+ ON dump_test.table_index_stats
+ ((col1 + 1), (col2 + 1));
+ ALTER INDEX dump_test.index_with_stats
+ ALTER COLUMN 1 SET STATISTICS 1000;',
+ regexp => qr/^
+ \QALTER INDEX dump_test.index_with_stats ALTER COLUMN 1 SET STATISTICS 1000;\E
+ /xm,
+ like =>
+ { %full_runs, %dump_test_schema_runs, section_post_data => 1, },
+ unlike => { exclude_dump_test_schema => 1, },
+ },
+
'CREATE STATISTICS extended_stats_no_options' => {
create_order => 97,
create_sql => 'CREATE STATISTICS dump_test.test_ext_stats_no_options
On Mon, Dec 17, 2018 at 11:54 AM Michael Paquier <michael@paquier.xyz> wrote:
On Mon, Dec 17, 2018 at 12:24:15AM -0500, Tom Lane wrote:
If we were to rename the "foo.expr" column at this point,
and then dump and reload, the expression column in the
second index would presumably acquire the name "expr"
not "expr1", because "expr" would no longer be taken.
So if pg_dump were to try to use that index column name
in ALTER ... SET STATISTICS, it'd fail.Good point, thanks! I did not think about the case where a table uses
an attribute name matching what would be generated for indexes.So this settles the argument that we had better not do anything before
v11. Switching the dump code to use column numbers has not proved to be
complicated as only the query and some comments had to be tweaked.
Attached is an updated patch, and I am switching back the patch to
"Needs review" to have an extra pair of eyes look at that in case I
missed something.
+1, will have a look, thanks.
Regards,
Amul
Hi Michael,
On Mon, Dec 17, 2018 at 11:54 AM Michael Paquier <michael@paquier.xyz> wrote:
[...]
So this settles the argument that we had better not do anything before
v11. Switching the dump code to use column numbers has not proved to be
complicated as only the query and some comments had to be tweaked.
Attached is an updated patch, and I am switching back the patch to
"Needs review" to have an extra pair of eyes look at that in case I
missed something.
This v4-patch needs a rebase against the latest master head(#67915fb).
Regards,
Amul
On Mon, Dec 17, 2018 at 12:49:03PM +0530, amul sul wrote:
On Mon, Dec 17, 2018 at 11:54 AM Michael Paquier <michael@paquier.xyz> wrote:
So this settles the argument that we had better not do anything before
v11. Switching the dump code to use column numbers has not proved to be
complicated as only the query and some comments had to be tweaked.
Attached is an updated patch, and I am switching back the patch to
"Needs review" to have an extra pair of eyes look at that in case I
missed something.This v4-patch needs a rebase against the latest master head(#67915fb).
I am on top of the master branch at 67915fb8, and this applies fine for
me:
$ patch -p1 < dump-alter-index-stats-v4.patch
patching file src/bin/pg_dump/pg_dump.c
patching file src/bin/pg_dump/pg_dump.h
patching file src/bin/pg_dump/t/002_pg_dump.pl
Thanks,
--
Michael
On Mon, Dec 17, 2018 at 1:04 PM Michael Paquier <michael@paquier.xyz> wrote:
On Mon, Dec 17, 2018 at 12:49:03PM +0530, amul sul wrote:
On Mon, Dec 17, 2018 at 11:54 AM Michael Paquier <michael@paquier.xyz>
wrote:
So this settles the argument that we had better not do anything before
v11. Switching the dump code to use column numbers has not proved to
be
complicated as only the query and some comments had to be tweaked.
Attached is an updated patch, and I am switching back the patch to
"Needs review" to have an extra pair of eyes look at that in case I
missed something.This v4-patch needs a rebase against the latest master head(#67915fb).
I am on top of the master branch at 67915fb8, and this applies fine for
me:
$ patch -p1 < dump-alter-index-stats-v4.patch
patching file src/bin/pg_dump/pg_dump.c
patching file src/bin/pg_dump/pg_dump.h
patching file src/bin/pg_dump/t/002_pg_dump.pl
Thanks, patch command works for me as well, with git I was getting a
following failure:
Laptop215:PG edb$ git apply ~/Downloads/dump-alter-index-stats-v4.patch
/Users/edb/Downloads/dump-alter-index-stats-v4.patch:10: trailing
whitespace.
i_indreloptions,
/Users/edb/Downloads/dump-alter-index-stats-v4.patch:11: trailing
whitespace.
i_indstatcols,
/Users/edb/Downloads/dump-alter-index-stats-v4.patch:12: trailing
whitespace.
i_indstatvals;
/Users/edb/Downloads/dump-alter-index-stats-v4.patch:21: trailing
whitespace.
"t.reloptions AS indreloptions, "
/Users/edb/Downloads/dump-alter-index-stats-v4.patch:22: trailing
whitespace.
"(SELECT pg_catalog.array_agg(attnum ORDER BY attnum) "
error: patch failed: src/bin/pg_dump/pg_dump.c:6712
error: src/bin/pg_dump/pg_dump.c: patch does not apply
error: patch failed: src/bin/pg_dump/pg_dump.h:360
error: src/bin/pg_dump/pg_dump.h: patch does not apply
error: patch failed: src/bin/pg_dump/t/002_pg_dump.pl:2343
error: src/bin/pg_dump/t/002_pg_dump.pl: patch does not apply
Laptop215:PG edb$ git --version
git version 2.14.1
Regards,
Amul
On Mon, Dec 17, 2018 at 01:19:00PM +0530, amul sul wrote:
Laptop215:PG edb$ git --version
git version 2.14.1
Using 2.20, git apply works fine for me but... If patch -p1 works, why
not just using it then? It is always possible to check the patch for
whitespaces or such with "git diff --check" anyway.
--
Michael
On Mon, Dec 17, 2018 at 1:57 PM Michael Paquier <michael@paquier.xyz> wrote:
On Mon, Dec 17, 2018 at 01:19:00PM +0530, amul sul wrote:
Laptop215:PG edb$ git --version
git version 2.14.1Using 2.20, git apply works fine for me but... If patch -p1 works, why
not just using it then? It is always possible to check the patch for
whitespaces or such with "git diff --check" anyway.Agree, will adopt the same practice in future, thank you.
Regards,
Amul
On Mon, Dec 17, 2018 at 12:20 PM amul sul <sulamul@gmail.com> wrote:
On Mon, Dec 17, 2018 at 11:54 AM Michael Paquier <michael@paquier.xyz>
wrote:On Mon, Dec 17, 2018 at 12:24:15AM -0500, Tom Lane wrote:
If we were to rename the "foo.expr" column at this point,
and then dump and reload, the expression column in the
second index would presumably acquire the name "expr"
not "expr1", because "expr" would no longer be taken.
So if pg_dump were to try to use that index column name
in ALTER ... SET STATISTICS, it'd fail.Good point, thanks! I did not think about the case where a table uses
an attribute name matching what would be generated for indexes.So this settles the argument that we had better not do anything before
v11. Switching the dump code to use column numbers has not proved to be
complicated as only the query and some comments had to be tweaked.
Attached is an updated patch, and I am switching back the patch to
"Needs review" to have an extra pair of eyes look at that in case I
missed something.+1, will have a look, thanks.
I been through the patch -- looks good and does the expected job as
discussed.
make check and make check-world also fine.
Regards,
Amul
amul sul <sulamul@gmail.com> writes:
On Mon, Dec 17, 2018 at 11:54 AM Michael Paquier <michael@paquier.xyz>
So this settles the argument that we had better not do anything before
v11. Switching the dump code to use column numbers has not proved to be
complicated as only the query and some comments had to be tweaked.
Attached is an updated patch, and I am switching back the patch to
"Needs review" to have an extra pair of eyes look at that in case I
missed something.
I been through the patch -- looks good and does the expected job as
discussed.
I eyeballed the patch, and it seems reasonable to me as well. The test
case could perhaps be made more extensive (say, a multicolumn index
with a mix of columns with and without stats targets) but I'm not sure
that it's worth the trouble.
regards, tom lane
On Mon, Dec 17, 2018 at 02:24:08PM -0500, Tom Lane wrote:
I eyeballed the patch, and it seems reasonable to me as well. The test
case could perhaps be made more extensive (say, a multicolumn index
with a mix of columns with and without stats targets) but I'm not sure
that it's worth the trouble.
Thanks all for the lookup and input! I have added a test pattern more
complex with multiple columns, and committed it down to v11.
--
Michael