Re: SQL/MED - file_fdw

Here is a revised patch, that including jagged csv support.
A new exported function is named as NextCopyFromRawFields.

On Mon, Feb 7, 2011 at 21:16, Noah Misch <noah(at)leadboat(dot)com> wrote:
> Perhaps I'm missing something.  The new API does not expose a "processed" count
> at all; that count is used for the command tag of a top-level COPY.  This part
> of the patch is just changing how we structure the code to maintain that tally,
> and it has no implications for observers outside copy.c.  Right?

True, but the counter is tightly bound with INSERT-side of COPY FROM.
| copy.c (1978)
| * We count only tuples not suppressed by a BEFORE INSERT trigger;

I think it is cleaner way to separate it from CopyState
because it is used as a file reader rather than a writer.
However, if there are objections, I'd revert it.

>> I changed "COPY %s, .." to "relation %s, ..."
> My comment originated with a faulty idea that file_fdw's internal use of COPY
> was an implementation detail that users should not need to see.  Looking now,
> the file_fdw documentation clearly explains the tie to COPY, even referring
> users to the COPY documentation.  I no longer see a need to hide the fact that
> the "foreign" source is a PostgreSQL COPY command.  The error messages are fine
> as they were.

OK, I reverted the changes. User-visible changes might be more important,
pointed by Andrew.

> ExecEvalExpr(), used to acquire default values, will still use the
> per-tuple context of CopyState->estate.  That per-tuple context will never get
> reset explicitly, so default value computations leak until EndCopyFrom().

Ah, I see. I didn't see the leak because we rarely use per-tuple memory
context in the estate. We just use CurrentMemoryContext in many cases.
But the leak could occur, and the code is misleading.
I moved ResetPerTupleExprContext() into NextCopyFrom(), but
CurrentMemoryContext still used in it to the result values.

Another possible design might be passing EState as an argument of
NextCopyFrom and remove estate from CopyState. It seems a much cleaner way
in terms of control flow, but it requires more changes in file_fdw.
Comments?

> This block comment remains roughly half correct.  Again, I think a small comment
> on every field below should replace it.
>
> This is just a verbatim copy of the CopyGetAttnums() header comment.  (The
> middle paragraph happens to be true, though.)

Silly mistakes. Maybe came from too many 'undo' in my editor.

--
Itagaki Takahiro