Re: Segfault in 9.0 inlining SRF

Brendan Jurd <direvus(at)gmail(dot)com> writes:
> On 25 October 2010 07:36, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I'm guessing it was modified in the temporary memory context and not
>> properly copied out to the parent context when we finished inlining
>> the function.

> Thanks for the hint; I found that the attached patch resolved my
> specific segfault, but I am wondering whether it goes far enough. The
> patch just copies invalItems up out of the temporary context before it
> is deleted. Could there also be changes to other elements of
> PlannerGlobal that need to be saved? Should we in fact be copying out
> the whole of PlannerGlobal each time, and would that necessitate a new
> copyfunc for it?

Well, it definitely doesn't go far enough, because the invalItems list
has to be restored to its original state if we fail to inline at some
point after calling eval_const_expressions. I'm currently testing the
attached patch.

I thought about whether we need something more general, but for the
moment I think this is sufficient; eval_const_expressions has only
very limited reason to examine the PlannerInfo data at all, and less
reason to modify it. Copying the whole structure would be overkill.
Moreover, it wouldn't do anything to improve modularity anyhow: this
function would still have to know which parts of the structure to copy
back to the top level, and which not. So it'd still need to know
quite a bit about exactly what eval_const_expressions is doing.

regards, tom lane

diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 13e89ec6678f01e7baf39f722aa960bf4550261b..00156d8eeeadfc18ad7680d283dc14a4db641f62 100644
*** a/src/backend/optimizer/util/clauses.c
--- b/src/backend/optimizer/util/clauses.c
*************** rowtype_field_matches(Oid rowtypeid, int
*** 2018,2028 ****
* will not be pre-evaluated here, although we will reduce their
* arguments as far as possible.
*
* We assume that the tree has already been type-checked and contains
* only operators and functions that are reasonable to try to execute.
*
* NOTE: "root" can be passed as NULL if the caller never wants to do any
! * Param substitutions.
*
* NOTE: the planner assumes that this will always flatten nested AND and
* OR clauses into N-argument form. See comments in prepqual.c.
--- 2018,2033 ----
* will not be pre-evaluated here, although we will reduce their
* arguments as far as possible.
*
+ * Whenever a function is eliminated from the expression by means of
+ * constant-expression evaluation or inlining, we add the function's
+ * OID to root->glob->invalItems. This ensures the plan is known to
+ * depend on such functions, even though they aren't referenced anymore.
+ *
* We assume that the tree has already been type-checked and contains
* only operators and functions that are reasonable to try to execute.
*
* NOTE: "root" can be passed as NULL if the caller never wants to do any
! * Param substitutions nor receive info about inlined functions.
*
* NOTE: the planner assumes that this will always flatten nested AND and
* OR clauses into N-argument form. See comments in prepqual.c.
*************** inline_set_returning_function(PlannerInf
*** 4095,4100 ****
--- 4100,4106 ----
bool modifyTargetList;
MemoryContext oldcxt;
MemoryContext mycxt;
+ List *saveInvalItems;
inline_error_callback_arg callback_arg;
ErrorContextCallback sqlerrcontext;
List *raw_parsetree_list;
*************** inline_set_returning_function(PlannerInf
*** 4181,4186 ****
--- 4187,4202 ----
ALLOCSET_DEFAULT_MAXSIZE);
oldcxt = MemoryContextSwitchTo(mycxt);

+ /*
+ * When we call eval_const_expressions below, it might try to add items
+ * to root->glob->invalItems. Since it is running in the temp context,
+ * those items will be in that context, and will need to be copied out
+ * if we're successful. Temporarily reset the list so that we can keep
+ * those items separate from the pre-existing list contents.
+ */
+ saveInvalItems = root->glob->invalItems;
+ root->glob->invalItems = NIL;
+
/* Fetch the function body */
tmp = SysCacheGetAttr(PROCOID,
func_tuple,
*************** inline_set_returning_function(PlannerInf
*** 4307,4312 ****
--- 4323,4331 ----

querytree = copyObject(querytree);

+ root->glob->invalItems = list_concat(saveInvalItems,
+ copyObject(root->glob->invalItems));
+
MemoryContextDelete(mycxt);
error_context_stack = sqlerrcontext.previous;
ReleaseSysCache(func_tuple);
*************** inline_set_returning_function(PlannerInf
*** 4322,4327 ****
--- 4341,4347 ----
/* Here if func is not inlinable: release temp memory and return NULL */
fail:
MemoryContextSwitchTo(oldcxt);
+ root->glob->invalItems = saveInvalItems;
MemoryContextDelete(mycxt);
error_context_stack = sqlerrcontext.previous;
ReleaseSysCache(func_tuple);