Thread: Some more function-default issues

Some further reflection about Rushabh Lathia's bug report of yesterday
led me to the realization that there's a pretty big hole in the function
defaults patch.  Since we add default values during planning, it doesn't
work for any expression that's not fed through the planner.  For
instance, ALTER COLUMN USING:

regression=# create function add(int, int = 42) returns int
regression-# as 'select $1+$2' language sql;
CREATE FUNCTION
regression=# create table foo(f1 int);
CREATE TABLE
regression=# insert into foo values (1);
INSERT 0 1
regression=# alter table foo alter column f1 type bigint using add(f1)::bigint;
ERROR:  no value found for parameter 2
CONTEXT:  SQL function "add" statement 1

The minimum-code-change solution would be to run around and try to make
sure every such expression gets passed through eval_const_expressions()
before we try to execute it.  This is probably doable (looking for calls
to fix_opfuncids would be a good guide) but it seems like the potential
for errors of omission is large, particularly in third-party add-ons.

I wonder if anyone has an idea for a better way to attack this?
        regards, tom lane

Tom Lane wrote:
> The minimum-code-change solution would be to run around and try to make
> sure every such expression gets passed through eval_const_expressions()
> before we try to execute it.  This is probably doable (looking for calls
> to fix_opfuncids would be a good guide) but it seems like the potential
> for errors of omission is large, particularly in third-party add-ons.

That seems ok to me. Calling eval_const_expressions() in ALTER COLUMN 
and elsewhere is a good idea for performance reasons as well.

I can only find one more call to fix_opfuncids, where we're not already 
calling eval_const_expressions(): GetDomainConstraints(). Adding a 
eval_const_expressions() call to ExecPrepareExpr() would take care of 
the ALTER COLUMN and many other cases where we have a problem now.

I can't imagine a third-party add-on so tightly integrated with the 
backend that it needs to mess with Expr nodes, and call fix_opfuncids().  ExecPrepareExpr, maybe, but if we fix that as
Ipresume we would, the 
 
add-ons wouldn't be affected.

Overall, I don't see much potential for bugs-of-omission. You could put 
a comment at the top of fix_opfuncids() as a reminder that outside the 
executor you need to call eval_const_expressions() too.

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> That seems ok to me. Calling eval_const_expressions() in ALTER COLUMN 
> and elsewhere is a good idea for performance reasons as well.

Yeah, probably so.

> I can only find one more call to fix_opfuncids, where we're not already 
> calling eval_const_expressions(): GetDomainConstraints(). Adding a 
> eval_const_expressions() call to ExecPrepareExpr() would take care of 
> the ALTER COLUMN and many other cases where we have a problem now.

I'd prefer not to have ExecPrepareExpr do it, though; that's supposed
to be working from a read-only expression tree supplied by the caller.
(The fix_opfuncids call in it is already pushing the bounds of that
concept.)

From a structural point of view the right thing would be to introduce
a concept of "expression planning", along the lines of
expr = plan_expression(expr);

which callers would be required to invoke before ExecPrepareExpr.
Right now this would do the fix_opfuncids bit and
eval_const_expressions, but I could see someday allowing SubLinks
in standalone expressions because we'd have the ability to invoke
the full planner from inside here.

The trick is to get the attention of third-party code about the need
to make this change.  Removing fix_opfuncids from ExecPrepareExpr
wouldn't really help much, because in very many common cases it's
a no-op anyway; so unless their testing is quite thorough they would
not see a failure before shipping.

The only idea I have at the moment is to rename ExecPrepareExpr to
something else, but it's not clear if that will persuade people to read
its header comment or not ...
        regards, tom lane

I wrote:
> I'd prefer not to have ExecPrepareExpr do it, though; that's supposed
> to be working from a read-only expression tree supplied by the caller.
> (The fix_opfuncids call in it is already pushing the bounds of that
> concept.)

> From a structural point of view the right thing would be to introduce
> a concept of "expression planning", along the lines of

>     expr = plan_expression(expr);

> which callers would be required to invoke before ExecPrepareExpr.
> Right now this would do the fix_opfuncids bit and
> eval_const_expressions, but I could see someday allowing SubLinks
> in standalone expressions because we'd have the ability to invoke
> the full planner from inside here.

I looked around and found that the main issue with doing it the way
I was envisioning is the ii_Expressions and ii_Predicate expression
trees in struct IndexInfo.  I wanted to run those trees through
plan_expression() before sticking them into IndexInfo, but the trouble
with that approach is that in UpdateIndexRelation() we expect that we
can use nodeToString() on them to produce the index definitional data
that will get stored into pg_index.  We do *not* want to use "planned"
expressions for that --- it would cause function defaults to propagate
into the index definition, which is exactly what we are trying to avoid
by not inserting defaults until plan time.

So it seems like the best solution is to allow ExecPrepareExpr to
call plan_expression().  A look through the callers suggests that
this won't really result in much repeated work because there are few
cases where the results could be re-used anyway.  It's still a tad
annoying, but not enough so to justify a lot of code rearrangement
right now.
        regards, tom lane