Re: Warn when parallel restoring a custom dump without data offsets

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: David Gilman <dgilman(at)gilslotd(dot)com>
Cc: pgsql-hackers(at)lists(dot)postgresql(dot)org, pryzby(at)telsasoft(dot)com
Subject: Re: Warn when parallel restoring a custom dump without data offsets
Date: 2020-07-15 20:40:59
Message-ID: 3172545.1594845659@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
0000-remove-filePos-tracking.patch text/x-diff 4.8 KB
0001-remember-seek-positions.patch text/x-diff 6.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2020-07-15 21:45:35 Re: gs_group_1 crashing on 13beta2/s390x
Previous Message Christoph Berg 2020-07-15 20:29:00 Re: gs_group_1 crashing on 13beta2/s390x