Re: SQL/MED - file_fdw

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.

Or, if you eventually make a HeapTuple from values and nulls arrays,
you could modify NextCopyFrom() to return a HeapTuple instead of values,
nulls, and tupleOid. The reason I didn't use HeapTuple is that I've
seen arrays were used in the proposed FDW APIs. But we don't have to
use such arrays if you use HeapTuple based APIs.

IMHO, I prefer HeapTuple because we can simplify NextCopyFrom and
keep EState private in copy.c.

#2. Can you avoid making EXPLAIN text in fplan->explainInfo on
non-EXPLAIN cases? It's a waste of CPU cycles in normal executions.
I doubt whether FdwPlan.explainInfo field is the best design.
How do we use the EXPLAIN text for XML or JSON explain formats?
Instead, we could have an additional routine for EXPLAIN.

#3. Why do you re-open a foreign table in estimate_costs() ?
Since the caller seems to have the options for them, you can
pass them directly, no?

In addition, passing a half-initialized fplan to estimate_costs()
is a bad idea. If you think it is an OUT parameter, the OUT params
should be *startup_cost and *total_cost.

#4. It'a minor cosmetic point, but our coding conventions would be
we don't need (void *) when we cast a pointer to void *, but need
(Type *) when we cast a void pointer to another type.

--
Itagaki Takahiro