Re: unique indexes on partitioned tables

Here is a mini-patch on top of yours to fix a few cosmetic things.

I don't understand the variable name "third". I don't see a "first" or
"second" nearby.

I find some of the columns in pg_constraint confusing. For a primary
key on a partitioned table, for the PK on the partition I get

conislocal = false, coninhcount = 1, connoinherit = true

The last part is confusing to me.

I don't know if that's really on this patch. But perhaps it could be
documented better.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 12 February 2018 at 15:26, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

Hello,

Thanks, Peter, Jesper, Amit, for reviewing the patch. Replying to
all review comments at once:

[... v5 of patch attached ...]

Hi Álvaro,

attached a tiny patch (on top of yours) that silence two "variables
uninitilized" warnings.

also noted that if you:

"""
create table t1(i int) partition by hash (i);
create table t1_0 partition of t1 for values with (modulus 2, remainder 0);
create table t1_1 partition of t1 for values with (modulus 2, remainder 1);
create unique index on t1(i);
alter table t1 add primary key using index t1_i_idx ;
"""

the ALTER TABLE ADD PK does not recurse to partitions, which maybe is
perfectly fine because i'm using USING INDEX but it feels like an
oversight to me

--
Jaime Casanova www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Jaime Casanova wrote:

Hi �lvaro,

attached a tiny patch (on top of yours) that silence two "variables
uninitilized" warnings.

Thanks! Applied.

also noted that if you:

"""
create table t1(i int) partition by hash (i);
create table t1_0 partition of t1 for values with (modulus 2, remainder 0);
create table t1_1 partition of t1 for values with (modulus 2, remainder 1);
create unique index on t1(i);
alter table t1 add primary key using index t1_i_idx ;
"""

the ALTER TABLE ADD PK does not recurse to partitions, which maybe is
perfectly fine because i'm using USING INDEX but it feels like an
oversight to me

Ouch. Yeah, this is a bug. I'll try to come up with something.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

I pushed this now, with fixes for the last few comments there were.

Peter Eisentraut wrote:

I don't understand the variable name "third". I don't see a "first" or
"second" nearby.

Hah. That was referring to variables "myself" and "referenced". I
changed the variable name to "parentConstr".

I find some of the columns in pg_constraint confusing. For a primary
key on a partitioned table, for the PK on the partition I get

conislocal = false, coninhcount = 1, connoinherit = true

The last part is confusing to me.

Yeah, I think it was patently wrong. I changed it so that connoinherit
becomes true in this case.

Alvaro Herrera wrote:

Jaime Casanova wrote:

also noted that if you:

"""
create table t1(i int) partition by hash (i);
create table t1_0 partition of t1 for values with (modulus 2, remainder 0);
create table t1_1 partition of t1 for values with (modulus 2, remainder 1);
create unique index on t1(i);
alter table t1 add primary key using index t1_i_idx ;
"""

the ALTER TABLE ADD PK does not recurse to partitions, which maybe is
perfectly fine because i'm using USING INDEX but it feels like an
oversight to me

Ouch. Yeah, this is a bug. I'll try to come up with something.

After looking at it for a few minutes I determined that adding this
feature requires some more work: you need to iterate on all partitions,
obtain the corresponding index, cons up a few fake parse nodes, then
recurse to create the PK in the children. I think this should be doable
with a couple dozen lines of code, but it's a refinement that can be
added on top. Care to submit a patch? In the meantime, I added an
ereport(ERROR) to avoid leaving the system in an inconsistent state
(that probably would not even be reproduced correctly by pg_dump).

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi.

On 2018/02/20 5:45, Alvaro Herrera wrote:

I pushed this now, with fixes for the last few comments there were.

I noticed with the commit that, while ON CONFLICT (conflict_target) DO
UPDATE gives a less surprising error message by catching it in the parser,
ON CONFLICT (conflict_target) DO NOTHING will go into the executor without
the necessary code to handle the case. Example:

create table p (a int primary key, b text) partition by list (a);
create table p12 partition of p for values in (1, 2);
create table p3 partition of p (a unique) for values in (3);

insert into p values (1, 'a') on conflict (a) do nothing;
ERROR: unexpected failure to find arbiter index

Attached is a patch to fix that. Actually, there are two -- one that
adjusts the partitioned table tests in insert_conflict.sql to have a
partitioned unique index and another that fixes the code.

I suppose we'd need to apply this temporarily until we fix the ON CONFLICT
(conflict_target) case to be able to use partitioned indexes.

Thanks,
Amit

Hi.

I tried this feature with the latest snapshot. When I executed the following SQL statement, multiple primary keys were created on the partition.
Is this the intended behavior?

-- test
postgres=> CREATE TABLE part1(c1 INT PRIMARY KEY, c2 INT, c3 VARCHAR(10)) PARTITION BY RANGE(c1) ;
CREATE TABLE
postgres=> CREATE TABLE part1v1 (LIKE part1) ;
CREATE TABLE
postgres=> ALTER TABLE part1v1 ADD CONSTRAINT pk_part1v1 PRIMARY KEY (c1, c2) ;
ALTER TABLE
postgres=> ALTER TABLE part1 ATTACH PARTITION part1v1 FOR VALUES FROM (100) TO (200) ;
ALTER TABLE
postgres=> \d part1v1
Table "public.part1v1"
Column | Type | Collation | Nullable | Default
--------+-----------------------+-----------+----------+---------
c1 | integer | | not null |
c2 | integer | | not null |
c3 | character varying(10) | | |
Partition of: part1 FOR VALUES FROM (100) TO (200)
Indexes:
"part1v1_pkey" PRIMARY KEY, btree (c1)
"pk_part1v1" PRIMARY KEY, btree (c1, c2)

Regards,

Noriyoshi Shinoda

-----Original Message-----
From: Amit Langote [mailto:Langote_Amit_f8@lab.ntt.co.jp]
Sent: Tuesday, February 20, 2018 6:24 PM
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Jaime Casanova <jaime.casanova@2ndquadrant.com>
Cc: Jesper Pedersen <jesper.pedersen@redhat.com>; Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: unique indexes on partitioned tables

Hi.

On 2018/02/20 5:45, Alvaro Herrera wrote:

I pushed this now, with fixes for the last few comments there were.

I noticed with the commit that, while ON CONFLICT (conflict_target) DO UPDATE gives a less surprising error message by catching it in the parser, ON CONFLICT (conflict_target) DO NOTHING will go into the executor without the necessary code to handle the case. Example:

create table p (a int primary key, b text) partition by list (a); create table p12 partition of p for values in (1, 2); create table p3 partition of p (a unique) for values in (3);

insert into p values (1, 'a') on conflict (a) do nothing;
ERROR: unexpected failure to find arbiter index

Attached is a patch to fix that. Actually, there are two -- one that adjusts the partitioned table tests in insert_conflict.sql to have a partitioned unique index and another that fixes the code.

I suppose we'd need to apply this temporarily until we fix the ON CONFLICT
(conflict_target) case to be able to use partitioned indexes.

Thanks,
Amit

Hi,

Shinoda, Noriyoshi wrote:

I tried this feature with the latest snapshot. When I executed the
following SQL statement, multiple primary keys were created on the
partition. Is this the intended behavior?

Hahah. Is that a serious question? Of course it isn't. I'll fix it:

-- test
postgres=> CREATE TABLE part1(c1 INT PRIMARY KEY, c2 INT, c3 VARCHAR(10)) PARTITION BY RANGE(c1) ;
CREATE TABLE
postgres=> CREATE TABLE part1v1 (LIKE part1) ;
CREATE TABLE
postgres=> ALTER TABLE part1v1 ADD CONSTRAINT pk_part1v1 PRIMARY KEY (c1, c2) ;
ALTER TABLE
postgres=> ALTER TABLE part1 ATTACH PARTITION part1v1 FOR VALUES FROM (100) TO (200) ;
ALTER TABLE

I think the correct behavior here is to error out the ATTACH PARTITION
indicating that a primary key already exist.

Thanks for pointing this out. Please keep testing,

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Shinoda, Noriyoshi wrote:

Hi,

I tried this feature with the latest snapshot. When I executed the
following SQL statement, multiple primary keys were created on the
partition.
Is this the intended behavior?

It turns out that the error check for duplicate PKs is only invoked if
you tell this code that it's being invoked by ALTER TABLE, and my
original patch wasn't. I changed it and now everything seems to behave
as expected.

I added a test case pretty much like yours, which now works correctly.
I also added another one where the bogus PK is two levels down rather
than one. This is because I had originally developed a different fix --
which fixed the problem for your test case, until I realized that since
this code is recursive, we could cause trouble at a distance.

Thanks for reporting the problem

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi Álvaro,

Thank you for your developing the new patch.
I will continue testing.

-----Original Message-----
From: Alvaro Herrera [mailto:alvherre@2ndquadrant.com]
Sent: Tuesday, March 13, 2018 7:51 AM
To: Shinoda, Noriyoshi <noriyoshi.shinoda@hpe.com>
Cc: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Jaime Casanova <jaime.casanova@2ndquadrant.com>; Jesper Pedersen <jesper.pedersen@redhat.com>; Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: unique indexes on partitioned tables

Shinoda, Noriyoshi wrote:

Hi,

I tried this feature with the latest snapshot. When I executed the
following SQL statement, multiple primary keys were created on the
partition.
Is this the intended behavior?

It turns out that the error check for duplicate PKs is only invoked if you tell this code that it's being invoked by ALTER TABLE, and my original patch wasn't. I changed it and now everything seems to behave as expected.

I added a test case pretty much like yours, which now works correctly.
I also added another one where the bogus PK is two levels down rather than one. This is because I had originally developed a different fix -- which fixed the problem for your test case, until I realized that since this code is recursive, we could cause trouble at a distance.

Thanks for reporting the problem

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services