Thread: pg_restore -t should match views, matviews, and foreign tables
Following on from this -bugs post:
this patch adds support for views, foreign tables, and materialised views to the pg_restore -t flag.
Attachment
On 3/31/15 11:01 PM, Craig Ringer wrote: > Following on from this -bugs post: > > http://www.postgresql.org/message-id/CAMsr+YGJ50TvTVK4Dbp66gAjeOC0KaP6KXFEHAOM+neQmHvoQA@mail.gmail.com > > this patch adds support for views, foreign tables, and materialised > views to the pg_restore -t flag. I think this is a good change. Any concerns?
Peter Eisentraut <peter_e@gmx.net> writes:
> On 3/31/15 11:01 PM, Craig Ringer wrote:
>> this patch adds support for views, foreign tables, and materialised
>> views to the pg_restore -t flag.
> I think this is a good change. Any concerns?
Are we happy with pg_dump/pg_restore not distinguishing these objects
by type? It seems rather analogous to letting ALTER TABLE work on views
etc. Personally I'm fine with this, but certainly some people have
complained about that approach so far as ALTER is concerned. (But the
implication would be that we'd need four distinct switches, which is
not an outcome I favor.)
Also, I think you missed "MATERIALIZED VIEW DATA".
Also, shouldn't there be a documentation update?
regards, tom lane
Peter Eisentraut <peter_e@gmx.net> writes:
> On 3/31/15 11:01 PM, Craig Ringer wrote:
>> this patch adds support for views, foreign tables, and materialised
>> views to the pg_restore -t flag.
> I think this is a good change. Any concerns?
Are we happy with pg_dump/pg_restore not distinguishing these objects
by type? It seems rather analogous to letting ALTER TABLE work on views
etc. Personally I'm fine with this, but certainly some people have
complained about that approach so far as ALTER is concerned. (But the
implication would be that we'd need four distinct switches, which is
not an outcome I favor.)
The pg_dump documentation for the equivalent "-t" switch states:
"Dump only tables (or views or sequences or foreign tables) matching table"
Does pg_dump need to be updated to address materialized views here?
Does pg_restore need to be updated to address sequences here?
ISTM that the two should mirror each other.
David J.
On 8 April 2015 at 04:33, Tom Lane <tgl@sss.pgh.pa.us> wrote:
My reasoning was that these are all relations that, as far as SELECT et al are concerned, can be interchangeable.
Yes. Again, amended.
I've also added mention of materialized views to the pg_dump docs for --table, which omitted them.
Peter Eisentraut <peter_e@gmx.net> writes:
> On 3/31/15 11:01 PM, Craig Ringer wrote:
>> this patch adds support for views, foreign tables, and materialised
>> views to the pg_restore -t flag.
> I think this is a good change. Any concerns?
Are we happy with pg_dump/pg_restore not distinguishing these objects
by type? It seems rather analogous to letting ALTER TABLE work on views
etc. Personally I'm fine with this, but certainly some people have
complained about that approach so far as ALTER is concerned. (But the
implication would be that we'd need four distinct switches, which is
not an outcome I favor.)
My reasoning was that these are all relations that, as far as SELECT et al are concerned, can be interchangeable.
I guess this is more like the ALTER TABLE case though - if you "pg_restore -t" a view, you don't get the data from any table(s) it depends on. So substituting a table for a view won't be transparent to the user anyway.
I mostly just don't see the point of requiring multiple flags for things that are all in the same namespace. It'll mean new flags each time we add some new object type, more logic in apps that invoke pg_restore, etc, and for what seems like no meaningful gain. We'll just land up with "No table 'viewblah' matched, did you mean -V 'viewblah'? "
Also, I think you missed "MATERIALIZED VIEW DATA".
Thanks, amended.
Also, shouldn't there be a documentation update?
Yes. Again, amended.
(It's rather unfortunate that pg_restore's -t is completely different to pg_dump's -t . Fixing that would involve implementing wildcard search support in pg_restore and would break backward compatibility, though).
Attachment
On 8 April 2015 at 05:05, David G. Johnston <david.g.johnston@gmail.com> wrote:
Peter Eisentraut <peter_e@gmx.net> writes:
> On 3/31/15 11:01 PM, Craig Ringer wrote:
>> this patch adds support for views, foreign tables, and materialised
>> views to the pg_restore -t flag.
> I think this is a good change. Any concerns?
Are we happy with pg_dump/pg_restore not distinguishing these objects
by type? It seems rather analogous to letting ALTER TABLE work on views
etc. Personally I'm fine with this, but certainly some people have
complained about that approach so far as ALTER is concerned. (But the
implication would be that we'd need four distinct switches, which is
not an outcome I favor.)The pg_dump documentation for the equivalent "-t" switch states:"Dump only tables (or views or sequences or foreign tables) matching table"Does pg_dump need to be updated to address materialized views here?
The pg_dump code handles materialized views, the docs weren't updated. I added mention of them in the next rev of the patch to pg_restore.
Does pg_restore need to be updated to address sequences here?
I'd be against that if pg_dump didn't already behave the same way. Given that, yes, I think so.
ISTM that the two should mirror each other.
Ideally, yes, but the differences go much deeper than this.
to get the equivalent of:
pg_restore -n myschema -t sometable
in pg_dump you need:
pg_dump -t "\"myschema\".\"sometable\""
pg_dump -n myschema -t sometable is **not** equivalent. In fact, the -n is ignored, and -t will match using the search_path.
so they're never really going to be the same, just similar enough to catch people out most of the time.
I think you're right that sequences should be included by pg_restore since they are by pg_dump, though. So v3 patch attached.
Attachment
Hi
I am sending a review of this trivial patch. + strcmp(te->desc, "TABLE DATA") == 0 ||
+ strcmp(te->desc, "VIEW") == 0 ||
+ strcmp(te->desc, "FOREIGN TABLE") == 0 ||
+ strcmp(te->desc, "MATERIALIZED VIEW") == 0 ||
+ strcmp(te->desc, "MATERIALIZED VIEW DATA") == 0 ||
+ strcmp(te->desc, "SEQUENCE") == 0)
-S, --superuser=NAME superuser user name to use for disabling triggers
-t, --table=NAME restore named table
-T, --trigger=NAME restore named trigger
5. All tests passed
6. There are no tests. But pg_dump related sw has not any tests yet.
I don't see any issues - this patch is really trivial without risks. It is working already on pg_dump side, so the fix on pg_restore side is natural.
Regards
Pavel
2015-04-01 5:01 GMT+02:00 Craig Ringer <craig@2ndquadrant.com>:
Following on from this -bugs post:this patch adds support for views, foreign tables, and materialised views to the pg_restore -t flag.--
Craig Ringer <craig@2ndquadrant.com> writes:
> I think you're right that sequences should be included by pg_restore since
> they are by pg_dump, though. So v3 patch attached.
You forgot "SEQUENCE SET" :-(. I fixed that and adjusted the docs a bit
more and committed.
regards, tom lane