Re: SQL/MED - file_fdw

On Fri, 24 Dec 2010 11:09:16 +0900
Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> wrote:
> On Tue, Dec 21, 2010 at 21:32, Itagaki Takahiro
> <itagaki(dot)takahiro(at)gmail(dot)com> wrote:
> > On Tue, Dec 21, 2010 at 20:14, Shigeru HANADA <hanada(at)metrosystems(dot)co(dot)jp> wrote:
> >> Attached is the revised version of file_fdw patch.  This patch is
> >> based on Itagaki-san's copy_export-20101220.diff patch.
> >
> > #1. Don't you have per-tuple memory leak? I added GetCopyExecutorState()
> > because the caller needs to reset the per-tuple context periodically.
>
> Sorry, I found there are no memory leak here. The related comment is:
> [execnodes.h]
> * CurrentMemoryContext should be set to ecxt_per_tuple_memory before
> * calling ExecEvalExpr() --- see ExecEvalExprSwitchContext().
> I guess CurrentMemoryContext in Iterate callback a per-tuple context.
> So, we don't have to xport cstate->estate via GetCopyExecutorState().

Iterate is called in query context, so GetCopyExecutorState() need to
be exported to avoid memory leak happens in NextCopyFrom(). Or,
enclosing context switching into NextCopyFrom() is better? Then,
CopyFrom() would need to create tuples in Portal context and set
shouldFree of ExecStoreTuple() true to free stored tuple at next call.

Please try attached patch.

> > Or, if you eventually make a HeapTuple from values and nulls arrays,
>
> ExecStoreVirtualTuple() seems to be better than the combination of
> heap_form_tuple() and ExecStoreTuple() for the purpose. Could you try
> to use slot->tts_values and slot->tts_isnull for NextCopyFrom() directly?

Virtual tuple would be enough to carry column data, but virtual tuple
doesn't have system attributes including tableoid...

Regards,
--
Shigeru Hanada