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(at)leadboat(dot)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