Horton

Thread

Commits

  1. Clean up some aspects of pg_dump/pg_restore item-selection logic.

  1. Further cleanup of pg_dump/pg_restore item selection code

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-01-25T02:49:37Z

    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

  2. Further cleanup of pg_dump/pg_restore item selection code

    David G. Johnston <david.g.johnston@gmail.com> — 2018-01-25T03:36:05Z

    On Wednesday, January 24, 2018, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> 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
>
>
pg_dump -t:

"The -n and -N switches have no effect when -t is used, because tables
selected by -t will be dumped regardless of those switches, and non-table
objects will not be dumped."

a database is a non-table object...though the proposed behavior seems
reasonable; but maybe worthy of being pointed out just like the -n switches
are.

(probably been asked before but why 'no effect' instead of 'will cause the
dump to fail before it begins'?)


> 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.
>

pg_restore -t:

"When -t is specified, pg_restore makes no attempt to restore any other
database objects that the selected table(s) might depend upon. Therefore,
there is no guarantee that a specific-table restore into a clean database
will succeed."

That -t was specified on the dump instead of the restore could be clarified
but applies nonetheless.

Do we document anywhere what is a "database object" and what are "property
objects" that are restored even when -t is specified?


> 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.


Makes sense, the bit about regardless of other switches probably should
find it's way into the docs.

Adding a drop database where there never was one before is a bit
disconcerting though...even if the switches imply the user should be
expecting one,


>
>  And it fixes selective pg_restore to include
> subsidiary ACLs, comments, and security labels.


+1

David J.

  3. Re: Further cleanup of pg_dump/pg_restore item selection code

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-01-25T04:17:38Z

    "David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Wednesday, January 24, 2018, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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.

> pg_restore -t:

> "When -t is specified, pg_restore makes no attempt to restore any other
> database objects that the selected table(s) might depend upon. Therefore,
> there is no guarantee that a specific-table restore into a clean database
> will succeed."

I think you might be missing one of the main points here, which is
that --create is specified as causing both a CREATE DATABASE and a
reconnect to that database.  If it silently becomes a no-op, which
is what happens today, the restore is likely to go into the wrong
database entirely (or at least not the DB the user expected).

I won't deny the possibility that some people are depending on the
existing wrong behavior, but that's a hazard with any bug fix.

			regards, tom lane

  4. Re: Further cleanup of pg_dump/pg_restore item selection code

    David G. Johnston <david.g.johnston@gmail.com> — 2018-01-25T05:19:03Z

    On Wednesday, January 24, 2018, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>
> 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.
>

Unless we really wanted to keep prior-version compatibility it would seem
more reliable to have pg_dump generate a new TOC entry type describing
these dependencies and have pg_restore read and interpret them during
selective restore mode.  Basically a cross-referencing "if you restore A
also restore B". Where A can only be tables and B indexes and constraints
(if we can get away with it being that limited initially).

David J.

  5. Re: Further cleanup of pg_dump/pg_restore item selection code

    David G. Johnston <david.g.johnston@gmail.com> — 2018-01-25T05:24:02Z

    On Wednesday, January 24, 2018, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I think you might be missing one of the main points here, which is
> that --create is specified as causing both a CREATE DATABASE and a
> reconnect to that database.
>

I missed the implication though I read and even thought about questioning
that aspect (in pg_dump though, not restore).

Should pg_restore fail if asked to --create without a database entry in the
TOC?

David J.

  6. Re: Further cleanup of pg_dump/pg_restore item selection code

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-01-25T17:02:59Z

    "David G. Johnston" <david.g.johnston@gmail.com> writes:
> Should pg_restore fail if asked to --create without a database entry in the
> TOC?

Yeah, I wondered about that too.  This patch makes it a non-issue for
archives created with v11 or later pg_dump, but there's still a hazard
if you're restoring from an archive made by an older pg_dump.  Is it
worth putting in logic to catch that?

Given the lack of field complaints, I felt fixing it going forward is
sufficient, but there's room to argue differently.

			regards, tom lane