Обсуждение: Another issue in default-values patch: defaults expanded too soon
Consider create function foo(f1 int, f2 int = 42, f2 int = 43) ... create view v1 as select foo(11); In CVS HEAD this gives regression=# \d v1 View "public.v1"Column | Type | Modifiers --------+---------+-----------foo | integer | View definition:SELECT foo(11, 42, 43) AS foo; which is an accurate representation of the truth: if you change the defaults for function foo, v1 will keep on calling it with the old default values. Does anyone think this is either unsurprising or desirable? I'm not sure we can do much to fix it, though. It'd probably be possible to have the rewriter or planner insert the default expressions, instead of the parser; but that creates its own issues. Suppose I had v1 defined as above and then did create or replace function foo(f1 int, f2 int, f2 int = 43) ... ie, remove one or more default expressions. *This function definition no longer matches the original call*. If we did plan-time insertion of defaults we'd have little choice but to fail when v1 is executed, because there'd be no principled way to insert a default for f2. Treating the defaults as being inserted at parse time at least ensures that v1's call to foo still works. This at least needs documentation, I think. Comments? regards, tom lane
On Dec 16, 2008, at 9:25 PM, Tom Lane wrote: > Consider > > create function foo(f1 int, f2 int = 42, f2 int = 43) ... > create view v1 as select foo(11); > > In CVS HEAD this gives > > regression=# \d v1 > View "public.v1" > Column | Type | Modifiers > --------+---------+----------- > foo | integer | > View definition: > SELECT foo(11, 42, 43) AS foo; > > which is an accurate representation of the truth: if you change the > defaults for function foo, v1 will keep on calling it with the old > default values. Ooh. Ow. > Does anyone think this is either unsurprising or desirable? Not I! > I'm not sure we can do much to fix it, though. It'd probably be > possible to have the rewriter or planner insert the default > expressions, > instead of the parser; but that creates its own issues. Would the same thing happen for a prepared statement that calls the function? Or another function? > Suppose I had v1 defined as above and then did > > create or replace function foo(f1 int, f2 int, f2 int = 43) ... > > ie, remove one or more default expressions. *This function definition > no longer matches the original call*. If we did plan-time insertion > of > defaults we'd have little choice but to fail when v1 is executed, > because there'd be no principled way to insert a default for f2. That seems like it'd be the reasonable thing to do. > Treating the defaults as being inserted at parse time at least ensures > that v1's call to foo still works. That leads to mysterious action-at-a-distance bugs, though. Too weird. > This at least needs documentation, I think. > > Comments? Documentation at least, yes, but it'd be better, I think, if the planner inserted the defaults. Best, David
"David E. Wheeler" <david@kineticode.com> writes: > On Dec 16, 2008, at 9:25 PM, Tom Lane wrote: >> I'm not sure we can do much to fix it, though. It'd probably be >> possible to have the rewriter or planner insert the default >> expressions, instead of the parser; but that creates its own issues. > Would the same thing happen for a prepared statement that calls the > function? Or another function? Prepared statements and functions don't have such a problem, because they don't have a long-lived parsetree representation. If you change the defaults attached to an existing function, that will result in an invalidation on all plans referencing the function (thanks to some changes that went in a commitfest or two back), and things should pretty much "just work". A view or rule is a bigger problem because it will have a hard binding to a particular function OID. >> Suppose I had v1 defined as above and then did >> >> create or replace function foo(f1 int, f2 int, f2 int = 43) ... >> >> ie, remove one or more default expressions. *This function definition >> no longer matches the original call*. If we did plan-time insertion of >> defaults we'd have little choice but to fail when v1 is executed, >> because there'd be no principled way to insert a default for f2. > That seems like it'd be the reasonable thing to do. I'm not too thrilled about it. One thing to consider is that with the default gone, it might be that there is some other function that matches the textual call better than this one. But we can't really change the view to reference that other function. So it's going to work differently than the replan-from-source case, no matter what. In some ways this is analogous to the problem of "when do you expand * in a SELECT list?". The SQL spec is clear that it's expanded at CREATE VIEW time; but there are certainly lots of cases where people wish it didn't work like that. regards, tom lane
On Dec 16, 2008, at 10:15 PM, Tom Lane wrote: >> Would the same thing happen for a prepared statement that calls the >> function? Or another function? > > Prepared statements and functions don't have such a problem, because > they don't have a long-lived parsetree representation. If you change > the defaults attached to an existing function, that will result in an > invalidation on all plans referencing the function (thanks to some > changes that went in a commitfest or two back), and things should > pretty much "just work". A view or rule is a bigger problem because > it will have a hard binding to a particular function OID. Well, that's good, except that the behavior then becomes inconsistent in rules and views vs. everything else. I understand the underlying reasons for this (thanks to your reply here), but those not familiar with the architecture will find it mystifying. Documentation, at the very least, is certainly in order. >> That seems like it'd be the reasonable thing to do. > > I'm not too thrilled about it. One thing to consider is that with the > default gone, it might be that there is some other function that > matches > the textual call better than this one. But we can't really change the > view to reference that other function. So it's going to work > differently than the replan-from-source case, no matter what. Bleh. > In some ways this is analogous to the problem of "when do you expand * > in a SELECT list?". The SQL spec is clear that it's expanded at > CREATE > VIEW time; but there are certainly lots of cases where people wish it > didn't work like that. Yeah, function default values aren't specified by the spec, are they? Best, David
"David E. Wheeler" <david@kineticode.com> writes: > On Dec 16, 2008, at 10:15 PM, Tom Lane wrote: >> I'm not too thrilled about it. One thing to consider is that with the >> default gone, it might be that there is some other function that >> matches >> the textual call better than this one. But we can't really change the >> view to reference that other function. So it's going to work >> differently than the replan-from-source case, no matter what. > Bleh. I haven't really thought through the consequences of this, but one thing we could consider doing to tamp down the damage is to prohibit changing the number of defaultable parameters of an existing function, ie treat pronargdefaults as not allowed to change during CREATE OR REPLACE FUNCTION. The point here would be to ensure that function replacement couldn't change the parser's decisions about whether a function matches a call or not; which is the case in existing releases, but has been falsified by this patch. If that's acceptable, then we could insert default expressions at plan time with confidence that no defaults we need have disappeared under us. (It might be enough to not allow pronargdefaults to decrease. Not sure about that though. It's certainly possible that adding a default could make a call ambiguous when it was not before.) There's another slightly annoying issue here, which is that in at least some cases, inserting defaults at plan time would require an additional traversal of the parsetree. This isn't a huge deal probably, but it would result in some performance loss in long-but-simple queries. regards, tom lane
On Dec 16, 2008, at 10:36 PM, Tom Lane wrote: > I haven't really thought through the consequences of this, but one > thing > we could consider doing to tamp down the damage is to prohibit > changing > the number of defaultable parameters of an existing function, ie treat > pronargdefaults as not allowed to change during CREATE OR REPLACE > FUNCTION. The point here would be to ensure that function replacement > couldn't change the parser's decisions about whether a function > matches > a call or not; which is the case in existing releases, but has been > falsified by this patch. > > If that's acceptable, then we could insert default expressions at plan > time with confidence that no defaults we need have disappeared under > us. Wouldn't you still have the problem if you still allow the default values to be changed? And I would hope that they could be changed! > There's another slightly annoying issue here, which is that in at > least > some cases, inserting defaults at plan time would require an > additional > traversal of the parsetree. This isn't a huge deal probably, but it > would result in some performance loss in long-but-simple queries. Yes, and it avoids the principal of least surprise. Best, David
"David E. Wheeler" <david@kineticode.com> writes: > On Dec 16, 2008, at 10:36 PM, Tom Lane wrote: >> ... The point here would be to ensure that function replacement >> couldn't change the parser's decisions about whether a function matches >> a call or not; which is the case in existing releases, but has been >> falsified by this patch. >> >> If that's acceptable, then we could insert default expressions at plan >> time with confidence that no defaults we need have disappeared under >> us. > Wouldn't you still have the problem if you still allow the default > values to be changed? And I would hope that they could be changed! No, you could change the *values* of the default expressions. What you'd not be allowed to do is remove a default entirely. (Or, perhaps, add one; I'm less sure about that.) The point here is that adding or removing a default changes the set of calls a function could possibly match, just as changing the list of parameter types changes what it can match. We don't allow the latter and I'm thinking we shouldn't allow the former either. regards, tom lane
And while we're on the subject ... as the patch stands, it lets you provide defaults for polymorphic parameters, as in regression=# create function eq (f1 anyelement, f2 anyelement default 42) returns bool as 'select $1 = $2' language sql; CREATE FUNCTION regression=# select eq(now(),now());eq ----t (1 row) regression=# select eq(now()); ERROR: arguments declared "anyelement" are not all alike DETAIL: timestamp with time zone versus integer regression=# select eq(11,12);eq ----f (1 row) regression=# select eq(11); eq ----f (1 row) regression=# select eq(42);eq ----t (1 row) The reason this is pretty wacko is that changing the default can change the set of calls the function can match: regression=# create or replace function eq (f1 anyelement, f2 anyelement default now()) returns bool as 'select $1 = $2'language sql; CREATE FUNCTION regression=# select eq(now()); eq ----t (1 row) regression=# select eq(42); ERROR: arguments declared "anyelement" are not all alike DETAIL: integer versus timestamp with time zone In fact, it's worse than that: changing the default can change the resolved output type of the function. regression=# create function dupl (f1 anyelement default 42) returns anyelement as 'select $1' language sql; CREATE FUNCTION regression=# select dupl();dupl ------ 42 (1 row) regression=# create or replace function dupl (f1 anyelement default now()) returns anyelement as 'select $1' language sql; CREATE FUNCTION regression=# select dupl(); dupl -------------------------------2008-12-16 17:39:41.418412-05 (1 row) Isn't *that* special. Needless to say, this would utterly break any view or rule referencing the function. I'm inclined to think we should forbid defaults for polymorphic parameters, and wondered if anyone can come up with an actually useful use-case that'd require it. If we were going to allow it, we'd at least have to restrict things enough so that the resolved output type couldn't change. (The reason I stumbled across this was that the current behavior is an artifact of inserting the default expressions at parse time; in fact, before resolving polymorphic types. It would get very much more painful to support any sort of behavior along this line if we don't do that.) regards, tom lane
Tom Lane wrote: > Consider > > create function foo(f1 int, f2 int = 42, f2 int = 43) ... > create view v1 as select foo(11); > > In CVS HEAD this gives > > regression=# \d v1 > View "public.v1" > Column | Type | Modifiers > --------+---------+----------- > foo | integer | > View definition: > SELECT foo(11, 42, 43) AS foo; > > which is an accurate representation of the truth: if you change the > defaults for function foo, v1 will keep on calling it with the old > default values. > > Does anyone think this is either unsurprising or desirable? Huh? Shouldn't changing a function which a view depends on require a rebuild of the view? That is, I think we should treat changing the defaults the same as we would changing the number and type of parameters; it kicks off a dependency check and requires a CASCADE. --Josh
Josh Berkus <josh@agliodbs.com> writes: > Huh? Shouldn't changing a function which a view depends on require a > rebuild of the view? There is no such concept as "a rebuild of the view". > That is, I think we should treat changing the defaults the same as we > would changing the number and type of parameters; it kicks off a > dependency check and requires a CASCADE. Dream on ... there is no such facility in Postgres and we are not going to build one in the 8.4 timeframe. regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes: > Josh Berkus <josh@agliodbs.com> writes: > >> That is, I think we should treat changing the defaults the same as we >> would changing the number and type of parameters; it kicks off a >> dependency check and requires a CASCADE. > > Dream on ... there is no such facility in Postgres and we are not going > to build one in the 8.4 timeframe. Well there is this: postgres=# create or replace function foo (text) returns text as 'select 1' language sql; ERROR: 42P13: cannot change return type of existing function HINT: Use DROP FUNCTION first. LOCATION: ProcedureCreate, pg_proc.c:366 We could say that changing the type of a default argument for a polymorphic argument isn't allowed just like changing the return value. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's PostGIS support!
Gregory Stark <stark@enterprisedb.com> writes: > We could say that changing the type of a default argument for a polymorphic > argument isn't allowed just like changing the return value. The point I was trying to make is that allowing defaults for polymorphic args at all is going to cause a very significant amount of work and complication, of which enforcing the above check is just a small part. I wanted to see a plausible use-case for it before expending that work. regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes: > Gregory Stark <stark@enterprisedb.com> writes: >> We could say that changing the type of a default argument for a polymorphic >> argument isn't allowed just like changing the return value. > > The point I was trying to make is that allowing defaults for polymorphic > args at all is going to cause a very significant amount of work and > complication, of which enforcing the above check is just a small part. > I wanted to see a plausible use-case for it before expending that work. Well honestly I don't see a terribly compelling use case for default arguments altogether. Obviously they're just a programmer convenience and don't really let anyone do anything they couldn't do without them. So it's not like any use case for default polymorphic arguments is going to be especially compelling either. But I don't see any reason it'll be any less useful for polymorphic arguments than any other type. The fundamental problem with polymorphic parameters and default arguments is that the type of the argument determines the type of the return value. And the type of the return value can't change within an existing parse tree -- so it seems to me that barring changing the type of a default argument for a polymorphic parameter is precisely targeted to the source and should cover all problems it causes. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's 24x7 Postgres support!
Greg, > Well honestly I don't see a terribly compelling use case for default arguments > altogether. Obviously they're just a programmer convenience and don't really > let anyone do anything they couldn't do without them. The primary use case is for people who are porting applications from other DBMSes. Which don't support polymorphic arguments in the first place. --Josh
Gregory Stark <stark@enterprisedb.com> writes: > So it's not like any use case for default polymorphic arguments is going to be > especially compelling either. But I don't see any reason it'll be any less > useful for polymorphic arguments than any other type. Well, the reason it seems funny to me is that the point of a polymorphic argument is to be datatype-agnostic, and as soon as you put in a default value of a particular type you're letting go of that principle. The only case I can imagine being actually useful is where the default is an untyped null, and you have a non-defaultable anyelement argument that drives determination of what the defaulted one is, ie something like create function array2 (f1 anyelement, f2 anyelement = null)returns anyarrayas 'select array[$1,$2]' language sql; Although this seems well-defined in principle, I'm not sure if it actually works with the current patch; and it'd be even trickier if we try to put in some code that says "the default's type can't change". A null constant has the same type as an unmarked string literal (namely UNKNOWN) but IIRC they don't really behave the same for type resolution purposes. regards, tom lane
On Tue, Dec 16, 2008 at 3:25 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Does anyone think this is either unsurprising or desirable? That's horrible. I wonder whether the whole architecture is wrong here. Perhaps when a function is created with N arguments of which M have default values, we should actually create M entries in pg_proc: one for each possible number of arguments from N-M up through N. The one with N arguments would be the main entry, and the rest would be dependent entries that would get dropped if the main entry were dropped (similar to the way sequences for serial columns depend on the parent table). If one of the dependent entries conflicted with an existing entry, then CREATE FUNCTION would simply fail. I think this would kill all of the problems reported thus far at one blow. There wouldn't be any need for code to resolve ambiguous function calls because there wouldn't be any ambiguous function calls, or at least no more than what already exists in 8.3. The problem you report here would go away because the view definition would match one of the dummy entries. Removing a necessary default value would remove that dummy entry, which would be caught by the existing dependency stuff. Changing a default would amount to changing the definition of a dummy entry as if by CREATE OR REPLACE FUNCTION, which would work just as expected. ...Robert
"Robert Haas" <robertmhaas@gmail.com> writes: > I wonder whether the whole architecture is wrong here. Perhaps when a > function is created with N arguments of which M have default values, > we should actually create M entries in pg_proc: one for each possible > number of arguments from N-M up through N. That's been considered and rejected before, in the context of the variadic-function patch which has a lot of the same issues. What it mostly does is bloat pg_proc. > I think this would kill all of the problems reported thus far at one > blow. No, it doesn't resolve any of them ... particularly not the ones associated with defaults for polymorphics. regards, tom lane
On Tue, Dec 16, 2008 at 9:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > "Robert Haas" <robertmhaas@gmail.com> writes: >> I wonder whether the whole architecture is wrong here. Perhaps when a >> function is created with N arguments of which M have default values, >> we should actually create M entries in pg_proc: one for each possible >> number of arguments from N-M up through N. > > That's been considered and rejected before, in the context of the > variadic-function patch which has a lot of the same issues. What it > mostly does is bloat pg_proc. Only if you have a large number of functions with a large number of optional arguments each. That's possible, I suppose, but it hardly seems likely, or worth worrying about. >> I think this would kill all of the problems reported thus far at one >> blow. > > No, it doesn't resolve any of them ... particularly not the ones > associated with defaults for polymorphics. I think that's hyperbole. You would probably still need to forbid non-polymorphic defaults for polymorphic parameters (you might be able to make NULL work, and maybe the empty array for anyarray... not sure), but I think that most of the other issues you raised would be addressed by my proposal. You may hate it anyway; I'm OK with that. :-) ...Robert
2008/12/17 Robert Haas <robertmhaas@gmail.com>: > On Tue, Dec 16, 2008 at 9:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> "Robert Haas" <robertmhaas@gmail.com> writes: >>> I wonder whether the whole architecture is wrong here. Perhaps when a >>> function is created with N arguments of which M have default values, >>> we should actually create M entries in pg_proc: one for each possible >>> number of arguments from N-M up through N. >> >> That's been considered and rejected before, in the context of the >> variadic-function patch which has a lot of the same issues. What it >> mostly does is bloat pg_proc. > > Only if you have a large number of functions with a large number of > optional arguments each. That's possible, I suppose, but it hardly > seems likely, or worth worrying about. > >>> I think this would kill all of the problems reported thus far at one >>> blow. >> >> No, it doesn't resolve any of them ... particularly not the ones >> associated with defaults for polymorphics. > > I think that's hyperbole. You would probably still need to forbid > non-polymorphic defaults for polymorphic parameters (you might be able > to make NULL work, and maybe the empty array for anyarray... not > sure), but I think that most of the other issues you raised would be > addressed by my proposal. You may hate it anyway; I'm OK with that. > :-) it's not good solution, problem is anywhere else - we raise exception about ambigonous call to early, when we don't have all knowleadges. Pavel > > ...Robert > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
Gregory Stark wrote: > Well honestly I don't see a terribly compelling use case for default arguments > altogether. Obviously they're just a programmer convenience and don't really > let anyone do anything they couldn't do without them. The real payoff comes with name-based paramter lists (the name => value busines) and allowing defaults anywhere in the parameter list (not just at the end). This is required to port many PL/SQL-using applications, and you can't write it any other way.