SQL/MED - file_fdw

On Thu, 25 Nov 2010 17:12:44 +0900
Shigeru HANADA <hanada@metrosystems.co.jp> wrote:

Attached is a patch that adds file_fdw, FDW which reads records from
files on the server side, as a contrib module. This patch is based on
"SQL/MED core functionality" patch.

[SQL/MED - core functionality]
http://archives.postgresql.org/pgsql-hackers/2010-11/msg01698.php

I'm going to add new CommitFest items for this patch and "SQL/MED -
postgresql_fdw" patch which have been split from "SQL/MED" patch. Can
I add them to CF 2010-11 which original "SQL/MED" item is in? Or
should I add them to CF 2011-01?

Regards,
--
Shigeru Hanada

On Thu, Nov 25, 2010 at 05:51:11PM +0900, Shigeru HANADA wrote:

On Thu, 25 Nov 2010 17:12:44 +0900
Shigeru HANADA <hanada@metrosystems.co.jp> wrote:

Attached is a patch that adds file_fdw, FDW which reads records from
files on the server side, as a contrib module. This patch is based on
"SQL/MED core functionality" patch.

[SQL/MED - core functionality]
http://archives.postgresql.org/pgsql-hackers/2010-11/msg01698.php

I'm going to add new CommitFest items for this patch and "SQL/MED -
postgresql_fdw" patch which have been split from "SQL/MED" patch. Can
I add them to CF 2010-11 which original "SQL/MED" item is in? Or
should I add them to CF 2011-01?

The original.

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Thu, 25 Nov 2010 18:40:09 -0800
David Fetter <david@fetter.org> wrote:

On Thu, Nov 25, 2010 at 05:51:11PM +0900, Shigeru HANADA wrote:

I'm going to add new CommitFest items for this patch and "SQL/MED -
postgresql_fdw" patch which have been split from "SQL/MED" patch. Can
I add them to CF 2010-11 which original "SQL/MED" item is in? Or
should I add them to CF 2011-01?

The original.

Thanks, added them to CF 2010-11.

--
Shigeru Hanada

On 11/25/2010 03:12 AM, Shigeru HANADA wrote:

Hi, hackers,

Attached is a patch that adds file_fdw, FDW which reads records from
files on the server side, as a contrib module. This patch is based on
"SQL/MED core functionality" patch.

[SQL/MED - core functionality]
http://archives.postgresql.org/pgsql-hackers/2010-11/msg01698.php

File_fdw can be installed with the steps similar to other contrib
modules, and you can create FDW with the script:
$SHAREDIR/contrib/file_fdw.sql
Note that you need to create file_fdw for each database.

Document for file_fdw is included in the patch, although the contents
might not be enough.

Any comments and questions are welcome.

Looking at file_parser.c, it seems to be largely taken from copy.c.
Wouldn't it be better to call those functions, or refactor them so they
are callable if necessary?

cheers

andrew

On Sun, Dec 5, 2010 at 07:24, Andrew Dunstan <andrew@dunslane.net> wrote:

Looking at file_parser.c, it seems to be largely taken from copy.c. Wouldn't
it be better to call those functions, or refactor them so they are callable
if necessary?

We could export private functions and structs in copy.c,
though details of the implementation should be kept in copy.c.

How about splitting the file_fdw patch into two pieces?
One exports the copy functions from the core, and another
implements file_fdw using the infrastructure.

--
Itagaki Takahiro

On 12/04/2010 11:11 PM, Itagaki Takahiro wrote:

On Sun, Dec 5, 2010 at 07:24, Andrew Dunstan<andrew@dunslane.net> wrote:

Looking at file_parser.c, it seems to be largely taken from copy.c. Wouldn't
it be better to call those functions, or refactor them so they are callable
if necessary?

We could export private functions and structs in copy.c,
though details of the implementation should be kept in copy.c.

How about splitting the file_fdw patch into two pieces?
One exports the copy functions from the core, and another
implements file_fdw using the infrastructure.

Yes please.

cheers

andrew

2010/11/25 Shigeru HANADA <hanada@metrosystems.co.jp>:

Hi, hackers,

Attached is a patch that adds file_fdw, FDW which reads records from
files on the server side, as a contrib module.  This patch is based on
"SQL/MED core functionality" patch.

[SQL/MED - core functionality]
http://archives.postgresql.org/pgsql-hackers/2010-11/msg01698.php

File_fdw can be installed with the steps similar to other contrib
modules, and you can create FDW with the script:
   $SHAREDIR/contrib/file_fdw.sql
Note that you need to create file_fdw for each database.

Document for file_fdw is included in the patch, although the contents
might not be enough.

Any comments and questions are welcome.

I think it is better to add encoding option to FileFdwOption. In the
patch the encoding of file is assumed as client_encoding, but we may
want to SELECT from different-encoded csv in a query.

Apart from the issue with fdw, I've been thinking client_encoding for
COPY is not appropriate in any way. client_encoding is the encoding of
the statement the client sends, not the COPY target which is on the
server's filesystem. Adding encoding option to COPY will eliminate
allowEncodingChanges option from JDBC driver.

Regards,

--
Hitoshi Harada

On Mon, Dec 6, 2010 at 5:48 AM, Hitoshi Harada <umi.tanuki@gmail.com> wrote:

I think it is better to add encoding option to FileFdwOption. In the
patch the encoding of file is assumed as client_encoding, but we may
want to SELECT from different-encoded csv in a query.

Apart from the issue with fdw, I've been thinking client_encoding for
COPY is not appropriate in any way. client_encoding is the encoding of
the statement the client sends, not the COPY target which is on the
server's filesystem. Adding encoding option to COPY will eliminate
allowEncodingChanges option from JDBC driver.

Yeah, this point has been raised before, and I agree with it. I
haven't heard anyone who speaks a European language complain about
this, but it seems to keep coming up for Japanese speakers. I am
guessing that means that using multiple encodings is fairly common in
Japan. I typically don't run into anything other than UTF-8 and
Latin-1, which are mostly compatible especially if you're an English
speaker, but if it weren't for that happy coincidence I think this
would be quite annoying.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 12/04/2010 11:11 PM, Itagaki Takahiro wrote:

On Sun, Dec 5, 2010 at 07:24, Andrew Dunstan<andrew@dunslane.net> wrote:

Looking at file_parser.c, it seems to be largely taken from copy.c. Wouldn't
it be better to call those functions, or refactor them so they are callable
if necessary?

We could export private functions and structs in copy.c,
though details of the implementation should be kept in copy.c.

How about splitting the file_fdw patch into two pieces?
One exports the copy functions from the core, and another
implements file_fdw using the infrastructure.

Who is actually going to do this split?

cheers

andrew

On Sat, Dec 11, 2010 at 05:30, Andrew Dunstan <andrew@dunslane.net> wrote:

On 12/04/2010 11:11 PM, Itagaki Takahiro wrote:

One exports the copy functions from the core, and another
implements file_fdw using the infrastructure.

Who is actually going to do this split?

I'm working for it :-) I extract those functions from copy.c:

- CopyState BeginCopyFrom(Relation rel, const char *filename,
List *attnamelist, List *options);
- void EndCopyFrom(CopyState cstate);
- bool NextCopyFrom(CopyState cstate,
Datum *values, bool *nulls, Oid *oid);

There was Reset() in file_fdw, but it is not contained in the
patch. It will be added again if required, but I wonder we might
need not only reset but also mark/restore a position in a file.

--
Itagaki Takahiro

On 12/13/2010 01:31 AM, Itagaki Takahiro wrote:

On Sat, Dec 11, 2010 at 05:30, Andrew Dunstan<andrew@dunslane.net> wrote:

On 12/04/2010 11:11 PM, Itagaki Takahiro wrote:

One exports the copy functions from the core, and another
implements file_fdw using the infrastructure.

Who is actually going to do this split?

I'm working for it :-) I extract those functions from copy.c:

- CopyState BeginCopyFrom(Relation rel, const char *filename,
List *attnamelist, List *options);
- void EndCopyFrom(CopyState cstate);
- bool NextCopyFrom(CopyState cstate,
Datum *values, bool *nulls, Oid *oid);

There was Reset() in file_fdw, but it is not contained in the
patch. It will be added again if required, but I wonder we might
need not only reset but also mark/restore a position in a file.

Hmm. I don't think that's going to expose enough for what I want to be
able to do. I actually had in mind exposing lower level routines like
CopyReadAttibutesCSV/CopyReadAttributesText and allowing the Foreign
Data Wrapper to manipulate the raw values read (for example from an
irregularly shaped CSV file).

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:

Hmm. I don't think that's going to expose enough for what I want to be
able to do. I actually had in mind exposing lower level routines like
CopyReadAttibutesCSV/CopyReadAttributesText and allowing the Foreign
Data Wrapper to manipulate the raw values read (for example from an
irregularly shaped CSV file).

I think that exposing the guts of COPY to the open air is a bad idea.
We refactor that code for performance or other reasons every release or
two. I don't want to find us tied down to the current implementation
because we're afraid of breaking third-party FDWs.

regards, tom lane

On 12/13/2010 11:12 AM, Tom Lane wrote:

Andrew Dunstan<andrew@dunslane.net> writes:

Hmm. I don't think that's going to expose enough for what I want to be
able to do. I actually had in mind exposing lower level routines like
CopyReadAttibutesCSV/CopyReadAttributesText and allowing the Foreign
Data Wrapper to manipulate the raw values read (for example from an
irregularly shaped CSV file).

I think that exposing the guts of COPY to the open air is a bad idea.
We refactor that code for performance or other reasons every release or
two. I don't want to find us tied down to the current implementation
because we're afraid of breaking third-party FDWs.

In that case I guess I'll need to do what Shigeru-san has done, and copy
large parts of copy.c.

cheers

andrew

On Tue, Dec 14, 2010 at 01:25, Andrew Dunstan <andrew@dunslane.net> wrote:

On 12/13/2010 11:12 AM, Tom Lane wrote:

I think that exposing the guts of COPY to the open air is a bad idea.

I don't want to export the details, too.

In that case I guess I'll need to do what Shigeru-san has done, and copy
large parts of copy.c.

I found file_fdw would require the executor state in CopyState and
the error callback function. I revised the patch to export them.
Now 5 functions are exported from copy.c:

- BeginCopyFrom(rel, filename, attnamelist, options) : CopyState
- EndCopyFrom(cstate) : void
- NextCopyFrom(cstate, OUT values, OUT nulls, OUT tupleOid) : bool
- GetCopyExecutorState(cstate) : EState *
- CopyFromErrorCallback(arg)

Are they enough, Shigeru-san? Note that the internal CopyFrom() is
now implemented only with them, so I think file_fdw is also possible.

BTW, we might have another choice instead of GetCopyExecutorState()
because the EState will be used only for ResetPerTupleExprContext()
in file_fdw. If NextCopyFrom() returns a HeapTuple instead of values
and nulls arrays, we could hide EState in NextCopyFrom().

--
Itagaki Takahiro

On Tue, 14 Dec 2010 12:01:36 +0900
Itagaki Takahiro <itagaki.takahiro@gmail.com> wrote:

On Tue, Dec 14, 2010 at 01:25, Andrew Dunstan <andrew@dunslane.net> wrote:

On 12/13/2010 11:12 AM, Tom Lane wrote:
In that case I guess I'll need to do what Shigeru-san has done, and copy
large parts of copy.c.

I found file_fdw would require the executor state in CopyState and
the error callback function. I revised the patch to export them.
Now 5 functions are exported from copy.c:

- BeginCopyFrom(rel, filename, attnamelist, options) : CopyState
- EndCopyFrom(cstate) : void
- NextCopyFrom(cstate, OUT values, OUT nulls, OUT tupleOid) : bool
- GetCopyExecutorState(cstate) : EState *
- CopyFromErrorCallback(arg)

Are they enough, Shigeru-san? Note that the internal CopyFrom() is
now implemented only with them, so I think file_fdw is also possible.

In addition to above, ResetCopyFrom() is necessary to support nested
loops which inner node is a ForeignScan.

On the other hand, I think that MarkPos()/RestrPos() wouldn't be
necessary until ForeignScan supports ordered output. ForeignScan
can't become direct child of MergeJoin because ForeignScan is not an
ordered scan, at least in current SQL/MED implementation.

Regards,
--
Shigeru Hanada

On Tue, Dec 14, 2010 at 15:31, Shigeru HANADA <hanada@metrosystems.co.jp> wrote:

- BeginCopyFrom(rel, filename, attnamelist, options) : CopyState
- EndCopyFrom(cstate) : void
- NextCopyFrom(cstate, OUT values, OUT nulls, OUT tupleOid) : bool
- GetCopyExecutorState(cstate) : EState *
- CopyFromErrorCallback(arg)

Are they enough, Shigeru-san?  Note that the internal CopyFrom() is
now implemented only with them, so I think file_fdw is also possible.

In addition to above, ResetCopyFrom() is necessary to support nested
loops which inner node is a ForeignScan.

I think you can add ResetCopyFrom() to the core in your next file_fdw
patch because the function is not used by COPY command.

I'll note other differences between the API and your FileState:

- There are no superuser checks in the exported functions because
the restriction should be only at CREATE/ALTER FOREIGN TABLE.
If the superuser grants SELECT privileges to normal users, they
should be able to read the file contents.
(But we might need to hide the file path.)
- errcontext and values/nulls arrays are not included in CopyState.
They will be additionally kept in a holder of the CopyState.
- You need to pass non-NULL filename. If it is NULL, the server
tries to read data from the client.
- The framework supports to read dumped binary files and files
with OIDs. If you don't want to support them, please check
parameters not to include those options.

--
Itagaki Takahiro

Hi hackers,

Attached is the revised WIP version of file_fdw patch. This patch
should be applied after both of fdw_syntax and fdw_scan patches, which
have been posted to another thread "SQL/MED - core functionality".

In this version, file_fdw consists of two parts, file_fdw core part
and copy of COPY FROM codes as they were in last version. The reason
of this form is to make it possible to test actual SELECT statement
ASAP. I'll revise file_fdw again according to Itagaki-san's
export-copy-routines patch.

Note that this version of file_fdw doesn't support force_not_null
option because column-level generic option is not supported by current
fdw_syntax. It will be available if column-level generic option is
implemented.

And, as possible implementation of FDW-specific EXPLAIN information,
EXPLAIN SELECT xxx FROM file shows name and size of the file. It
may be better to hide file information if the user was not superuser
for security reason. If so, filename option should not appear in
output of \det psql command too.

Regards,
--
Shigeru Hanada

On Tue, 14 Dec 2010 15:51:18 +0900
Itagaki Takahiro <itagaki.takahiro@gmail.com> wrote:

On Tue, Dec 14, 2010 at 15:31, Shigeru HANADA <hanada@metrosystems.co.jp> wrote:

In addition to above, ResetCopyFrom() is necessary to support nested
loops which inner node is a ForeignScan.

I think you can add ResetCopyFrom() to the core in your next file_fdw
patch because the function is not used by COPY command.

Agreed. I tried your patch with adding ResetCopyFrom() to copy.c, and
found the patch works fine for superuser at least.

I'll note other differences between the API and your FileState:

- There are no superuser checks in the exported functions because
the restriction should be only at CREATE/ALTER FOREIGN TABLE.
If the superuser grants SELECT privileges to normal users, they
should be able to read the file contents.
(But we might need to hide the file path.)
- errcontext and values/nulls arrays are not included in CopyState.
They will be additionally kept in a holder of the CopyState.
- You need to pass non-NULL filename. If it is NULL, the server
tries to read data from the client.
- The framework supports to read dumped binary files and files
with OIDs. If you don't want to support them, please check
parameters not to include those options.

All differences above wouldn't be serious problem, but I worry about
difference between file_fdw and COPY FROM.

"COPY FROM" is a command which INSERT data from a file essentially,
so it requires RowExclusiveLock on the target table. On the other
hand, file_fdw is a feature which reads data from a file through a
table, so it requires AccessShareLock on the source table.

Current export_copy patch doesn't allow non-superusers to fetch data
from files because BeginCopy() acquires RowExclusiveLock when
SELECTing from file_fdw table.

Using COPY routines from file_fdw might need another kind of
modularization, such as split file operation from COPY module and use
it from both of COPY and file_fdw. But it would require more code work,
and possibly performance tests.

Regards,
--
Shigeru Hanada

On Thu, Dec 16, 2010 at 18:45, Shigeru HANADA <hanada@metrosystems.co.jp> wrote:

"COPY FROM" is a command which INSERT data from a file essentially,
so it requires RowExclusiveLock on the target table.  On the other
hand, file_fdw is a feature which reads data from a file through a
table, so it requires AccessShareLock on the source table.

Ah, I found my bug in BeginCopy(), but it's in the usage of
ExecCheckRTPerms() rather than RowExclusiveLock, right?
The target relation should have been opened and locked by the caller.
I think we can move the check to DoCopy() as like as checking for
superuser(). In my understanding, we don't have to check permissions
in each FDW because it was done in parse and analyze phases.
Could you fix it? Or, shall I do?

Using COPY routines from file_fdw might need another kind of
modularization,  such as split file operation from COPY module and use
it from both of COPY and file_fdw.  But it would require more code work,
and possibly performance tests.

My plan was that the 'rel' argument for BeginCopyFrom() is a "template"
for the CSV file. So, we need only AccessShareLock (or, NoLock?) for
the relation. TupleDesc might be enough for the purpose, but I've not
changed the type because of DEFAULT columns.

OTOH, CopyFrom(cstate, rel) will require an additional 'rel' argument,
that means the "target" to be inserted, though it's always same with
the above "template" in COPY FROM. RowExclusiveLock is required only
for the target relation.

--
Itagaki Takahiro