On Wed, Feb 9, 2011 at 03:49, Noah Misch <noah@leadboat.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