Re: SQL/MED - file_fdw

On Wed, Feb 9, 2011 at 03:49, Noah Misch <noah(at)leadboat(dot)com> wrote:
>> A new exported function is named as NextCopyFromRawFields.
> Seems a bit incongruous to handle the OID column in that function.  That part
> fits with the other per-column code in NextCopyFrom().

Hmmm, I thought oid columns should be separated from user columns, but it
might be a confusing interface. I removed the explicit oid output from
NextCopyFromRawFields. file_fdw won't use oids at all in any cases, though.

> 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. 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)

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.

> Sorry, I missed a lot of these memory details on my first couple of reviews.

You did great reviews! Thank you very much.

--
Itagaki Takahiro