Finding and Reporting Postgres Bug #8257

1. Finding and ReportingPostgres Bug #8257 By: Lloyd Albin 8/6/2013

2. The Bug This presentation will describe the finding of a bug in the multi-threaded restores and how the fix was implemented.

3. The PG_RESTORE command When doing a multi-threaded 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. pg_restore -C -j 3 -d postgres -Fc test_db.dump

4. Create a test database Here we create a test database for our test. CREATE DATABASE test_db WITH OWNER = postgres ENCODING = 'UTF8' TEMPLATE = template0;

5. Creating our test table We will create a basic table with just a primary key. We don’t need any other fields for this example. CREATE TABLE public.tbl_test ( pkey TEXT NOT NULL, CONSTRAINT tbl_test_pkey PRIMARY KEY(pkey) );

6. Creating an index on the Primary Key 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. COMMENT ON INDEX public.tbl_test_pkey IS 'Index Comment';

7. Backing up the database 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. pg_dump-Fc test_db > test_db.dump dropdbtest_db

8. Showing the problem 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 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_pkeypostgres 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

9. Submitting the bug The first thing you should do is to search the pgsql-bugs and pgsql-hackers 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:NameEmailPostgreSQL versionOperating SystemShort DescriptionLong Description

10. The submitted bug To see the full bug submission: http://www.postgresql.org/message-id/E1UrzMa-0007Ns-KX@wrigleys.postgresql.org 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: 8257 Logged by: Lloyd Albin Email address: lalbin(at)fhcrc(dot)org PostgreSQL version: 9.2.4 Operating system: SUSE Linux (64-bit)

11. Internals of the dump index 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 FreundPostgreSQL Development2ndQuadrant ; Selected TOC Entries: ... 170; 1259 69261 TABLE public tbl_testandres ; depends on: 6 1941; 0 69261 TABLE DATA public tbl_testandres ; depends on: 170 1833; 2606 69268 CONSTRAINT public tbl_test_pkeyandres ; depends on: 170 170 1950; 0 0 COMMENT public INDEX tbl_test_pkeyandres ; depends on: 1832

12. With the patch So what we need to do is to make the comment depend on the constraint instead. Unsurprisingly after that restore completes. Andres FreundPostgreSQLDevelopment2nd Quadrant http://www.postgresql.org/message-id/20130627080135.GA12870@awork2.anarazel.de 170; 1259 69261 TABLE public tbl_testandres ; depends on: 6 1941; 0 69261 TABLE DATA public tbl_testandres ; depends on: 170 1833; 2606 69268 CONSTRAINT public tbl_test_pkeyandres ; depends on: 170 170 1950; 0 0 COMMENT public INDEX tbl_test_pkeyandres ; depends on: 1833

13. Reviewing the patch #1 Tom Lane points out that the fix will only fix new dumps and not any previous dumps. http://www.postgresql.org/message-id/20760.1372343354@sss.pgh.pa.us 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.

14. Reviewing the patch #2 Andres Freund points out that unfortunately this is the best fix. http://www.postgresql.org/message-id/20130627144315.GL1254@alap2.anarazel.de 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.

15. Reviewing the patch #3 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/message-id/25223.1372353553@sss.pgh.pa.us 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.

16. Patch approved by Tom Lane Tom Lane approved the patch. http://www.postgresql.org/message-id/26518.1372355789@sss.pgh.pa.us 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.

17. Patch committed 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. 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

18. Affected Versions These are the affected versions: 8.4.179.0.139.1.99.2.49.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.