Обсуждение: 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

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:
https://www.postgresql.org/message-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:
> https://www.postgresql.org/message-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?
>

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

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

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 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

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