On Wed, Feb 09, 2011 at 02:55:26PM +0900, Itagaki Takahiro wrote:
> On Wed, Feb 9, 2011 at 03:49, Noah Misch <n...@leadboat.com> wrote:
> > The code still violates the contract of ExecEvalExpr() by calling it with
> > CurrentMemoryContext != econtext->ecxt_per_tuple_memory.
>
> I'm not sure whether we have such contract because the caller might
> want to get the expression result in long-lived context.
execQual.c has this comment:
/* ----------------------------------------------------------------
* ExecEvalExpr routines
...
* The caller should already have switched into the temporary memory
* context econtext->ecxt_per_tuple_memory. The convenience entry point
* ExecEvalExprSwitchContext() is provided for callers who don't prefer to
* do the switch in an outer loop. We do not do the switch in these
routines
* because it'd be a waste of cycles during nested expression evaluation.
* ----------------------------------------------------------------
*/
Assuming that comment is accurate, ExecEvalExpr constrains us; any default
values must get allocated in econtext->ecxt_per_tuple_memory. To get them in a
long-lived allocation, the caller can copy the datums or supply a long-lived
ExprContext. Any CurrentMemoryContext works when econtext == NULL, of course.
I'd suggest enhancing your new paragraph in the NextCopyFrom() header comment
like this:
- * 'econtext' is used to evaluate default expression for each columns not
- * read from the file. It can be NULL when no default values are used, i.e.
- * when all columns are read from the file.
+ * 'econtext' is used to evaluate default expression for each columns not read
+ * from the file. It can be NULL when no default values are used, i.e. when
all
+ * columns are read from the file. If econtext is not NULL, the caller must
have
+ * switched into the temporary memory context econtext->ecxt_per_tuple_memory.
> But anyway
> using an external ExprContext looks cleaner. The new prototype is:
>
> bool NextCopyFrom(
> [IN] CopyState cstate, ExprContext *econtext,
> [OUT] Datum *values, bool *nulls, Oid *tupleOid)
Looks good.
> Note that econtext can be NULL if no default values are used.
> Since file_fdw won't use default values, we can just pass NULL for it.
Nice. Good thinking. One small point:
> --- 2475,2504 ----
> * provided by the input data. Anything not processed here or
> above
> * will remain NULL.
> */
> + /* XXX: End of only-indentation changes. */
> + if (num_defaults > 0)
> + {
> + Assert(econtext != NULL);
> +
> for (i = 0; i < num_defaults; i++)
This could be better-written as "Assert(num_defaults == 0 || econtext !=
NULL);".
>From a functional and code structure perspective, I find this ready to commit.
(I assume you'll drop the XXX: indent only comments on commit.) Kevin, did you
want to do that performance testing you spoke of?
Thanks,
nm
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
