Re: Warn when parallel restoring a custom dump without data offsets
Tom Lane <tgl@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
To: David Gilman <dgilman@gilslotd.com>
Cc: pgsql-hackers@lists.postgresql.org, pryzby@telsasoft.com
Date: 2020-07-15T20:40:59Z
Lists: pgsql-hackers
Attachments
- 0000-remove-filePos-tracking.patch (text/x-diff) patch 0000
- 0001-remember-seek-positions.patch (text/x-diff) patch 0001
I wrote: > The attached 0001 rewrites your 0001 as per the above ideas (dropping > the proposed doc change for now), and includes your 0004 for simplicity. > I'm including your 0002 verbatim just so the cfbot will be able to do a > meaningful test on 0001; but as stated, I don't really want to commit it. I spent some more time testing this, by trying to dump and restore the core regression database. I immediately noticed that I sometimes got "ftell mismatch with expected position -- ftell used" warnings, though it was a bit variable depending on the -j level. The reason was fairly apparent on looking at the code: we had various fseeko() calls in code paths that did not bother to correct ctx->filePos afterwards. In fact, *none* of the four existing fseeko calls in pg_backup_custom.c did so. It's fairly surprising that that hadn't caused a problem up to now. I started to add adjustments of ctx->filePos after all the fseeko calls, but then began to wonder why we don't just rip the variable out entirely. The only places where we need it are to set dataPos for data blocks, but that's an entirely pointless activity if we don't have seek capability, because we're not going to be able to rewrite the TOC to emit the updated values. Hence, the 0000 patch attached rips out ctx->filePos, and then 0001 is the currently-discussed patch rebased on that. I also added an additional refinement, which is to track the furthest point we've scanned to while looking for data blocks in an offset-less file. If we have seek capability, then when we need to resume looking for data blocks we can search forward from that spot rather than wherever we happened to have stopped at. This fixes an additional source of potentially-O(N^2) behavior if we have to restore blocks in a very out-of-order fashion. I'm not sure that it makes much difference in common cases, but with this we can say positively that we don't scan the same block more than once per worker process. I'm still unhappy about the proposed test case (0002), but now I have a more concrete reason for that: it didn't catch this bug, so the coverage is still pretty miserable. Dump-and-restore-the-regression-database used to be a pretty common manual test for pg_dump, but we never got around to automating it, possibly because we figured that the pg_upgrade test script covers that ground. It's becoming gruesomely clear that pg_upgrade is a distinct operating mode that doesn't necessarily have the same bugs. So I'm inclined to feel that what we ought to do is automate a test of that sort; but first we'll have to fix the existing bugs described at [1][2]. Given the current state of affairs, I'm inclined to commit the attached with no new test coverage, and then come back and look at better testing after the other bugs are dealt with. regards, tom lane [1] https://www.postgresql.org/message-id/3169466.1594841366%40sss.pgh.pa.us [2] https://www.postgresql.org/message-id/3170626.1594842723%40sss.pgh.pa.us
Commits
-
Cope with data-offset-less archive files during out-of-order restores.
- 71e8e66f783c 13.0 landed
- f009591d6edd 14.0 landed
- 66232220ee6e 12.4 landed
-
Remove manual tracking of file position in pg_dump/pg_backup_custom.c.
- 447cf2f8e9dc 13.0 landed
- a8d0732ac2b5 14.0 landed
- 39a068ce6675 12.4 landed
-
Improve performance of tuple conversion map generation
- 42f70cd9c3db 12.0 cited
-
Fix pg_restore so parallel restore doesn't fail when the input file doesn't
- b779ea8a9a2d 9.0.0 cited