Horton

Thread

Commits

  1. Disallow non-default collation in ADD PRIMARY KEY/UNIQUE USING INDEX.

  1. pg_dump losing index column collations for unique and primary keys

    Alexey Bashtanov <bashtanov@imap.cc> — 2019-12-03T16:35:20Z

    Hello,

I found that pg_dump fails to note down the collations used primary and 
unique keys supporting indexes.
To reproduce:

create table f(a text);
create unique index f_pkey on f(a collate "C.UTF-8");
alter table f add primary key using index f_pkey;
\d f

pg_dump -t f

The only way to dump it correctly is to create indexes explicitly and 
then use them in constraint definitions.
Please could someone have a look at the patch attached. Thank you.

Best,
   Alex

  2. Re: pg_dump losing index column collations for unique and primary keys

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-03T16:57:57Z

    Alexey Bashtanov <bashtanov@imap.cc> writes:
> I found that pg_dump fails to note down the collations used primary and 
> unique keys supporting indexes.
> To reproduce:

> create table f(a text);
> create unique index f_pkey on f(a collate "C.UTF-8");
> alter table f add primary key using index f_pkey;

Hm.  I kinda think that we should reject the above.  The point of
ADD PRIMARY KEY USING INDEX ought to be to allow you to build
the index concurrently; it should not be something you can use to
create a DDL state that is impossible to arrive at with plain
ADD PRIMARY KEY.

As an example of the sort of problem that I'm worried about,
consider a situation where the index's collation has a different
notion of equality than the column's default collation has.
(Which is entirely possible now that we have nondeterministic
collations.)  That's going to lead to weird restrictions on
whether the index really satisfies query WHERE conditions, and
I'd bet considerable money that we'd have bugs due to failing
to account for that.

In short, I'd say the bug here is not pg_dump's fault at all,
but failure to insist on collation match in ADD PRIMARY KEY
USING INDEX.

			regards, tom lane

  3. Re: pg_dump losing index column collations for unique and primary keys

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-03T18:24:38Z

    I wrote:
> In short, I'd say the bug here is not pg_dump's fault at all,
> but failure to insist on collation match in ADD PRIMARY KEY
> USING INDEX.

Concretely, I think we should do the attached.

I'm not quite sure whether we should back-patch this, though.
It's been wrong since we added collations, but the main impact
of a back-patch might be to break cases that were working more
or less okay for people.

A compromise idea is to back-patch only into v12, where the issue
became quite a lot more important due to nondeterministic
collations.

Thoughts?

			regards, tom lane

  4. Re: pg_dump losing index column collations for unique and primary keys

    Alexey Bashtanov <bashtanov@imap.cc> — 2019-12-03T19:30:49Z

    Hi Tom,

>> In short, I'd say the bug here is not pg_dump's fault at all,
>> but failure to insist on collation match in ADD PRIMARY KEY
>> USING INDEX.
While being entirely reasonable especially with the non-deterministic 
collations,
this breaks down an important facility of changing a column collation 
without
rewriting it even if it is a part of a unique constraint: first change 
the PK then the
column itself.

> Concretely, I think we should do the attached.

While being entirely reasonable especially with the non-deterministic 
collations,
this breaks down an important facility of changing a column collation 
without
rewriting it even if it is a part of a unique constraint: first change 
the PK then the
column itself.

Disallowing changing the direction (ASC/DESC) also looks cruel to me.

BTW with playing with this stuff I came across another issue,
which seems unrelated to collations:

2019-12-03 19:08:26 alexey@[local]/alexey=# \d g
                  Table "public.g"
┌────────┬──────┬────────────┬──────────┬─────────┐
│ Column │ Type │ Collation  │ Nullable │ Default │
├────────┼──────┼────────────┼──────────┼─────────┤
│ a      │ text │ en_US.utf8 │ not null │         │
└────────┴──────┴────────────┴──────────┴─────────┘
Indexes:
     "g_pkey" PRIMARY KEY, btree (a)
     "g_a_idx" UNIQUE, btree (a)

2019-12-03 19:08:27 alexey@[local]/alexey=# begin; alter table g drop 
constraint g_pkey, add primary key using index g_a_idx, alter column a 
type text;
BEGIN
Time: 0.421 ms
ERROR:  could not open relation with OID 16417
Time: 9.990 ms

Is it something known?

 From user's perspective I think an ideal solution would be to fix the above
and to enforce the PK/UK and column collation to be the same only
by the end of ALTER TABLE command (and document this, as it isn't very 
obvious).
I haven't yet looked in the code though to see how comfortable would 
that be to implement it.

Best, Alex

  5. Re: pg_dump losing index column collations for unique and primary keys

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-03T19:52:13Z

    Alexey Bashtanov <bashtanov@imap.cc> writes:
>> In short, I'd say the bug here is not pg_dump's fault at all,
>> but failure to insist on collation match in ADD PRIMARY KEY
>> USING INDEX.

> While being entirely reasonable especially with the non-deterministic 
> collations,
> this breaks down an important facility of changing a column collation 
> without
> rewriting it even if it is a part of a unique constraint: first change 
> the PK then the
> column itself.

Well, I disagree that that's a high-priority use case, but it seems to me
that you can still do it.  You just can't call the index the pkey until
the column collation agrees.  So roughly it'd be

* CREATE UNIQUE INDEX ... (col COLLATE new-collation)
* Drop old pkey (the new index is still enforcing uniqueness)
* ALTER TABLE ... ALTER COLUMN ... COLLATE new-collation
* ALTER TABLE ADD PRIMARY KEY USING INDEX

These seem to me to be pretty much exactly the same steps you'd need
today, although maybe the current code is more forgiving about
their ordering.

> Disallowing changing the direction (ASC/DESC) also looks cruel to me.

That restriction has been there since day one, and we have had a grand
total of zero complaints about it.

> BTW with playing with this stuff I came across another issue,
> which seems unrelated to collations:

Yeah, see
https://www.postgresql.org/message-id/flat/10365.1558909428@sss.pgh.pa.us
If you'd like to help move things along by reviewing that patch,
it'd be great.

			regards, tom lane

  6. Re: pg_dump losing index column collations for unique and primary keys

    Alexey Bashtanov <bashtanov@imap.cc> — 2019-12-04T13:51:32Z

    > Well, I disagree that that's a high-priority use case, but it seems to me
> that you can still do it.  You just can't call the index the pkey until
> the column collation agrees.  So roughly it'd be
>
> * CREATE UNIQUE INDEX ... (col COLLATE new-collation)
> * Drop old pkey (the new index is still enforcing uniqueness)
> * ALTER TABLE ... ALTER COLUMN ... COLLATE new-collation
> * ALTER TABLE ADD PRIMARY KEY USING INDEX
>
> These seem to me to be pretty much exactly the same steps you'd need
> today, although maybe the current code is more forgiving about
> their ordering.
Agreed.
>> Disallowing changing the direction (ASC/DESC) also looks cruel to me.
> That restriction has been there since day one, and we have had a grand
> total of zero complaints about it.
True, my bad.

What do you think of making pg_dump warn the user if they are trying
to dump a weird PK/UK which has collations in index and column not matching?
And maybe even throw an error in case of --binary-upgrade?

>> BTW with playing with this stuff I came across another issue,
>> which seems unrelated to collations:
> Yeah, see
> https://www.postgresql.org/message-id/flat/10365.1558909428@sss.pgh.pa.us
> If you'd like to help move things along by reviewing that patch,
> it'd be great.
I'll have a look.

Best, Alex

  7. Re: pg_dump losing index column collations for unique and primary keys

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-04T15:48:58Z

    Alexey Bashtanov <bashtanov@imap.cc> writes:
> What do you think of making pg_dump warn the user if they are trying
> to dump a weird PK/UK which has collations in index and column not matching?
> And maybe even throw an error in case of --binary-upgrade?

I can't get excited about spending any additional effort on this issue.
If I thought it were actually happening in the field to any significant
extent, I'd be more interested in supporting the case instead of just
disallowing it.  But I think the actual occurrences are epsilon, so
there are better places to be spending our time.

			regards, tom lane