Horton

Thread

Commits

  1. Don't mark partitioned indexes invalid unnecessarily

  2. Allow ALTER TABLE ONLY on partitioned tables

  1. don't mark indexes invalid unnecessarily

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-12-03T22:50:19Z

    While working on FKs pointing to partitioned tables, I noticed that in
PG11 we fail to produce a working dump in the case of a partitioned
table that doesn't have partitions.

The attached patch fixes that.  In doing so, it breaks a test ... and
analyzing that, it turns out that the test was broken, it wasn't testing
what it was supposed to be testing.  I patched it so that it continues
to work (and now it tests the correct thing).  To be precise, the test
was doing this:

 create table parted_conflict (a int, b text) partition by range (a);
 create table parted_conflict_1 partition of parted_conflict for values from (0) to (1000) partition by range (a);
 create unique index on only parted_conflict_1 (a);
 create unique index on only parted_conflict (a);
 alter index parted_conflict_a_idx attach partition parted_conflict_1_a_idx;
 create table parted_conflict_1_1 partition of parted_conflict_1 for values from (0) to (500);

with the expectation that the partition would not have a proper index on
which to run an INSERT INTO parted_conflict_1 ON CONFLICT (a), expecting
that partition parted_conflict_1_1 would not have the index.  But in
reality parted_conflict_1_1 does have the index (and it only fails
because the index in its parent is marked invalid).  So the patch moves
the CREATE TABLE parted_conflict_1_1 to before the indexes creation, so
that the partition really does not have the index, and then it gets the
expected error.

If you were to run the pg_upgrade test with my fks-to-partitioned-tables
patch, it will fail because of a PK on an partitionless partitioned
table being marked invalid after restore; but if you run it after
applying this patch, it should work (it works for me).

-- 
Álvaro Herrera                            39°50'S 73°21'W

  2. Re: don't mark indexes invalid unnecessarily

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-12-05T08:59:07Z

    Hi,

On 2018/12/04 7:50, Alvaro Herrera wrote:
> While working on FKs pointing to partitioned tables, I noticed that in
> PG11 we fail to produce a working dump in the case of a partitioned
> table that doesn't have partitions.
> 
> The attached patch fixes that.

The fix makes sense.  I see that the fix is similar in spirit to:

commit 9139aa19423b736470f669e566f8ef6a7f19b801
Author: Stephen Frost <sfrost@snowman.net>
Date:   Tue Apr 25 16:57:43 2017 -0400

  Allow ALTER TABLE ONLY on partitioned tables

  There is no need to forbid ALTER TABLE ONLY on partitioned tables,
  when no partitions exist yet.  This can be handy for users who are
  building up their partitioned table independently and will create actual
  partitions later.

  In addition, this is how pg_dump likes to operate in certain instances.

> In doing so, it breaks a test ... and
> analyzing that, it turns out that the test was broken, it wasn't testing
> what it was supposed to be testing.

Hadn't considered it this closely at the time, but...

> I patched it so that it continues
> to work (and now it tests the correct thing).  To be precise, the test
> was doing this:
> 
>  create table parted_conflict (a int, b text) partition by range (a);
>  create table parted_conflict_1 partition of parted_conflict for values from (0) to (1000) partition by range (a);
>  create unique index on only parted_conflict_1 (a);
>  create unique index on only parted_conflict (a);
>  alter index parted_conflict_a_idx attach partition parted_conflict_1_a_idx;
>  create table parted_conflict_1_1 partition of parted_conflict_1 for values from (0) to (500);
> 
> with the expectation that the partition would not have a proper index on
> which to run an INSERT INTO parted_conflict_1 ON CONFLICT (a), expecting
> that partition parted_conflict_1_1 would not have the index.  But in
> reality parted_conflict_1_1 does have the index (and it only fails
> because the index in its parent is marked invalid).  So the patch moves
> the CREATE TABLE parted_conflict_1_1 to before the indexes creation, so
> that the partition really does not have the index, and then it gets the
> expected error.

... isn't the test really trying to show that invalid unique index on
table mentioned in the insert command cannot be used to proceed with the
on conflict clause, and so isn't broken per se?  But maybe I misunderstood
you.

Thanks,
Amit

  3. Re: don't mark indexes invalid unnecessarily

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-12-05T15:24:46Z

    On 2018-Dec-05, Amit Langote wrote:

> Hi,
> 
> On 2018/12/04 7:50, Alvaro Herrera wrote:
> > While working on FKs pointing to partitioned tables, I noticed that in
> > PG11 we fail to produce a working dump in the case of a partitioned
> > table that doesn't have partitions.
> > 
> > The attached patch fixes that.
> 
> The fix makes sense.  I see that the fix is similar in spirit to:
> 
> commit 9139aa19423b736470f669e566f8ef6a7f19b801

Ooh, right, it's pretty much the same.  I wasn't aware of this commit,
thanks for pointing it out.

> > I patched it so that it continues
> > to work (and now it tests the correct thing).  To be precise, the test
> > was doing this:
> > 
> >  create table parted_conflict (a int, b text) partition by range (a);
> >  create table parted_conflict_1 partition of parted_conflict for values from (0) to (1000) partition by range (a);
> >  create unique index on only parted_conflict_1 (a);
> >  create unique index on only parted_conflict (a);
> >  alter index parted_conflict_a_idx attach partition parted_conflict_1_a_idx;
> >  create table parted_conflict_1_1 partition of parted_conflict_1 for values from (0) to (500);
> > 
> > with the expectation that the partition would not have a proper index on
> > which to run an INSERT INTO parted_conflict_1 ON CONFLICT (a), expecting
> > that partition parted_conflict_1_1 would not have the index.  But in
> > reality parted_conflict_1_1 does have the index (and it only fails
> > because the index in its parent is marked invalid).  So the patch moves
> > the CREATE TABLE parted_conflict_1_1 to before the indexes creation, so
> > that the partition really does not have the index, and then it gets the
> > expected error.
> 
> ... isn't the test really trying to show that invalid unique index on
> table mentioned in the insert command cannot be used to proceed with the
> on conflict clause, and so isn't broken per se?  But maybe I misunderstood
> you.

You're right, I misinterpreted one bit: I was thinking that when we
create the first partition parted_conflict_1_1, then the index
automatically created in it causes the index in parted_conflict_1 to
become valid.  But that's not so: the index remains invalid.  This seems
a bit broken in itself, but it's not really important because that's
precisely what's getting fixed by my patch, namely that the index
created ONLY is valid if there are no partitions.  So if we want the
index to appear invalid (to continue to reproduce the scenario), we need
to create the partition first.

Thanks for reviewing.

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

  4. Re: don't mark indexes invalid unnecessarily

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-12-05T16:39:00Z

    Pushed, thanks for the review.

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