Thread: add_missing_from breaks existing views
Sample case:
regression=# create table t1(f1 int, f2 int);
CREATE TABLE
regression=# set add_missing_from = true;
SET
regression=# create view v1 as select t1.*;
NOTICE: adding missing FROM-clause entry for table "t1"
CREATE VIEW
regression=# \d v1 View "public.v1"Column | Type | Modifiers
--------+---------+-----------f1 | integer |f2 | integer |
View definition:SELECT t1.f1, t1.f2;
The problem with this is that pg_dump will dump the view exactly like
that:
$ pg_dump -t v1 regression
...
--
-- Name: v1; Type: VIEW; Schema: public; Owner: postgres
--
CREATE VIEW v1 AS SELECT t1.f1, t1.f2;
ALTER TABLE public.v1 OWNER TO postgres;
and therefore the dump will fail to load into a machine with
add_missing_from set to false.
What I suggest we do about this is change addImplicitRTE() to set
inFromCl true for implicitly added RTEs, so that the view rule will
later be dumped as if the query had been written per spec.
The problem with this is that it does not retroactively fix existing
dumps (and pg_dump can't force the backend to list the view correctly,
so "use 8.1 pg_dump" is no answer). That leaves us with two not very
appealing choices:
1. Tell people they may have to set add_missing_from = true to reload
a dump that contains such views.
2. Revert the change to make add_missing_from default as false, and
wait a few more releases before making it default.
Comments?
regards, tom lane
On Tue, 2005-25-10 at 17:43 -0400, Tom Lane wrote: > What I suggest we do about this is change addImplicitRTE() to set > inFromCl true for implicitly added RTEs, so that the view rule will > later be dumped as if the query had been written per spec. Sounds reasonable. I wonder if this should be backpatched -- ISTM the proper representation of the view is with an explicit FROM list anyway, and making the correctness of view definition dependent on a GUC variable is only asking for trouble. > 1. Tell people they may have to set add_missing_from = true to reload > a dump that contains such views. pg_dump could itself specify "SET add_missing_from = true" before view definitions. However, that is definitely a hack :( -Neil
On Tue, 2005-10-25 at 17:43 -0400, Tom Lane wrote: > 1. Tell people they may have to set add_missing_from = true to reload > a dump that contains such views. > > 2. Revert the change to make add_missing_from default as false, and > wait a few more releases before making it default. > > Comments? This needs to be (1). Whichever version we make the change for, there will always be a point where the pg_dump would be taken with add_missing_from=true by default and moving to new version with add_missing_from=false. Putting that off for another release will still allow that failure condition to exist at that later time: so we must tell people about it now. Best Regards, Simon Riggs
On 2005-10-25, Simon Riggs <simon@2ndquadrant.com> wrote: > On Tue, 2005-10-25 at 17:43 -0400, Tom Lane wrote: > >> 1. Tell people they may have to set add_missing_from = true to reload >> a dump that contains such views. >> >> 2. Revert the change to make add_missing_from default as false, and >> wait a few more releases before making it default. >> >> Comments? > > This needs to be (1). > > Whichever version we make the change for, there will always be a point > where the pg_dump would be taken with add_missing_from=true by default > and moving to new version with add_missing_from=false. Putting that off > for another release will still allow that failure condition to exist at > that later time: so we must tell people about it now. Wild idea: how about having pg_dump include SET add_missing_from = true; in the dump file if, and only if, it is set that way in the server? Perhaps do this only for server versions <8.1... and ensure that dumps from an 8.1 server include the explicit table names... -- Andrew, Supernews http://www.supernews.com - individual and corporate NNTP services
> 2. Revert the change to make add_missing_from default as false, and > wait a few more releases before making it default. +1 No skin off our nose. What do we care if the default changes in a few releases time - however there are probably many end-users who will see problems upgrading... Chris
Neil Conway <neilc@samurai.com> writes:
> On Tue, 2005-25-10 at 17:43 -0400, Tom Lane wrote:
>> What I suggest we do about this is change addImplicitRTE() to set
>> inFromCl true for implicitly added RTEs, so that the view rule will
>> later be dumped as if the query had been written per spec.
> Sounds reasonable. I wonder if this should be backpatched -- ISTM the
> proper representation of the view is with an explicit FROM list anyway,
I think it'd be reasonable to back-patch it into the branches that have
the add_missing_from variable (how far back did we add that?). But that
won't fix the problem with incompatible dump files from existing
installations.
regards, tom lane
Andrew - Supernews <andrew+nonews@supernews.com> writes:
> Wild idea: how about having pg_dump include SET add_missing_from = true;
> in the dump file if, and only if, it is set that way in the server?
Uh, no ... the global setting of add_missing_from does *not* tell you
anything about whether there exist views in the database that were
created under a different setting.
regards, tom lane
On 2005-10-26, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Andrew - Supernews <andrew+nonews@supernews.com> writes: >> Wild idea: how about having pg_dump include SET add_missing_from = true; >> in the dump file if, and only if, it is set that way in the server? > > Uh, no ... the global setting of add_missing_from does *not* tell you > anything about whether there exist views in the database that were > created under a different setting. I realize that; but is it also not the case that someone who creates a view that requires add_missing_from, and then turns it off, has _already_ broken dump+restore on his own database? i.e. people who turn off add_missing_from intentionally are not likely to be using it. -- Andrew, Supernews http://www.supernews.com - individual and corporate NNTP services
Andrew - Supernews <andrew+nonews@supernews.com> writes:
> On 2005-10-26, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Uh, no ... the global setting of add_missing_from does *not* tell you
>> anything about whether there exist views in the database that were
>> created under a different setting.
> I realize that; but is it also not the case that someone who creates a
> view that requires add_missing_from, and then turns it off, has _already_
> broken dump+restore on his own database?
No, because we consider that a client-local setting. This argument is
akin to saying that if a client loads some data with client_encoding FOO
into a database with server_encoding BAR, we are not responsible for
dumping and reloading the data correctly.
In hindsight I think there's no doubt that we blew it in not making
ruleutils.c reverse-list implicit RTEs some time ago. The handwriting
has been on the wall for that "feature" for a good while, and so we
should long ago have thought about how to migrate nonstandard views
to standard syntax. We missed the bet, though, so the question is now
how to cover our mistake. Pretending it's the user's mistake isn't
an answer that fits down my craw very well...
regards, tom lane
On 2005-10-26, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Andrew - Supernews <andrew+nonews@supernews.com> writes: >> On 2005-10-26, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Uh, no ... the global setting of add_missing_from does *not* tell you >>> anything about whether there exist views in the database that were >>> created under a different setting. > >> I realize that; but is it also not the case that someone who creates a >> view that requires add_missing_from, and then turns it off, has _already_ >> broken dump+restore on his own database? > > No, because we consider that a client-local setting. This argument is > akin to saying that if a client loads some data with client_encoding FOO > into a database with server_encoding BAR, we are not responsible for > dumping and reloading the data correctly. 8.0: test=# show add_missing_from;add_missing_from ------------------off (1 row) test=# set add_missing_from to true; SET test=# create view v1 as select test.*; CREATE VIEW test=# \q % pg_dump -U pgsql -s -d test | psql -U pgsql -d test2 [...] ERROR: missing FROM-clause entry for table "test" ERROR: relation "public.v1" does not exist Looks broken to me. I wasn't arguing that the broken behaviour was correct, merely that it exists. > In hindsight I think there's no doubt that we blew it in not making > ruleutils.c reverse-list implicit RTEs some time ago. Obviously. Isn't hindsight wonderful. > Pretending it's the user's mistake isn't > an answer that fits down my craw very well... I'm not claiming it's the user's mistake. My point is that if the user did in fact remove add_missing_from after creating views that depend on it, then they have already run into a bug. -- Andrew, Supernews http://www.supernews.com - individual and corporate NNTP services
Should we allow CREATE VIEW to run with "add_missing_from = true" until we fix CREATE VIEW to handle this cleanly? We emit a warning when we add a missing FROM too, as I remember. --------------------------------------------------------------------------- Andrew - Supernews wrote: > On 2005-10-26, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Andrew - Supernews <andrew+nonews@supernews.com> writes: > >> On 2005-10-26, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >>> Uh, no ... the global setting of add_missing_from does *not* tell you > >>> anything about whether there exist views in the database that were > >>> created under a different setting. > > > >> I realize that; but is it also not the case that someone who creates a > >> view that requires add_missing_from, and then turns it off, has _already_ > >> broken dump+restore on his own database? > > > > No, because we consider that a client-local setting. This argument is > > akin to saying that if a client loads some data with client_encoding FOO > > into a database with server_encoding BAR, we are not responsible for > > dumping and reloading the data correctly. > > 8.0: > > test=# show add_missing_from; > add_missing_from > ------------------ > off > (1 row) > > test=# set add_missing_from to true; > SET > test=# create view v1 as select test.*; > CREATE VIEW > test=# \q > > % pg_dump -U pgsql -s -d test | psql -U pgsql -d test2 > > [...] > ERROR: missing FROM-clause entry for table "test" > ERROR: relation "public.v1" does not exist > > Looks broken to me. > > I wasn't arguing that the broken behaviour was correct, merely that it > exists. > > > In hindsight I think there's no doubt that we blew it in not making > > ruleutils.c reverse-list implicit RTEs some time ago. > > Obviously. Isn't hindsight wonderful. > > > Pretending it's the user's mistake isn't > > an answer that fits down my craw very well... > > I'm not claiming it's the user's mistake. My point is that if the user > did in fact remove add_missing_from after creating views that depend on it, > then they have already run into a bug. > > -- > Andrew, Supernews > http://www.supernews.com - individual and corporate NNTP services > > ---------------------------(end of broadcast)--------------------------- > TIP 1: if posting/reading through Usenet, please send an appropriate > subscribe-nomail command to majordomo@postgresql.org so that your > message can get through to the mailing list cleanly > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Should we allow CREATE VIEW to run with "add_missing_from = true" until
> we fix CREATE VIEW to handle this cleanly?
No. Not only is that horridly ugly, it doesn't fix the problem, because
CREATE VIEW is just one case (see also CREATE RULE). If we were to
defeat add_missing_from for both, there wouldn't be much left of it at
all. Going back to default-true would be far cleaner.
regards, tom lane
Andrew - Supernews <andrew+nonews@supernews.com> writes:
> On 2005-10-26, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Pretending it's the user's mistake isn't
>> an answer that fits down my craw very well...
> I'm not claiming it's the user's mistake. My point is that if the user
> did in fact remove add_missing_from after creating views that depend on it,
> then they have already run into a bug.
No, you're looking at this in the wrong direction. The problem is that
the user hasn't had to do anything so far, because add_missing_from has
defaulted to true in every prior release. So he could have been sailing
along with views written in the old style up to now, and not noticed any
problem. We are creating the problem by changing the default behavior
... or at least, that's how it will look to people who get burnt by this.
After sleeping on it, I feel that we should probably just fix the code
(to make the problem go away going forward) and document the possible
need to turn on add_missing_from to load old dump files as an
incompatibility. We've had worse ones.
regards, tom lane
Tom, > After sleeping on it, I feel that we should probably just fix the code > (to make the problem go away going forward) and document the possible > need to turn on add_missing_from to load old dump files as an > incompatibility. We've had worse ones. I'll agree with you here, for (1) reason: if we put this off for another release, the situation's not going to get any better. Eventually, for some release, we need to break something, because we can't fix the past. -- Josh Berkus Aglio Database Solutions San Francisco
On Wed, Oct 26, 2005 at 11:41:56AM -0400, Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Should we allow CREATE VIEW to run with "add_missing_from = true" > > until we fix CREATE VIEW to handle this cleanly? > > No. Not only is that horridly ugly, it doesn't fix the problem, > because CREATE VIEW is just one case (see also CREATE RULE). If we > were to defeat add_missing_from for both, there wouldn't be much > left of it at all. Going back to default-true would be far cleaner. A VIEW that depends on add-missing-from is a clear case of pilot error. We can document this, warn people, and turn it off by default. Is there some fairly simple way to find VIEWs that have this property so they can be fixed? Cheers, D -- David Fetter david@fetter.org http://fetter.org/ phone: +1 510 893 6100 mobile: +1 415 235 3778 Remember to vote!
On 2005-10-26, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Andrew - Supernews <andrew+nonews@supernews.com> writes: >> On 2005-10-26, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Pretending it's the user's mistake isn't >>> an answer that fits down my craw very well... > >> I'm not claiming it's the user's mistake. My point is that if the user >> did in fact remove add_missing_from after creating views that depend on it, >> then they have already run into a bug. > > No, you're looking at this in the wrong direction. It's quite possible that in trimming my messages for posting I'm removing too much of the context; is that the case? To recap: - you pointed out that there was an incompatibility. - I pointed out a way in which that incompatibility can be substantially reduced in scope, from affecting "everyone who has views defined using add_missing_from" to only affecting "everyone who has views defined using add_missing_from but who has subsequently turned that off, in spite of the bugs that they would encounter in doing so". - you respond by saying there is an incompatibility. Now, I don't know how I can possibly be clearer about this. I know that changing the default add_missing_from causes an incompatibility. If you prefer to keep it, rather than use a solution which will work for everyone who (a) isn't already vulnerable to dump+restore problems and (b) will use 8.1's pg_dump to upgrade, then personally I couldn't care less. I'm just surprised by the attitude. -- Andrew, Supernews http://www.supernews.com - individual and corporate NNTP services
I wrote:
> Neil Conway <neilc@samurai.com> writes:
>> On Tue, 2005-25-10 at 17:43 -0400, Tom Lane wrote:
>>> What I suggest we do about this is change addImplicitRTE() to set
>>> inFromCl true for implicitly added RTEs, so that the view rule will
>>> later be dumped as if the query had been written per spec.
>> Sounds reasonable. I wonder if this should be backpatched -- ISTM the
>> proper representation of the view is with an explicit FROM list anyway,
> I think it'd be reasonable to back-patch it into the branches that have
> the add_missing_from variable (how far back did we add that?).
I've committed the change, but only in HEAD. Even as late as 8.0,
it makes the regression tests fail all over the place, because
warnAutoRange gets confused (it's depending on the old behavior
of inFromCl). We could possibly rejigger the code to avoid this,
but I think the wiser course is to leave the back branches alone.
BTW, I noticed an interesting factoid: addImplicitRTE was never modified
to obey the SQL_inheritance parameter, so you always get an ONLY
reference:
regression=# set add_missing_from TO 1;
SET
regression=# create view vv as select int8_tbl.*;
NOTICE: adding missing FROM-clause entry for table "int8_tbl"
CREATE VIEW
regression=# \d vv View "public.vv"Column | Type | Modifiers
--------+--------+-----------q1 | bigint |q2 | bigint |
View definition:SELECT int8_tbl.q1, int8_tbl.q2 FROM ONLY int8_tbl;
Considering that this is strictly a legacy feature, we probably should
not change its behavior now ... but it does seem a tad inconsistent.
regards, tom lane
On Wednesday 26 October 2005 15:33, Andrew - Supernews wrote:
> On 2005-10-26, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Andrew - Supernews <andrew+nonews@supernews.com> writes:
> >> On 2005-10-26, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >>> Pretending it's the user's mistake isn't
> >>> an answer that fits down my craw very well...
> >>
> >> I'm not claiming it's the user's mistake. My point is that if the user
> >> did in fact remove add_missing_from after creating views that depend on
> >> it, then they have already run into a bug.
> >
> > No, you're looking at this in the wrong direction.
>
> It's quite possible that in trimming my messages for posting I'm removing
> too much of the context; is that the case? To recap:
>
> - you pointed out that there was an incompatibility.
>
> - I pointed out a way in which that incompatibility can be substantially
> reduced in scope, from affecting "everyone who has views defined using
> add_missing_from" to only affecting "everyone who has views defined using
> add_missing_from but who has subsequently turned that off, in spite of the
> bugs that they would encounter in doing so".
>
> - you respond by saying there is an incompatibility.
>
> Now, I don't know how I can possibly be clearer about this. I know that
> changing the default add_missing_from causes an incompatibility. If you
> prefer to keep it, rather than use a solution which will work for everyone
> who (a) isn't already vulnerable to dump+restore problems and (b) will use
> 8.1's pg_dump to upgrade, then personally I couldn't care less. I'm just
> surprised by the attitude.
Actually I rather liked your solution, except that it probably doesn't do
enough to get people off that setting as others want. Ie. I generally run
with that setting on, but I certainly don't make views/rules/etc.. that rely
on that setting. Still in your scenario, when upgrading to 8.1 I'd still end
up with the setting on rather than the intended default of off.
--
Robert Treat
Build A Brighter Lamp :: Linux Apache {middleware} PostgreSQL
On 2005-10-30, Robert Treat <xzilla@users.sourceforge.net> wrote: > Actually I rather liked your solution, except that it probably doesn't do > enough to get people off that setting as others want. Ie. I generally run > with that setting on, but I certainly don't make views/rules/etc.. that rely > on that setting. Still in your scenario, when upgrading to 8.1 I'd still end > up with the setting on rather than the intended default of off. You misunderstand. My suggestion was to set add_missing_from _in the output of pg_dump_ (or pg_restore) so that affected views would restore correctly. This would affect only the session in which the dump was restored. The changes that were already suggested (and since implemented, iiuc) would then make this unnecessary for future dumps/restores, and there would be no question of setting add_missing_from globally in the 8.1 database (unless the admin did that explicitly, themselves). -- Andrew, Supernews http://www.supernews.com - individual and corporate NNTP services
Andrew, > My suggestion was to set add_missing_from _in the output of pg_dump_ (or > pg_restore) so that affected views would restore correctly. This would > affect only the session in which the dump was restored. The changes that > were already suggested (and since implemented, iiuc) would then make this > unnecessary for future dumps/restores, and there would be no question of > setting > add_missing_from globally in the 8.1 database (unless the admin did that > explicitly, themselves). That sounds good if it works. However ... will an add_missing_from view work if add_missing_from is off in the session where the query is executed? If not, then I don't really find this a solution. In that case, it would be better to supply a script that allows users to find add_missing_from dependant views so they can fix them. -- Josh Berkus Aglio Database Solutions San Francisco
Josh Berkus <josh@agliodbs.com> writes:
> That sounds good if it works. However ... will an add_missing_from view work
> if add_missing_from is off in the session where the query is executed?
Yes, you only need to get it past the parser during CREATE VIEW.
regards, tom lane
Tom, > > That sounds good if it works. However ... will an add_missing_from view > > work if add_missing_from is off in the session where the query is > > executed? > > Yes, you only need to get it past the parser during CREATE VIEW. Then the proposal sounds like a good solution to me. It would still be nice to emit a warning, as well, if that's reasonably easy to do. -- Josh Berkus Aglio Database Solutions San Francisco