Transcript Postgres Bug #8257

Finding and Reporting
Postgres Bug #8257
BY: LLOYD ALBIN
8/6/2013
The Bug
This presentation will describe the finding of a bug in the multithreaded restores and how the fix was implemented.
The PG_RESTORE command
pg_restore -C -j 3 -d postgres -Fc test_db.dump
When doing a multithreaded restore, you
specify the –j option
and specify the number
of jobs / threads / cores.
In this example there
will be three threads
spawned by the parent
process.
You may only use the –j
option with custom or
directory backups.
Create a test database
CREATE DATABASE test_db
WITH OWNER = postgres
ENCODING = 'UTF8'
TEMPLATE = template0;
Here we create a test
database for our test.
Creating our test table
CREATE TABLE public.tbl_test (
pkey TEXT NOT NULL,
CONSTRAINT tbl_test_pkey PRIMARY KEY(pkey)
);
We will create a basic
table with just a primary
key. We don’t need any
other fields for this
example.
Creating an index on the Primary Key
COMMENT ON INDEX public.tbl_test_pkey
IS 'Index Comment';
We can now add a
comment to the index.
This is the whole root of
the problem for the
multi-threaded restore.
If you don’t have any
comment on
automatically created
index’s, then you won’t
have any issues with the
multi-threaded restore.
Backing up the database
pg_dump -Fc test_db > test_db.dump
dropdb test_db
We don’t need any data
for our example, so we
are now ready to backup
the database. Once the
database if backed up
we can go ahead and
drop it.
Showing the problem
pg_restore -C -j 3 -d postgres -Fc test_db.dump
pg_restore: [archiver (db)] Error while PROCESSING TOC:
pg_restore: [archiver (db)] Error from TOC entry 2525; 0 0
COMMENT INDEX
tbl_test_pkey postgres
pg_restore: [archiver (db)] could not execute query:
ERROR: relation
"tbl_test_pkey" does not exist
Command was: COMMENT ON INDEX tbl_test_pkey IS
'Index Comment';
pg_restore: [archiver] worker process failed: exit code 1
What happed is the
restore tried to add the
comment before the
index was created.
I have tested this with
Postgres 9.0.12 & 9.2.4
Submitting the bug
The first thing you should do is to search the pgsql-bugs and pgsqlhackers mailing lists for your problem. If you don’t find it, then go ahead
and submit a bug ticket.
http://www.postgresql.org/support/submitbug
The form will ask you for the following information:
Name
Email
PostgreSQL version
Operating System
Short Description
Long Description
The submitted bug
From:
lalbin(at)fhcrc(dot)org
To:
pgsql-bugs(at)postgresql(dot)org
Subject: BUG #8257: Multi-Core Restore fails when
containing index comments
Date:
2013-06-26 23:43:00
The following bug has been logged on the website:
Bug reference:
Logged by:
Email address:
PostgreSQL version:
Operating system:
8257
Lloyd Albin
lalbin(at)fhcrc(dot)org
9.2.4
SUSE Linux (64-bit)
To see the full bug
submission:
http://www.postgresql.
org/messageid/[email protected]
.org
Internals of the dump index
; Selected TOC Entries:
...
170; 1259 69261 TABLE public tbl_test andres
; depends on: 6
1941; 0 69261 TABLE DATA public tbl_test andres
;
depends on: 170
1833; 2606 69268 CONSTRAINT public
tbl_test_pkey andres
;
depends on: 170 170
1950; 0 0 COMMENT public INDEX tbl_test_pkey
andres
;
depends on: 1832
The problem is that
pg_dump makes the
comment depend on
the index instead of the
constraint:
There is no object 1832
in the dump since that
was ommitted in favor
of the constraint 1833
which internally creates
the index.
Andres Freund
PostgreSQL Development
2nd Quadrant
With the patch
170; 1259 69261 TABLE public tbl_test andres
; depends on: 6
1941; 0 69261 TABLE DATA public tbl_test andres
;
depends on: 170
1833; 2606 69268 CONSTRAINT public
tbl_test_pkey andres
;
depends on: 170 170
1950; 0 0 COMMENT public INDEX tbl_test_pkey
andres
;
depends on: 1833
So what we need to do is
to make the comment
depend on the
constraint instead.
Unsurprisingly after
that restore completes.
Andres Freund
PostgreSQL Development
2nd Quadrant
http://www.postgresql.
org/messageid/20130627080135.GA12
[email protected]
e
Reviewing the patch #1
Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> The problem is that pg_dump makes the comment depend on the
index instead of the constraint:
Yeah, I figured that out yesterday, but hadn't gotten to writing a patch
yet.
> ... So what we need to do is to make the comment depend on the
constraint instead.
Your proposed patch will only fix the problem for dumps created after
it ships. In the past, we've tried to deal with this type of issue by
having pg_restore fix up the dependencies when reading a dump, so that
it would still work on existing dumps.
I'm afraid there may be no way to do that in this case --- it doesn't
look like there's enough info in the dump to tell where the dependency
link should have led. But we should think about it a little before
taking the easy way out.
Tom Lane points out
that the fix will only fix
new dumps and not any
previous dumps.
http://www.postgresql.
org/messageid/20760.1372343354@ss
s.pgh.pa.us
Reviewing the patch #2
On 2013-06-27 10:29:14 -0400, Tom Lane wrote:
> > ... So what we need to do is to make the comment depend on the
constraint instead.
> Your proposed patch will only fix the problem for dumps created after it
ships. In the past, we've tried to deal with this type of issue by having
pg_restore fix up the dependencies when reading a dump, so that it would
still work on existing dumps.
Yes :(. On the other hand, it's probably not too common to create comments
on indexes that haven't been created explicitly.
> I'm afraid there may be no way to do that in this case --- it doesn't look
like there's enough info in the dump to tell where the dependency link
should have led. But we should think about it a little before taking the easy
way out.
The only thing I could think of - but which I thought to be too kludgey - was
to simply delay the creation of all comments and restore them together with
ACLs. I don't think we can have dependencies towards comments.
Andres Freund points
out that unfortunately
this is the best fix.
http://www.postgresql.
org/messageid/20130627144315.GL125
[email protected]
Reviewing the patch #3
Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2013-06-27 10:29:14 -0400, Tom Lane wrote:
>> Your proposed patch will only fix the problem for dumps created after it ships. In
the past, we've tried to deal with this type of issue by having pg_restore fix up the
dependencies when reading a dump, so that it would still work on existing dumps.
> Yes :(. On the other hand, it's probably not too common to create comments on
indexes that haven't been created explicitly.
Perhaps. The lack of previous complaints does suggest this situation isn't so common.
>> I'm afraid there may be no way to do that in this case --- it doesn't look like
there's enough info in the dump to tell where the dependency link should have led.
But we should think about it a little before taking the easy way out.
> The only thing I could think of - but which I thought to be too kludgey - was to
simply delay the creation of all comments and restore them together with ACLs.
I don't like that either, though we may be forced into it if we find more bugs in
comment dependencies.
Anyway, fixing pg_dump's logic is not wrong; I was just hoping we could also think of
a workaround on the pg_restore side.
Tom Lane, says that
Andres was correct with
patching pg_dump but
wished that pg_restore
could also have been
patched to support the
bad dumps but agrees
that the only way to fix
it is not a good
workaround.
http://www.postgresql.
org/messageid/25223.1372353553@sss
.pgh.pa.us
Patch approved by Tom Lane
Andres Freund <andres(at)2ndquadrant(dot)com>
writes:
> There is no object 1832 in the dump since that
was ommitted in favor of the constraint 1833 which
internally creates the index. So what we need to do
is to make the comment depend on the constraint
instead.
> With the attached patch we get: [ the right thing ]
Applied with minor cosmetic changes.
Tom Lane approved the
patch.
http://www.postgresql.
org/messageid/26518.1372355789@ss
s.pgh.pa.us
Patch committed
Mark index-constraint comments with correct dependency in pg_dump.
When there's a comment on an index that was created with UNIQUE or PRIMARY KEY
constraint syntax, we need to label the comment as depending on the constraint not the
index, since only the constraint object actually appears in the dump. This incorrect
dependency can lead to parallel pg_restore trying to restore the comment before the index
has been created, per bug #8257 from Lloyd Albin.
This patch fixes pg_dump to produce the right dependency in dumps made in the future.
Usually we also try to hack pg_restore to work around bogus dependencies, so that
existing (wrong) dumps can still be restored in parallel mode; but that doesn't seem
practical here since there's no easy way to relate the constraint dump entry to the
comment after the fact.
Andres Freund
Branch
-----REL9_3_STABLE
REL9_2_STABLE
REL9_1_STABLE
REL9_0_STABLE
REL8_4_STABLE
master
You can view all the
committed patched on
the pgsql-committers
mailing list.
Each branch patch is a
separate email on the
pgsql-committers
mailing list.
Affected Versions
These are the affected versions:
8.4.17
9.0.13
9.1.9
9.2.4
9.3 Beta 1 and possibly Beta 2 since it was released the same day as the
patches were committed.
It is affecting all current versions 8.4+ as of 8/6/2013.