Re: BUG #6699: pg_restore with -j -- doesn't restore view that groups by primary key

I wrote:
>> We could possibly hack something for the special case of rules, but
>> I don't think this would be the last time we hear about this type of
>> issue. I'm inclined to think that the best solution would be to add
>> generic logic to pg_dump that "looks through" any dependency references
>> to objects that are not going to be dumped, and replaces them with the
>> IDs of any objects they depend on that are going to be dumped.

> I wrote a trial patch that does things that way (attached) and it
> appears to work and solve the problem. However, it's not committable
> as-is because it breaks the build of pg_restore:

> ld: Unsatisfied symbols:
> findObjectByDumpId (code)

> since findObjectByDumpId is in common.c which isn't linked into
> pg_restore. I guess a brute force solution would be to link it,
> but probably we ought to think about refactoring the code to avoid
> that.

Actually, that brute force solution doesn't work at all, because
common.c contains many call-backs into pg_dump.c, and we certainly
don't want to link pg_dump.c into pg_restore. I thought for a little
bit about breaking common.c into two pieces, but am not excited about
doing major refactoring on the spur of the moment. (Mind you, this
code probably *needs* wholesale refactoring; I just don't want to do
it as part of a bug fix.)

Another problem I noticed after further thought is that in a partial or
data-only dump, this patch could rearrange dependencies that pg_restore
has hardwired knowledge of, such as the dependency of a TABLE DATA item
on its table. I'm not certain that that would break anything, but it
seems pretty darn dangerous. I think that we don't want to rearrange
any dependencies that are "built in" by pg_dump itself rather than being
obtained from pg_depend --- this includes the TABLE DATA case as well
as COMMENT, ACL, and SECURITY LABEL items' dependencies on their parent
objects.

So attached is another try that puts the work into pg_dump.c. There are
a couple things I don't especially like, but don't see an easy way to
improve on:

* This code would be the first in pg_dump.c that looks through Archive
to ArchiveHandle. Although we've muttered before that that distinction
is useless, doing this here still seems a bit out of place. But we
can't put it in pg_backup_archiver.c because of the findObjectByDumpId
dependence, so I see no better answer.

* To handle the "built in" dependency problem mentioned above, I
introduced a convention that only "built in" dependencies should be
explicitly passed to ArchiveEntry(); everything with regular
dependencies should pass NULL/0, and then we build the real dependencies
at the very end of the archive construction process. This is annoying
because it's easy to do the wrong thing: if you forget and use the old
style of passing the DumpableObject's dependencies to ArchiveEntry, it
will *appear* to work, and only break when/if one of these indirection
cases comes up. A less error-prone convention would be nice, but I
can't think of a good one offhand.

Another thing that might be a showstopper is that this will only work
as-is in HEAD and 9.2, because only since commit
4317e0246c645f60c39e6572644cff1cb03b4c65 do we have explicit advance
marking of which TOC entries are going to get dumped. If we want to
fix this problem this way in older branches, we're going to have to
back-port parts of that commit. I don't actually see any alternative
way to fix it, though. How concerned are we about making this work
in released branches?

Comments?

regards, tom lane