Re: SQL/MED - file_fdw

On Wed, Feb 09, 2011 at 02:55:26PM +0900, Itagaki Takahiro wrote:
> On Wed, Feb 9, 2011 at 03:49, Noah Misch <noah@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