Horton

Thread

Commits

  1. Fix typo in comment

  2. pg_dump: fix dependencies on FKs to partitioned tables

  1. Dependencies for partitioned indexes are still a mess

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-07-15T19:52:03Z

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

  2. Re: Dependencies for partitioned indexes are still a mess

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-08-12T21:49:18Z

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

  3. Re: Dependencies for partitioned indexes are still a mess

    Andres Freund <andres@anarazel.de> — 2020-08-12T22:13:13Z

    Hi,

On 2020-07-15 15:52:03 -0400, Tom Lane wrote:
> 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).

Yea, we really should have that. IIRC I was trying to add that, and
tests that compare dumps from primary / standby, and failed due to some
differences that were hard to fix.

A quick test with pg_dumpall shows some odd differences after:
1) create new cluster
2) installcheck-parallel
3) drop table gtest30_1, gtest1_1;
4) pg_dumpall > first.sql
5) recreate cluster
6) psql < first.sql > first.sql.log
7) pg_dumpall > second.sql

I've attached the diff between first.sql and second.sql. Here's the
highlights:

@@ -15392,9 +15392,9 @@
 --
 
 CREATE TABLE public.test_type_diff2_c1 (
+    int_two smallint,
     int_four bigint,
-    int_eight bigint,
-    int_two smallint
+    int_eight bigint
 )
 INHERITS (public.test_type_diff2);
...

@@ -39194,10 +39194,10 @@
 -- Data for Name: b_star; Type: TABLE DATA; Schema: public; Owner: andres
 --
 
-COPY public.b_star (class, aa, bb, a) FROM stdin;
-b      3       mumble  \N
+COPY public.b_star (class, aa, a, bb) FROM stdin;
+b      3       \N      mumble
 b      4       \N      \N
-b      \N      bumble  \N
+b      \N      \N      bumble
 b      \N      \N      \N
 \.
 

@@ -323780,7 +323780,7 @@
 -- Data for Name: renamecolumnanother; Type: TABLE DATA; Schema: public; Owner: andres
 --
 
-COPY public.renamecolumnanother (d, a, c, w) FROM stdin;
+COPY public.renamecolumnanother (d, w, a, c) FROM stdin;
 \.
 
 

The primary / standby differences are caused by sequence logging. I
wonder if there's some good way to hide those, or to force them to be
the same between primary / standby, without hiding bugs.

Greetings,

Andres Freund

  4. Re: Dependencies for partitioned indexes are still a mess

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-08-12T22:29:16Z

    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

  5. Re: Dependencies for partitioned indexes are still a mess

    Andres Freund <andres@anarazel.de> — 2020-08-12T22:38:53Z

    Hi,

On 2020-08-12 18:29:16 -0400, Tom Lane wrote:
> 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.

Ugh.  Obviously applications shouldn't use INSERT or SELECT without a
target list, but that still seems somewhat nasty.

I guess we could script it so that we don't compare the "original" with
a restored database, but instead compare the restored version with one
restored from that. But that seems likely to hide bugs.


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?


Greetings,

Andres Freund

  6. Re: Dependencies for partitioned indexes are still a mess

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-08-12T22:48:28Z

    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

  7. Re: Dependencies for partitioned indexes are still a mess

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-08-12T23:13:36Z

    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

  8. Re: Dependencies for partitioned indexes are still a mess

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-08-14T17:30:08Z

    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

  9. Re: Dependencies for partitioned indexes are still a mess

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-08-14T21:35:34Z

    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

  10. Re: Dependencies for partitioned indexes are still a mess

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-09-02T00:49:41Z

    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