Thread: Dependencies for partitioned indexes are still a mess

Dependencies for partitioned indexes are still a mess

From
Tom Lane
Date:
I've been experimenting with trying to dump-and-restore the
regression database, which is a test case that for some reason
we don't cover in the buildfarm (pg_upgrade is not the same thing).
It seems like the dependency choices we've made for partitioned
indexes are a complete failure for this purpose.

Setup:

1. make installcheck
2. Work around the bug complained of at [1]:
   psql regression -c 'drop table gtest30_1, gtest1_1'
3. pg_dump -Fc regression >regression.dump

Issue #1: "--clean" does not work

1. createdb r2
2. pg_restore -d r2 regression.dump
3. pg_restore --clean -d r2 regression.dump

pg_restore: while PROCESSING TOC:
pg_restore: from TOC entry 6606; 1259 35458 INDEX idxpart32_a_idx postgres
pg_restore: error: could not execute query: ERROR:  cannot drop index public.idxpart32_a_idx because index
public.idxpart3_a_idxrequires it 
HINT:  You can drop index public.idxpart3_a_idx instead.
Command was: DROP INDEX public.idxpart32_a_idx;
pg_restore: from TOC entry 6605; 1259 35454 INDEX idxpart31_a_idx postgres
pg_restore: error: could not execute query: ERROR:  cannot drop index public.idxpart31_a_idx because index
public.idxpart3_a_idxrequires it 
HINT:  You can drop index public.idxpart3_a_idx instead.
Command was: DROP INDEX public.idxpart31_a_idx;
...
pg_restore: from TOC entry 6622; 2606 35509 CONSTRAINT pk52 pk52_pkey postgres
pg_restore: error: could not execute query: ERROR:  cannot drop inherited constraint "pk52_pkey" of relation "pk52"
Command was: ALTER TABLE ONLY regress_indexing.pk52 DROP CONSTRAINT pk52_pkey;
pg_restore: from TOC entry 6620; 2606 35504 CONSTRAINT pk51 pk51_pkey postgres
pg_restore: error: could not execute query: ERROR:  cannot drop inherited constraint "pk51_pkey" of relation "pk51"
Command was: ALTER TABLE ONLY regress_indexing.pk51 DROP CONSTRAINT pk51_pkey;
pg_restore: from TOC entry 6618; 2606 35502 CONSTRAINT pk5 pk5_pkey postgres
pg_restore: error: could not execute query: ERROR:  cannot drop inherited constraint "pk5_pkey" of relation "pk5"
Command was: ALTER TABLE ONLY regress_indexing.pk5 DROP CONSTRAINT pk5_pkey;
...

(There seem to be some other problems as well, but most of the 54 complaints
are related to partitioned indexes/constraints.)

Issue #2: parallel restore does not work

1. dropdb r2; createdb r2
2. pg_restore -j8 -d r2 regression.dump

This is fairly timing-dependent, but some attempts fail with messages
like

pg_restore: while PROCESSING TOC:
pg_restore: from TOC entry 6684; 2606 29166 FK CONSTRAINT fk fk_a_fkey postgres
pg_restore: error: could not execute query: ERROR:  there is no unique constraint matching given keys for referenced
table"pk" 
Command was: ALTER TABLE fkpart3.fk
    ADD CONSTRAINT fk_a_fkey FOREIGN KEY (a) REFERENCES fkpart3.pk(a);

The problem here seems to be that some commands like this:

ALTER INDEX fkpart3.pk5_pkey ATTACH PARTITION fkpart3.pk52_pkey;

are not executed soon enough, indicating that we lack dependencies
that would guarantee the restore order.

I have not analyzed these issues in any detail -- they're just bugs
I tripped over while testing parallel pg_restore.  In particular
I do not know if #1 and #2 have the same root cause.

            regards, tom lane

[1] https://www.postgresql.org/message-id/3169466.1594841366%40sss.pgh.pa.us



Re: Dependencies for partitioned indexes are still a mess

From
Alvaro Herrera
Date:
On 2020-Jul-15, Tom Lane wrote:

> Issue #1: "--clean" does not work
> 
> 1. createdb r2
> 2. pg_restore -d r2 regression.dump
> 3. pg_restore --clean -d r2 regression.dump
> 
> pg_restore: while PROCESSING TOC:
> pg_restore: from TOC entry 6606; 1259 35458 INDEX idxpart32_a_idx postgres
> pg_restore: error: could not execute query: ERROR:  cannot drop index public.idxpart32_a_idx because index
public.idxpart3_a_idxrequires it
 
> HINT:  You can drop index public.idxpart3_a_idx instead.
> Command was: DROP INDEX public.idxpart32_a_idx;

I think this problem is just that we're trying to drop a partition index
that's not droppable.  This seems fixed with just leaving the dropStmt
empty, as in the attached.

One issue is that if you previously restored only that particular
partition and its indexes, but not the ATTACH command that would make it
dependent on the parent index, there would not be a DROP command to get
rid of it.  Do we need to be concerned about that case?  I'm inclined to
think not.

> (There seem to be some other problems as well, but most of the 54 complaints
> are related to partitioned indexes/constraints.)

In my run of it there's a good dozen remaining problems, all alike: we
do DROP TYPE widget CASCADE (which works) followed by DROP FUNCTION
public.widget_out(widget), which fails complaining that type widget
doesn't exist.  But in reality the error is innocuous, since that
function was dropped by the DROP TYPE CASCADE anyway.  You could say
that the same thing is happening with these noisy DROP INDEX of index
partitions: the complaints are right in that each partition's DROP INDEX
command doesn't actually work, but the indexes are dropped later anyway,
so the effect is the same.

> Issue #2: parallel restore does not work

Looking.

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

Attachment

Re: Dependencies for partitioned indexes are still a mess

From
Tom Lane
Date:
Andres Freund <andres@anarazel.de> writes:
> I've attached the diff between first.sql and second.sql. Here's the
> highlights:

As I recall, the differences in b_star etc are expected, because
pg_dump reorders that table's columns to match its inheritance parent,
which they don't to start with because of ALTER TABLE operations.

I'm pretty sure we set it up that way deliberately ages ago, because
pg_dump used to have bugs when contending with such cases.  Not sure
about a good way to mechanize recognizing that these diffs are
expected.

Dunno about test_type_diff2, but it might be a newer instance of
the same thing.

            regards, tom lane



Re: Dependencies for partitioned indexes are still a mess

From
Alvaro Herrera
Date:
On 2020-Jul-15, Tom Lane wrote:

> Issue #2: parallel restore does not work
> 
> 1. dropdb r2; createdb r2
> 2. pg_restore -j8 -d r2 regression.dump 
> 
> This is fairly timing-dependent, but some attempts fail with messages
> like
> 
> pg_restore: while PROCESSING TOC:
> pg_restore: from TOC entry 6684; 2606 29166 FK CONSTRAINT fk fk_a_fkey postgres
> pg_restore: error: could not execute query: ERROR:  there is no unique constraint matching given keys for referenced
table"pk"
 
> Command was: ALTER TABLE fkpart3.fk
>     ADD CONSTRAINT fk_a_fkey FOREIGN KEY (a) REFERENCES fkpart3.pk(a);

Hmm, we do make the FK constraint depend on the ATTACH for the direct
children; what I think we're lacking is dependencies on descendants
twice-removed (?) or higher.  This mock patch seems to fix this problem
by adding dependencies recursively on all children of the index; I no
longer see this problem with it.

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

Attachment

Re: Dependencies for partitioned indexes are still a mess

From
Tom Lane
Date:
Andres Freund <andres@anarazel.de> writes:
> Given that pg_dump already re-orders the columns for DDL, could we make
> it apply that re-ordering not just during the CREATE TABLE, but also
> when dumping the table contents?

Hm, possibly.  I think when this was last looked at, we didn't have any
way to get COPY to output the columns in non-physical order, but now we
do.

            regards, tom lane



Re: Dependencies for partitioned indexes are still a mess

From
Alvaro Herrera
Date:
On 2020-Aug-12, Alvaro Herrera wrote:

> Hmm, we do make the FK constraint depend on the ATTACH for the direct
> children; what I think we're lacking is dependencies on descendants
> twice-removed (?) or higher.  This mock patch seems to fix this problem
> by adding dependencies recursively on all children of the index; I no
> longer see this problem with it.

After going over this some more, this analysis seems correct.  Here's a
better version of the patch which seems final to me.

I'm not yet clear on whether the noisy DROP INDEX is an actual bug that
needs to be fixed, or instead it needs to be left alone.

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

Attachment

Re: Dependencies for partitioned indexes are still a mess

From
Alvaro Herrera
Date:
On 2020-Aug-14, Alvaro Herrera wrote:

> On 2020-Aug-12, Alvaro Herrera wrote:
> 
> > Hmm, we do make the FK constraint depend on the ATTACH for the direct
> > children; what I think we're lacking is dependencies on descendants
> > twice-removed (?) or higher.  This mock patch seems to fix this problem
> > by adding dependencies recursively on all children of the index; I no
> > longer see this problem with it.
> 
> After going over this some more, this analysis seems correct.  Here's a
> better version of the patch which seems final to me.

Pushed.

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



Re: Dependencies for partitioned indexes are still a mess

From
Alvaro Herrera
Date:
On 2020-Aug-12, Alvaro Herrera wrote:

> On 2020-Jul-15, Tom Lane wrote:

> > (There seem to be some other problems as well, but most of the 54 complaints
> > are related to partitioned indexes/constraints.)
> 
> In my run of it there's a good dozen remaining problems, all alike: we
> do DROP TYPE widget CASCADE (which works) followed by DROP FUNCTION
> public.widget_out(widget), which fails complaining that type widget
> doesn't exist.  But in reality the error is innocuous, since that
> function was dropped by the DROP TYPE CASCADE anyway.  You could say
> that the same thing is happening with these noisy DROP INDEX of index
> partitions: the complaints are right in that each partition's DROP INDEX
> command doesn't actually work, but the indexes are dropped later anyway,
> so the effect is the same.

I pushed the typo fix that was in this patch.  Other than that, I think
this patch should not be pushed; ISTM it would break the logic.
(Consider that the partition with its index might exist beforehand and
be an independent table.  If we wanted --clean to work properly, it
should definitely drop that index.)

Although I'm doubtful that it makes sense to do DROP INDEX when the
table is going to be dropped completely, even for regular tables.

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