Re: SQL/MED - file_fdw

On Wed, Jan 19, 2011 at 00:34, Shigeru HANADA <hanada(at)metrosystems(dot)co(dot)jp> wrote:
> Attached patch requires FDW API patches and copy_export-20110114.patch.

Some minor comments:

* Can you pass slot->tts_values and tts_isnull directly to NextCopyFrom()?
It won't allocate the arrays; just fill the array buffers.

* You can pass NULL for the 4th argument for NextCopyFrom().
| Oid tupleoid; /* just for required parameter */

* file_fdw_validator still has duplicated codes with BeginCopy,
but I have no idea to share the validation code in clean way...

* Try strVal() instead of DefElem->val.str
* FdwEPrivate seems too abbreviated for me. How about FileFdwPrivate?
* "private" is a bad identifier name because it's a C++ keyword.
We should rename FdwExecutionState->private.

> In that message, you also pointed out that FDW must generate
> explainInfo in every PlanRelScan call even if the planning is not for
> EXPLAIN.  I'll try to defer generating explainInfo until EXPLAIN
> VERBOSE really uses it.  It might need new hook point in expalain.c,
> though.

I complained about the overhead, but it won't be a problem for
file_fdw and pgsql_fdw. file_fdw can easily generate the text,
and pgsql_fdw needs to generate a SQL query anyway.

My concern is the explainInfo interface is not ideal for the purpose
and therefore it will be unstable interface. If we support nested plans
in FDWs, each FDW should receive a tree writer used internally in
explain.c. explainInfo, that is a plan text, is not enough for complex
FdwPlans. However, since we don't have any better solution for now,
we could have the variable for 9.1. It's much better than nothing.

--
Itagaki Takahiro