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