Re: SQL/MED - file_fdw - Mailing list pgsql-hackers

On Thu, 20 Jan 2011 22:21:37 +0900
Itagaki Takahiro <itagaki.takahiro@gmail.com> wrote:
> On Wed, Jan 19, 2011 at 00:34, Shigeru HANADA <hanada@metrosystems.co.jp> wrote:
> > Attached patch requires FDW API patches and copy_export-20110114.patch.
> 
> Some minor comments:

Thanks for the comments.
I'll post revised version of patches in new threads.

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

I think it's safe to pass them to NextCopyFrom() directly because that
arrays are allocated in ExecSetSlotDescriptor() during
ExecInitForeignScan(), and size of arrays are taken from 
Please let me know if I've missed your point.

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

I didn't know that it's NULL-safe, thanks.

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

It would be necessary to change considerable part of BeginCopy() to
separate validation from it to use validation from file_fdw...

> * Try strVal() instead of DefElem->val.str
> * FdwEPrivate seems too abbreviated for me. How about FileFdwPrivate?

Thanks, fixed.

> * "private" is a bad identifier name because it's a C++ keyword.
> We should rename FdwExecutionState->private.

Renamed to fdw_private, including another 'private' in FdwPlan.

> > 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.

When I was writing file_fdw, I hoped to use static functions in
explain.c such as ExplainProperty() to handle complex information. 
Even for single plan node, I think that filename and size (currently
they are printed in a plain text together) should be separated in the
output of explain, especially when the format was XML or JSON.

Regards,
--
Shigeru Hanada