Further cleanup of pg_dump/pg_restore item selection code

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Further cleanup of pg_dump/pg_restore item selection code
Date: 2018-01-25 02:49:37
Message-ID: 32668.1516848577@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

As I've been hacking on the pg_dump code recently, I got annoyed at the
ugliness of its code for deciding whether or not to emit database-related
TOC entries. That logic is implemented in a completely different place
from other TOC entry selection decisions, and it has a bunch of strange
behaviors that can really only be characterized as bugs. An example is
that

pg_dump --create -t sometable somedb

will not emit a CREATE DATABASE command as you might expect, because
the use of -t prevents the DATABASE TOC entry from being made in the
first place. Also,

pg_dump --clean --create -t sometable somedb

will not emit any DROP commands: the code expects that with the
combination of --clean and --create, it should emit only DROP
DATABASE, but nothing happens because there's no DATABASE entry.

The same behaviors occur if you do e.g.

pg_dump -Fc -t sometable somedb | pg_restore --create

which is another undocumented misbehavior: the docs for pg_restore do not
suggest that --create might fail if the source archive was selective.

Another set of problems is that if you use pg_restore's item
selection switches (-t etc), those do not restore ACLs, comments,
or security labels associated with the selected object(s). This
is strange, and unlike the behavior of the comparable switches in
pg_dump, and certainly undocumented.

Attached is a patch that proposes to clean this up. It arranges
things so that CREATE DATABASE (and, if --clean, DROP DATABASE)
is emitted if and only if you said --create, regardless of other
selectivity switches. And it fixes selective pg_restore to include
subsidiary ACLs, comments, and security labels.

This does not go all the way towards making pg_restore's item selection
switches dump subsidiary objects the same as pg_dump would, because
pg_restore doesn't really have enough info to deal with indexes and
table constraints the way pg_dump does. Perhaps we could intuit the
parent table by examining object dependencies, but that would be a
bunch of new logic that seems like fit material for a different patch.
In the meantime I just added a disclaimer to pg_restore.sgml explaining
this.

I also made an effort to make _tocEntryRequired() a little better
organized and better documented. It's grown a lot of different
behaviors over the years without much thought about layout or
keeping related checks together. (There's a chunk in the middle
of the function that now needs to be indented one stop to the right,
but in the interests of keeping the patch reviewable, I've not done
so yet.)

Comments?

regards, tom lane

Attachment Content-Type Size
pg_restore-item-selection-improvements-1.patch text/x-diff 13.5 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2018-01-25 03:12:49 Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump
Previous Message Jing Wang 2018-01-25 02:15:09 Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE