Re: BUG #5288: Restoring a 7.4.5 -Fc dump using -j 2 segfaults (patch included)

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Tom Lane wrote:
> "Jon Erdman (aka StuckMojo)" <postgresql(at)thewickedtribe(dot)net> writes:
>> So, I still run 7.4.5 for my medical billing app, and in playing around with
>> 8.5 at AustinPUG last week I discovered that if I try to restore one of my
>> backups from 7.4 (produced with 7.4 pg_dump) into 8.5devel using 8.5
>> pg_restore and -j 2, it immediately segfaults. 8.4 does as well.
>
> Ouch.
>
>> I built 8.5 with debug to get a bt and investigate what's going on, and I
>> found that it's a dependency in the archive TOC that is much higher than the
>> highest dump id in the TOC. This doesn't seem all that odd, considering the
>> comment right above the offending block says there can be deps to things
>> that aren't in the archive. This causes the code to index way off the end of
>> the array of TOC entries by dumpId.
>
> No, the problem is that back then the dependencies meant something
> completely different; they were object OIDs not dump ID numbers.

That makes much more sense, I had a feeling it was something like that
based on how far outside the range that particular value was.

> I think what we have to do is forbid -j restores from archives older
> than archive version 1.8 (ie, pg_dump 8.0). It doesn't seem worth it
> to try to support parallel restore from nearly-obsolete versions, and
> I suspect that we couldn't do it even if we tried --- the reason the
> representation got changed is that the old way simply didn't work for
> any significant use of the dependency info. Just ignoring the
> dependencies, as your patch effectively proposes, is going to lead to
> restore failures or worse.

Mind if I take a crack at this patch? I spent a few hours poking around
in that section of the code, and I think I should be able to find the
right place to make the check without too much trouble. Assuming you
haven't already written it of course, knowing how quick you tend to be ;)

> It seems like a good idea to put a range check into that loop as well,
> but I think it should throw error not silently ignore bad data ...

What I'm curious about is the comment above that loop:

/*
* It is possible that the dependencies list items that are not in the
* archive at all. Subtract such items from the depCounts.
*/

If things can be referenced that are not even in the dump, wouldn't it
be somewhat likely that the dependency dumpId could be outside the range
of Ids found in the TOC (and thus outside the range of the array,
requiring a range check)? And if it's better to return an error in that
case, why does this loop exist at all in it's current form?

Nice to be able to pay you back for a patch you did at my request in 7.4
to help me port from MS SQL, which was supporting column defaults on
views...
- --

Jon T Erdman (aka StuckMojo)
PostgreSQL Zealot
-----BEGIN PGP SIGNATURE-----

iEYEARECAAYFAktV84QACgkQRAk1+p0GhSFpvgCgiom9Dzre5R2bUlast+dRhmNu
nggAoJXWJ6FiOzaRH7PumLYsq1S6e3Sw
=qRyU
-----END PGP SIGNATURE-----