Re: ALTER INDEX ... ALTER COLUMN not present in dump
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 signature.asc Description: PGP signature
Re: ALTER INDEX ... ALTER COLUMN not present in dump
amul sul writes: >> On Mon, Dec 17, 2018 at 11:54 AM Michael Paquier >>> 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
Re: ALTER INDEX ... ALTER COLUMN not present in dump
On Mon, Dec 17, 2018 at 12:20 PM amul sul wrote: > On Mon, Dec 17, 2018 at 11:54 AM Michael Paquier > 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
Re: ALTER INDEX ... ALTER COLUMN not present in dump
On Mon, Dec 17, 2018 at 1:57 PM Michael Paquier wrote: > 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. > > Agree, will adopt the same practice in future, thank you. Regards, Amul
Re: ALTER INDEX ... ALTER COLUMN not present in dump
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 signature.asc Description: PGP signature
Re: ALTER INDEX ... ALTER COLUMN not present in dump
On Mon, Dec 17, 2018 at 1:04 PM Michael Paquier 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 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
Re: ALTER INDEX ... ALTER COLUMN not present in dump
On Mon, Dec 17, 2018 at 12:49:03PM +0530, amul sul wrote: > On Mon, Dec 17, 2018 at 11:54 AM Michael Paquier 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 signature.asc Description: PGP signature
Re: ALTER INDEX ... ALTER COLUMN not present in dump
On Mon, Dec 17, 2018 at 11:54 AM Michael Paquier 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
Re: ALTER INDEX ... ALTER COLUMN not present in dump
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 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 *
Re: ALTER INDEX ... ALTER COLUMN not present in dump
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 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 @@
Re: ALTER INDEX ... ALTER COLUMN not present in dump
On Mon, Dec 17, 2018 at 10:44 AM Michael Paquier wrote: > > On Fri, Dec 14, 2018 at 08:08:45AM +, 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: > https://www.postgresql.org/message-id/CAPpHfdsSYo6xpt0F=ngadqmpfjjhc7zapde9h1qwkdphpwf...@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? > 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
Re: ALTER INDEX ... ALTER COLUMN not present in dump
Michael Paquier 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
Re: ALTER INDEX ... ALTER COLUMN not present in dump
On Fri, Dec 14, 2018 at 08:08:45AM +, 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: https://www.postgresql.org/message-id/CAPpHfdsSYo6xpt0F=ngadqmpfjjhc7zapde9h1qwkdphpwf...@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 signature.asc Description: PGP signature
Re: ALTER INDEX ... ALTER COLUMN not present in dump
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
