Re: SQL/MED - file_fdw

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.

Regards,
--
Shigeru Hanada

On Thu, 25 Nov 2010 17:12:44 +0900
Shigeru HANADA <hanada(at)metrosystems(dot)co(dot)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(at)metrosystems(dot)co(dot)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(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)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(at)fetter(dot)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(at)dunslane(dot)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(at)dunslane(dot)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(at)metrosystems(dot)co(dot)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(dot)tanuki(at)gmail(dot)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(at)dunslane(dot)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(at)dunslane(dot)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(at)dunslane(dot)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(at)dunslane(dot)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(at)dunslane(dot)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(at)dunslane(dot)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(dot)takahiro(at)gmail(dot)com> wrote:
> On Tue, Dec 14, 2010 at 01:25, Andrew Dunstan <andrew(at)dunslane(dot)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(at)metrosystems(dot)co(dot)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(dot)takahiro(at)gmail(dot)com> wrote:
> On Tue, Dec 14, 2010 at 15:31, Shigeru HANADA <hanada(at)metrosystems(dot)co(dot)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(at)metrosystems(dot)co(dot)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

On Thu, Dec 16, 2010 at 5:35 AM, Itagaki Takahiro
<itagaki(dot)takahiro(at)gmail(dot)com> wrote:
> 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?

I believe that our project policy is that permissions checks must be
done at execution time, not parse/plan time.

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

On Thu, Dec 16, 2010 at 23:09, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I believe that our project policy is that permissions checks must be
> done at execution time, not parse/plan time.

Oops, yes. I should have said "permission checks for foreign tables
should have done in their own execution". So, additional checks in
each FDW are not required eventually.

In addition, we allow users to read the definition of the columns and
default values even if they don't have SELECT permission. So, I still
think permission checks for the template relation are not required in
the file reader API. But we need the checks in COPY FROM command because
the relation is used not only as a template but also as a target.

=> SELECT * FROM tbl;
ERROR: permission denied for relation tbl
=> \d+ tbl
Table "public.tbl"
Column | Type | Modifiers | Storage | Description
--------+---------+-----------+---------+-------------
i | integer | | plain |
j | integer | default 5 | plain |
Has OIDs: no

--
Itagaki Takahiro

On Thu, 16 Dec 2010 19:35:56 +0900
Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> wrote:
> On Thu, Dec 16, 2010 at 18:45, Shigeru HANADA <hanada(at)metrosystems(dot)co(dot)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.

The target foreign tables are locked in InitForeignScan() via
ExecOpenScanRelation(), so COPY routines don't need to lock it again.

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

In addition, ISTM that the check for read-only transaction should be
moved to DoCopy() because file_fdw scan can be used in read-only
transacntion.

> Could you fix it? Or, shall I do?

I've just moved permission check and read-only check from BeginCopy()
to DoCopy(). Please see attached patch.

Regards,
--
Shigeru Hanada

On Fri, Dec 17, 2010 at 11:49, Shigeru HANADA <hanada(at)metrosystems(dot)co(dot)jp> wrote:
> I've just moved permission check and read-only check from BeginCopy()
> to DoCopy().  Please see attached patch.

Thanks!

Are there any objections for the change? If acceptable,
I'd like to apply it prior to SQL/MED and file_fdw patches.

We could have some better name for Begin/Next/EndCopyFrom() and
the exported CopyState. As Shigeru mentioned, COPY FROM consists of
"a file reader" and "a heap inserter", but file_fdw only uses the
file reader. In addition, we could split CopyState into two versions
for COPY FROM and COPY TO later. So, it might be better to export
them as "FileReader API" or some similar names rather than CopyFrom.

--
Itagaki Takahiro

On Fri, Dec 17, 2010 at 11:01 PM, Itagaki Takahiro
<itagaki(dot)takahiro(at)gmail(dot)com> wrote:
> On Fri, Dec 17, 2010 at 11:49, Shigeru HANADA <hanada(at)metrosystems(dot)co(dot)jp> wrote:
>> I've just moved permission check and read-only check from BeginCopy()
>> to DoCopy().  Please see attached patch.
>
> Thanks!
>
> Are there any objections for the change? If acceptable,
> I'd like to apply it prior to SQL/MED and file_fdw patches.

I think at a bare minimum the functions you're adding should have a
comment explaining what they do, what their arguments mean, etc.

I'm sort of suspicious of the fact that BeginCopyTo() is a shell
around BeginCopy() while BeginCopyFrom() does a whole bunch of other
stuff. I haven't grokked what the code is doing here well enough to
have a concrete proposal though...

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

On Sun, Dec 19, 2010 at 12:18, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I'm sort of suspicious of the fact that BeginCopyTo() is a shell
> around BeginCopy() while BeginCopyFrom() does a whole bunch of other
> stuff.  I haven't grokked what the code is doing here well enough to
> have a concrete proposal though...

I added Begin/EndCopyTo() just because the internal code looks
symmetric. The proposal doesn't change behaviors of COPY commands
at all. It just exports a part of COPY FROM codes as "File Reader"
so that the file_fdw external module can reuse the code. I believe
we have the conclusion that we should avoid code duplication
to read files in the prior discussion.

We could arrange COPY TO codes as like as the COPY FROM APIs, but
I've not and I won't do that at this time because it is not required
by SQL/MED at all. If we do, it would be "File Writer" APIs, like:

cstate = BeginCopyTO(...);
while (tuple = ReadTupleFromSomewhere()) {
/* write the tuple into a TSV/CSV file */
NextCopyTo(cstate, tuple);
}
EndCopyTo(cstate);

--
Itagaki Takahiro

On Sat, Dec 18, 2010 at 10:43 PM, Itagaki Takahiro
<itagaki(dot)takahiro(at)gmail(dot)com> wrote:
> On Sun, Dec 19, 2010 at 12:18, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> I'm sort of suspicious of the fact that BeginCopyTo() is a shell
>> around BeginCopy() while BeginCopyFrom() does a whole bunch of other
>> stuff.  I haven't grokked what the code is doing here well enough to
>> have a concrete proposal though...
>
> I added Begin/EndCopyTo() just because the internal code looks
> symmetric. The proposal doesn't change behaviors of COPY commands
> at all. It just exports a part of COPY FROM codes as "File Reader"
> so that the file_fdw external module can reuse the code. I believe
> we have the conclusion that we should avoid code duplication
> to read files in the prior discussion.

I'm not questioning any of that. But I'd like the resulting code to
be as maintainable as we can make it.

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

On Sun, Dec 19, 2010 at 12:45, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I'm not questioning any of that.  But I'd like the resulting code to
> be as maintainable as we can make it.

I added comments and moved some setup codes for COPY TO to BeginCopyTo()
for maintainability. CopyTo() still contains parts of initialization,
but I've not touched it yet because we don't need the arrangement for now.

--
Itagaki Takahiro

On Mon, 20 Dec 2010 20:42:38 +0900
Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> wrote:
> On Sun, Dec 19, 2010 at 12:45, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > I'm not questioning any of that.  But I'd like the resulting code to
> > be as maintainable as we can make it.
>
> I added comments and moved some setup codes for COPY TO to BeginCopyTo()
> for maintainability. CopyTo() still contains parts of initialization,
> but I've not touched it yet because we don't need the arrangement for now.

Attached is the revised version of file_fdw patch. This patch is
based on Itagaki-san's copy_export-20101220.diff patch.

Changes from previous version are:

* file_fdw uses CopyErrorCallback() as error context callback routine
in fileIterate() to report error context. "CONTEXT" line in the
example below is added by the callback.

postgres=# select * From csv_tellers_bad;
ERROR: missing data for column "bid"
CONTEXT: COPY csv_tellers_bad, line 10: "10"
postgres=#

* Only superusers can change table-level file_fdw options. Normal
user can't change the options even if the user was the owner of the
table. This is for security reason.

Regards,
--
Shigeru Hanada

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

On Mon, Dec 20, 2010 at 6:42 AM, Itagaki Takahiro
<itagaki(dot)takahiro(at)gmail(dot)com> wrote:
> On Sun, Dec 19, 2010 at 12:45, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> I'm not questioning any of that.  But I'd like the resulting code to
>> be as maintainable as we can make it.
>
> I added comments and moved some setup codes for COPY TO to BeginCopyTo()
> for maintainability. CopyTo() still contains parts of initialization,
> but I've not touched it yet because we don't need the arrangement for now.

I haven't analyzed this enough to know whether I agree with it, but as
a trivial matter you should certainly revert this hunk:

/* field raw data pointers found by COPY FROM */
-
- int max_fields;
- char ** raw_fields;
+ int max_fields;
+ char **raw_fields;

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

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

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

--
Itagaki Takahiro

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

On Fri, Dec 24, 2010 at 20:04, Shigeru HANADA <hanada(at)metrosystems(dot)co(dot)jp> wrote:
> Iterate is called in query context,

Is it an unavoidable requirement? If possible, I'd like to use per-tuple memory
context as the default. We use per-tuple context in FunctionScan for SETOF
functions. I hope we could have little difference between SRF and FDW APIs.

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

We could add tts_tableOid into TupleTableSlot. We'd better avoid
materializing slot only for the tableoid support in foreign tables.
Almost all of the foreign tables should have different data format
from HeapTuple, including pgsql_fdw.

--
Itagaki Takahiro

On Mon, Dec 20, 2010 at 20:42, Itagaki Takahiro
<itagaki(dot)takahiro(at)gmail(dot)com> wrote:
> I added comments and moved some setup codes for COPY TO to BeginCopyTo()
> for maintainability. CopyTo() still contains parts of initialization,
> but I've not touched it yet because we don't need the arrangement for now.

I updated the COPY FROM API patch.
- GetCopyExecutorState() is removed because FDWs will use their own context.

The patch just rearranges codes for COPY FROM to export those functions.
It also modifies some of COPY TO codes internally for code readability.
- BeginCopyFrom(rel, filename, attnamelist, options)
- EndCopyFrom(cstate)
- NextCopyFrom(cstate, OUT values, OUT nulls, OUT tupleOid)
- CopyFromErrorCallback(arg)

Some items to be considered:
- BeginCopyFrom() could receive filename as an option instead of a separated
argument. If do so, file_fdw would be more simple, but it's a change only for
file_fdw. COPY commands in the core won't be improved at all.
- NextCopyFrom() returns values/nulls arrays rather than a HeapTuple. I expect
the caller store the result into tupletableslot with ExecStoreVirtualTuple().
It is designed for performance, but if the caller always needs an materialized
HeapTuple, HeapTuple is better for the result type.

--
Itagaki Takahiro

On Fri, 7 Jan 2011 10:57:17 +0900
Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> wrote:
> I updated the COPY FROM API patch.
> - GetCopyExecutorState() is removed because FDWs will use their own context.
>
> The patch just rearranges codes for COPY FROM to export those functions.
> It also modifies some of COPY TO codes internally for code readability.
> - BeginCopyFrom(rel, filename, attnamelist, options)
> - EndCopyFrom(cstate)
> - NextCopyFrom(cstate, OUT values, OUT nulls, OUT tupleOid)
> - CopyFromErrorCallback(arg)

For the purpose of file_fdw, additional ResetCopyFrom() would be
necessary. I'm planning to include such changes in file_fdw patch.
Please find attached partial patch for ResetCopyFrom(). Is there
anything else which should be done at reset?

> Some items to be considered:
> - BeginCopyFrom() could receive filename as an option instead of a separated
> argument. If do so, file_fdw would be more simple, but it's a change only for
> file_fdw. COPY commands in the core won't be improved at all.

> - NextCopyFrom() returns values/nulls arrays rather than a HeapTuple. I expect
> the caller store the result into tupletableslot with ExecStoreVirtualTuple().
> It is designed for performance, but if the caller always needs an materialized
> HeapTuple, HeapTuple is better for the result type.

IIUC, materizlizing is for tableoid system column. If we could add
tts_tableoid into TupleTableSlot, virtual tuple would be enough. In
this design, caller can receive results with tts_values/tts_isnull
arrays.

Regards,
--
Shigeru Hanada

Shigeru HANADA <hanada(at)metrosystems(dot)co(dot)jp> writes:
> For the purpose of file_fdw, additional ResetCopyFrom() would be
> necessary. I'm planning to include such changes in file_fdw patch.
> Please find attached partial patch for ResetCopyFrom(). Is there
> anything else which should be done at reset?

Seems like it would be smarter to close and re-open the copy operation.
Adding a reset function is just creating an additional maintenance
burden and point of failure, for what seems likely to be a negligible
performance benefit.

If you think it's not negligible, please show some proof of that before
asking us to support such code.

regards, tom lane

On Mon, 10 Jan 2011 19:26:11 -0500
Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Shigeru HANADA <hanada(at)metrosystems(dot)co(dot)jp> writes:
> > For the purpose of file_fdw, additional ResetCopyFrom() would be
> > necessary. I'm planning to include such changes in file_fdw patch.
> > Please find attached partial patch for ResetCopyFrom(). Is there
> > anything else which should be done at reset?
>
> Seems like it would be smarter to close and re-open the copy operation.
> Adding a reset function is just creating an additional maintenance
> burden and point of failure, for what seems likely to be a negligible
> performance benefit.

Agreed. fileReScan can be implemented with close/re-open with storing
some additional information into FDW private area. I would withdraw
the proposal.

> If you think it's not negligible, please show some proof of that before
> asking us to support such code.

Anyway, I've measured overhead of re-open with executing query
including inner join between foreign tables copied from pgbench schema.
I used SELECT statement below:

EXPLAIN (ANALYZE) SELECT count(*) FROM csv_accounts a JOIN
csv_branches b ON (b.bid = a.bid);

On the average of (Nested Loop - (Foreign Scan * 2)), overhead of
re-open is round 0.048ms per tuple (average of 3 times measurement).

After the implementation of file_fdw, I'm going to measure again. If
ResetCopyFrom significantly improves performance of ReScan, I'll
propose it as a separate patch.

=========================================================================

The results of EXPLAIN ANALYZE are:

[using ResetCopyFrom]
QUERY PLAN
--------------------------------------------------------------------------------------------------------------------------------------------
Aggregate (cost=11717.02..11717.03 rows=1 width=0) (actual time=73357.655..73357.657 rows=1 loops=1)
-> Nested Loop (cost=0.00..11717.01 rows=1 width=0) (actual time=0.209..71424.059 rows=1000000 loops=1)
-> Foreign Scan on public.csv_accounts a (cost=0.00..11717.00 rows=1 width=4) (actual time=0.144..6998.497 rows=1000000 loops=1)
-> Foreign Scan on public.csv_branches b (cost=0.00..0.00 rows=1 width=4) (actual time=0.008..0.037 rows=10 loops=1000000)
Total runtime: 73358.135 ms
(11 rows)

[using EndCopyFrom + BeginCopyFrom]
QUERY PLAN
--------------------------------------------------------------------------------------------------------------------------------------------
Aggregate (cost=11717.02..11717.03 rows=1 width=0) (actual time=120724.138..120724.140 rows=1 loops=1)
-> Nested Loop (cost=0.00..11717.01 rows=1 width=0) (actual time=0.321..118583.681 rows=1000000 loops=1)
-> Foreign Scan on public.csv_accounts a (cost=0.00..11717.00 rows=1 width=4) (actual time=0.156..7208.968 rows=1000000 loops=1)
-> Foreign Scan on public.csv_branches b (cost=0.00..0.00 rows=1 width=4) (actual time=0.016..0.046 rows=10 loops=1000000)
Total runtime: 121118.792 ms
(11 rows)

Time: 121122.205 ms

=========================================================================

Regards,
--
Shigeru Hanada

On Fri, 7 Jan 2011 10:57:17 +0900
Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> wrote:
> On Mon, Dec 20, 2010 at 20:42, Itagaki Takahiro
> <itagaki(dot)takahiro(at)gmail(dot)com> wrote:
> > I added comments and moved some setup codes for COPY TO to BeginCopyTo()
> > for maintainability. CopyTo() still contains parts of initialization,
> > but I've not touched it yet because we don't need the arrangement for now.
>
> I updated the COPY FROM API patch.
> - GetCopyExecutorState() is removed because FDWs will use their own context.

I rebased file_fdw patch to recent copy_export patch, and have some
comments.

> The patch just rearranges codes for COPY FROM to export those functions.
> It also modifies some of COPY TO codes internally for code readability.
> - BeginCopyFrom(rel, filename, attnamelist, options)
> - EndCopyFrom(cstate)
> - NextCopyFrom(cstate, OUT values, OUT nulls, OUT tupleOid)
> - CopyFromErrorCallback(arg)

This API set seems to be enough to implement file_fdw using COPY
routines.

But EndCopyFrom() seems not to be able to release memory which is
allocated in BeginCopy() and BeginCopyFrom(). I found this behavior
by executing a query which generates nested loop plan (outer 1000000
row * inner 10 row), and at last postgres grows up to 300MB+ from
108MB (VIRT of top command).

Attached patch would avoid this leak by adding per-copy context to
CopyState. This would be overkill, and ResetCopyFrom() might be
reasonable though.

Anyway, I couldn't find performance degrade with this patch (tested on
my Linux box).

==============
# csv_accounts and csv_branches are generated by:
1) pgbench -i -s 10
2) COPY pgbench_accounts to '/path/to/accounts.csv' WITH CSV;
3) COPY pgbench_branches to '/path/to/branches.csv' WITH CSV;

<Original (There is no memory swap during measurement) >
postgres=# explain analyze select * from csv_accounts b, csv_branches t where t.bid = b.bid;
QUERY PLAN
---------------------------------------------------------------------------------------------------------------------------------
Nested Loop (cost=0.00..11717.01 rows=1 width=200) (actual time=0.300..100833.057 rows=1000000 loops=1)
Join Filter: (b.bid = t.bid)
-> Foreign Scan on csv_accounts b (cost=0.00..11717.00 rows=1 width=100) (actual time=0.148..4437.595 rows=1000000 loops=1)
-> Foreign Scan on csv_branches t (cost=0.00..0.00 rows=1 width=100) (actual time=0.014..0.039 rows=10 loops=1000000)
Total runtime: 102882.308 ms
(5 rows)

<Patched, Using per-copy context to release memory>
postgres=# explain analyze select * from csv_accounts b, csv_branches t where t.bid = b.bid;
QUERY PLAN
---------------------------------------------------------------------------------------------------------------------------------
Nested Loop (cost=0.00..11717.01 rows=1 width=200) (actual time=0.226..100931.864 rows=1000000 loops=1)
Join Filter: (b.bid = t.bid)
-> Foreign Scan on csv_accounts b (cost=0.00..11717.00 rows=1 width=100) (actual time=0.085..4439.777 rows=1000000 loops=1)
-> Foreign Scan on csv_branches t (cost=0.00..0.00 rows=1 width=100) (actual time=0.015..0.039 rows=10 loops=1000000)
Total runtime: 102684.276 ms
(5 rows)
==============

This memory leak would not be problem when using from COPY command
because it handles only one CopyState in a query, and it will be
cleaned up with parent context.

> Some items to be considered:
> - BeginCopyFrom() could receive filename as an option instead of a separated
> argument. If do so, file_fdw would be more simple, but it's a change only for
> file_fdw. COPY commands in the core won't be improved at all.

ISTM that current design would be better.

> - NextCopyFrom() returns values/nulls arrays rather than a HeapTuple. I expect
> the caller store the result into tupletableslot with ExecStoreVirtualTuple().
> It is designed for performance, but if the caller always needs an materialized
> HeapTuple, HeapTuple is better for the result type.

I tried to add tableoid to TupleTableSlot as tts_tableoid, but it
seems to make codes such as slot_getaddr() and other staff tricky.

How about to implement using materialized tuples to avoid unnecessary
(at least for functionality) changes. I would like to send this
virtual-tuple-optimization to next development cycle because it would
not effect the interface heavily. I'll post materialized-tuple
version of foreign_scan patch soon.

Regards,
--
Shigeru Hanada

On Thu, Jan 13, 2011 at 19:00, Shigeru HANADA <hanada(at)metrosystems(dot)co(dot)jp> wrote:
> But EndCopyFrom() seems not to be able to release memory which is
> allocated in BeginCopy() and BeginCopyFrom().  I found this behavior
> by executing a query which generates nested loop plan (outer 1000000
> row * inner 10 row), and at last postgres grows up to 300MB+ from
> 108MB (VIRT of top command).
>
> Attached patch would avoid this leak by adding per-copy context to
> CopyState.  This would be overkill, and ResetCopyFrom() might be
> reasonable though.

Good catch. I merged your fix into the attached patch.

BTW, why didn't planner choose a materialized plan for the inner loop?
FDW scans are typically slower than heap scans or TupleTableslot scans,
it seems reasonable for me to add a Materialize node at the top of the
inner Foreign Scan, especially when we don't use indexes for the scan
keys or join keys.

--
Itagaki Takahiro

On Fri, 14 Jan 2011 13:03:27 +0900
Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> wrote:

> Good catch. I merged your fix into the attached patch.

Thanks, I'll rebase my patches.

> BTW, why didn't planner choose a materialized plan for the inner loop?
> FDW scans are typically slower than heap scans or TupleTableslot scans,
> it seems reasonable for me to add a Materialize node at the top of the
> inner Foreign Scan, especially when we don't use indexes for the scan
> keys or join keys.

Maybe because foreign tables lack statistics, and file_fdw's estimate
isn't smart enough.

After copying statisticsof pgbench_xxx tables into csv_xxx tables,
planner generates same plans as for local tables, but costs of
ForeignScan nodes are little lower than them of SeqScan nodes.

==============================
postgres=# explain analyze select * from csv_accounts a, csv_branches b where a.bid = b.bid;
QUERY PLAN
--------------------------------------------------------------------------------------------------------------------------------------
Hash Join (cost=0.33..45467.32 rows=1000000 width=197) (actual time=0.234..8044.077 rows=1000000 loops=1)
Hash Cond: (a.bid = b.bid)
-> Foreign Scan on csv_accounts a (cost=0.00..31717.00 rows=1000000 width=97) (actual time=0.107..4147.074 rows=1000000 loops=1)
-> Hash (cost=0.20..0.20 rows=10 width=100) (actual time=0.085..0.085 rows=10 loops=1)
Buckets: 1024 Batches: 1 Memory Usage: 1kB
-> Foreign Scan on csv_branches b (cost=0.00..0.20 rows=10 width=100) (actual time=0.027..0.056 rows=10 loops=1)
Total runtime: 9690.686 ms
(7 rows)

postgres=# explain analyze select * from pgbench_accounts a, pgbench_branches b where a.bid = b.bid;
QUERY PLAN
--------------------------------------------------------------------------------------------------------------------------------------
Hash Join (cost=1.23..40145.22 rows=1000000 width=197) (actual time=0.146..5693.883 rows=1000000 loops=1)
Hash Cond: (a.bid = b.bid)
-> Seq Scan on pgbench_accounts a (cost=0.00..26394.00 rows=1000000 width=97) (actual time=0.073..1884.018 rows=1000000 loops=1)
-> Hash (cost=1.10..1.10 rows=10 width=100) (actual time=0.048..0.048 rows=10 loops=1)
Buckets: 1024 Batches: 1 Memory Usage: 1kB
-> Seq Scan on pgbench_branches b (cost=0.00..1.10 rows=10 width=100) (actual time=0.003..0.021 rows=10 loops=1)
Total runtime: 7333.713 ms
(7 rows)
==============================

Forced Nested Loop uses Materialize node as expected.

==============================
postgres=# set enable_hashjoin = false;
SET
postgres=# explain select * from csv_accounts a, csv_branches b where a.bid = b.bid;
QUERY PLAN
-----------------------------------------------------------------------------------
Nested Loop (cost=0.00..181717.23 rows=1000000 width=197)
Join Filter: (a.bid = b.bid)
-> Foreign Scan on csv_accounts a (cost=0.00..31717.00 rows=1000000 width=97)
-> Materialize (cost=0.00..0.25 rows=10 width=100)
-> Foreign Scan on csv_branches b (cost=0.00..0.20 rows=10 width=100)
(5 rows)

postgres=# explain select * from pgbench_accounts a, pgbench_branches b where a.bid = b.bid;
QUERY PLAN
-----------------------------------------------------------------------------------
Nested Loop (cost=0.00..176395.12 rows=1000000 width=197)
Join Filter: (a.bid = b.bid)
-> Seq Scan on pgbench_accounts a (cost=0.00..26394.00 rows=1000000 width=97)
-> Materialize (cost=0.00..1.15 rows=10 width=100)
-> Seq Scan on pgbench_branches b (cost=0.00..1.10 rows=10 width=100)
(5 rows)
==============================

ISTM that new interface which is called from ANALYZE would help to
update statistics of foreign talbes. If we could leave sampling
argorythm to FDWs, acquire_sample_rows() might fit for that purpose.

If a FDW doesn't provide analyze handler, postgres might be able to
execute "SELECT * FROM foreign_table LIMIT sample_num" internally to
get sample rows.

Regards,
--
Shigeru Hanada

On Fri, Jan 14, 2011 at 14:20, Shigeru HANADA <hanada(at)metrosystems(dot)co(dot)jp> wrote:
> After copying statisticsof pgbench_xxx tables into csv_xxx tables,
> planner generates same plans as for local tables, but costs of
> ForeignScan nodes are little lower than them of SeqScan nodes.
> Forced Nested Loop uses Materialize node as expected.

Interesting. It means we need per-column statistics for foreign
tables in addition to cost values.

> ISTM that new interface which is called from ANALYZE would help to
> update statistics of foreign talbes.  If we could leave sampling
> argorythm to FDWs, acquire_sample_rows() might fit for that purpose.

We will discuss how to collect statistics from foreign tables
in the next development cycle. I think we have two choice here:

#1. Retrieve sample rows from remote foreign tables and
store stats in the local pg_statistic.
#2. Use remote statistics for each foreign table directly.

acquire_sample_rows() would be a method for #1, Another approach
for #2 is to use remote statistics directly. We provide hooks to
generate virtual statistics with get_relation_stats_hook() and
families. We could treat statistics for foreign tables in a similar
way as the hook.

file_fdw likes #1 because there are no external storage to store
statistics for CSV files, but pgsql_fdw might prefer #2 because
the remote server already has stats for the underlying table.

--
Itagaki Takahiro

On Fri, 14 Jan 2011 14:20:20 +0900
Shigeru HANADA <hanada(at)metrosystems(dot)co(dot)jp> wrote:
> On Fri, 14 Jan 2011 13:03:27 +0900
> Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> wrote:
>
> > Good catch. I merged your fix into the attached patch.
>
> Thanks, I'll rebase my patches.

I've rebased WIP patches for file_fdw onto Itagaki-san's recent
copy_export patch.

Interface of NextCopyFrom() is fixed to return HeapTuple, to support
tableoid without any change to TupleTableSlot.

Please apply patches in this order:

1) patches for FDW API (NOT attached to this message)
These patches are attached and described in this message.
http://archives.postgresql.org/pgsql-hackers/2011-01/msg01096.php

2) copy_export-20110114.patch (NOT attached to this message)
This patch is attached and described in this message.
http://archives.postgresql.org/pgsql-hackers/2011-01/msg01066.php

3) 20110114-copy_export_HeapTupe.patch
This patch fixes interface of NextCopyFrom() to return results as
HeapTuple.

4) 20110114-foreign_scan.patch
This patch adds HANDLER option of FOREIGN DATA WRAPPER, and
ForeignScan executor node. Note that no wrapper is available in this
patch.

5) 20110114-file_fdw.patch
This patch adds contrib/file_fdw, foreign-data wrapper for server-side
files. Supported file formats are similar to COPY command, and
COPY options except "force_not_null" are accepted as generic options
of foreign table.

Regards,
--
Shigeru Hanada

On Sat, Jan 15, 2011 at 08:35, Shigeru HANADA <hanada(at)metrosystems(dot)co(dot)jp> wrote:
> Interface of NextCopyFrom() is fixed to return HeapTuple, to support
> tableoid without any change to TupleTableSlot.
>
> 3) 20110114-copy_export_HeapTupe.patch
> This patch fixes interface of NextCopyFrom() to return results as
> HeapTuple.

I think file_fdw can return tuples in virtual tuples forms,
and ForeignNext() calls ExecMaterializeSlot() to store tableoid.

--
Itagaki Takahiro

On Tue, 18 Jan 2011 15:17:12 +0900
Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> wrote:

> On Sat, Jan 15, 2011 at 08:35, Shigeru HANADA <hanada(at)metrosystems(dot)co(dot)jp> wrote:
> > Interface of NextCopyFrom() is fixed to return HeapTuple, to support
> > tableoid without any change to TupleTableSlot.
> >
> > 3) 20110114-copy_export_HeapTupe.patch
> > This patch fixes interface of NextCopyFrom() to return results as
> > HeapTuple.
>
> I think file_fdw can return tuples in virtual tuples forms,
> and ForeignNext() calls ExecMaterializeSlot() to store tableoid.

Thanks for the comment. I've fixed file_fdw to use tts_values and
tts_isnull to receive results from NextCopyFrom, and store virtual
tuple in the slot.

Attached patch requires FDW API patches and copy_export-20110114.patch.
Please see also:
http://archives.postgresql.org/message-id/20110119002615.8316.6989961C@metrosystems.co.jp

And also cost estimation of file_fdw is simplified along your comments
below.

On Tue, 21 Dec 2010 21:32:17 +0900
Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> wrote:
> #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.

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.

Regards,
--
Shigeru Hanada

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

On Thu, 20 Jan 2011 22:21:37 +0900
Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> wrote:
> 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:

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

On Fri, Jan 21, 2011 at 22:12, Shigeru HANADA <hanada(at)metrosystems(dot)co(dot)jp> wrote:
>> 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.

Just an idea -- we could return complex node trees with explainInfo
if we use XML or JSON for the format. For example, pgsql_fdw can
return the result from "EXPLAIN (FORMAT json)" without modification.

It might be one of the reasons we should should support JSON in the core :)

--
Itagaki Takahiro

On Fri, Jan 21, 2011 at 8:59 AM, Itagaki Takahiro
<itagaki(dot)takahiro(at)gmail(dot)com> wrote:
> On Fri, Jan 21, 2011 at 22:12, Shigeru HANADA <hanada(at)metrosystems(dot)co(dot)jp> wrote:
>>> 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.
>
> Just an idea -- we could return complex node trees with explainInfo
> if we use XML or JSON for the format. For example, pgsql_fdw can
> return the result from "EXPLAIN (FORMAT json)" without modification.
>
> It might be one of the reasons we should should support JSON in the core :)

Nice try, but I think that'd be a real drag. You wouldn't want to
return JSON when the explain format is text, or XML.

I think we probably need to modify the EXPLAIN code so that FDWs get a
chance to inject their own customer properties into the output, but I
don't know that we need to get that done right this minute. We can
ship something really crude/basic for 9.1, if need be, and fix this up
for 9.2. Of course if it turns out that getting EXPLAIN working the
way we'd like is really easy, then we can just do it.

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

On Fri, Jan 21, 2011 at 8:12 AM, Shigeru HANADA
<hanada(at)metrosystems(dot)co(dot)jp> wrote:
>> * Try strVal() instead of DefElem->val.str
>> * FdwEPrivate seems too abbreviated for me. How about FileFdwPrivate?
>
> Thanks, fixed.

Was there supposed to be a patch attached here? Or where is it? We
are past out of time to get this committed, and there hasn't been a
new version in more than two weeks.

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

On Fri, 4 Feb 2011 10:10:56 -0500
Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Was there supposed to be a patch attached here? Or where is it? We
> are past out of time to get this committed, and there hasn't been a
> new version in more than two weeks.
Sorry for late to post patches.
Attached are revised version of file_fdw patch.

This patch is based on latest FDW API patches which are posted in
another thread "SQL/MED FDW API", and copy_export-20110104.patch which
was posted by Itagaki-san.

Regards,
--
Shigeru Hanada

On Mon, Feb 7, 2011 at 16:01, Shigeru HANADA <hanada(at)metrosystems(dot)co(dot)jp> wrote:
> This patch is based on latest FDW API patches which are posted in
> another thread "SQL/MED FDW API", and copy_export-20110104.patch which
> was posted by Itagaki-san.

I have questions about estimate_costs().

* What value does baserel->tuples have?
Foreign tables are never analyzed for now. Is the number correct?

* Your previous measurement showed it has much more startup_cost.
When you removed ReScan, it took long time but planner didn't choose
materialized plans. It might come from lower startup costs.

* Why do you use lstat() in it?
Even if the file is a symlink, we will read the linked file in the
succeeding copy. So, I think it should be stat() rather than lstat().

+estimate_costs(const char *filename, RelOptInfo *baserel,
+ double *startup_cost, double *total_cost)
+{
...
+ /* get size of the file */
+ if (lstat(filename, &stat) == -1)
+ {
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not stat file \"%s\": %m", filename)));
+ }
+
+ /*
+ * The way to estimate costs is almost same as cost_seqscan(), but there
+ * are some differences:
+ * - DISK costs are estimated from file size.
+ * - CPU costs are 10x of seq scan, for overhead of parsing records.
+ */
+ pages = stat.st_size / BLCKSZ + (stat.st_size % BLCKSZ > 0 ? 1 : 0);
+ run_cost += seq_page_cost * pages;
+
+ *startup_cost += baserel->baserestrictcost.startup;
+ cpu_per_tuple = cpu_tuple_cost + baserel->baserestrictcost.per_tuple;
+ run_cost += cpu_per_tuple * 10 * baserel->tuples;
+ *total_cost = *startup_cost + run_cost;
+
+ return stat.st_size;
+}

--
Itagaki Takahiro

On Mon, 7 Feb 2011 21:00:53 +0900
Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> wrote:
> On Mon, Feb 7, 2011 at 16:01, Shigeru HANADA <hanada(at)metrosystems(dot)co(dot)jp> wrote:
> > This patch is based on latest FDW API patches which are posted in
> > another thread "SQL/MED FDW API", and copy_export-20110104.patch which
> > was posted by Itagaki-san.
>
> I have questions about estimate_costs().
>
> * What value does baserel->tuples have?
> Foreign tables are never analyzed for now. Is the number correct?

No, baserel->tuples is always 0, and baserel->pages is 0 too.
In addition, width and rows are set to 100 and 1, as default values.
I think ANALYZE support is needed to make PostgreSQL's standard
optimization for foreign scans. At present, estimation for foreign
tables would be awful.

> * Your previous measurement showed it has much more startup_cost.
> When you removed ReScan, it took long time but planner didn't choose
> materialized plans. It might come from lower startup costs.

I tested joining file_fdw tables, accounts and branches, which are
initialized with "pgbench -i -s 10" and exported to csv files.

At first, I tried adding random_page_cost (4.0) to startup_cost as
cost to open file (though it's groundless), but materialized was not
chosen. After updating pg_class.reltuples to correct value, Hash-join
was choosen for same query. With disabling Hash-join, finally
materialized was choosen.

ISTM that choosing simple nested loop would come from wrong
estimation of loop count, but not from startup cost. IMHO, supporting
analyze (PG-style statistics) is necessary to make PostgreSQL to
generate sane plan.

> * Why do you use lstat() in it?
> Even if the file is a symlink, we will read the linked file in the
> succeeding copy. So, I think it should be stat() rather than lstat().

Good catch! Fixed version is attached.

Regards,
--
Shigeru Hanada

On Tue, Feb 8, 2011 at 00:30, Shigeru HANADA <hanada(at)metrosystems(dot)co(dot)jp> wrote:
> Fixed version is attached.

I reviewed your latest git version, that is a bit newer than the attached patch.
http://git.postgresql.org/gitweb?p=users/hanada/postgres.git;a=commit;h=0e1a1e1b0e168cb3d8ff4d637747d0ba8f7b8d55

The code still works with small adjustment, but needs to be rebased on the
latest master, especially for extension support and copy API changes.

Here are a list of comments and suggestions:

* You might forget some combination or unspecified options in
file_fdw_validator().
For example, format == NULL or !csv && header cases. I've not tested all
cases, but please recheck validations used in BeginCopy().

* estimate_costs() needs own row estimation rather than using baserel.
> > What value does baserel->tuples have?
> > Foreign tables are never analyzed for now. Is the number correct?
> No, baserel->tuples is always 0, and baserel->pages is 0 too.
> In addition, width and rows are set to 100 and 1, as default values.

It means baserel is not reliable at all, right? If so, we need alternative
solution in estimate_costs(). We adjust statistics with runtime relation
size in estimate_rel_size(). Also, we use get_rel_data_width() for not
analyzed tables. We could use similar technique in file_fdw, too.

* Should use int64 for file byte size (or, uint32 in blocks).
unsigned long might be 32bit. ulong is not portable.

* Oid List (list_make1_oid) might be more handy than Const to hold relid
in FdwPlan.fdw_private.

* I prefer FileFdwExecutionState to FileFdwPrivate, because there are
two different 'fdw_private' variables in FdwPlan and FdwExecutionState.

* The comment in fileIterate seems wrong. It should be
/* Store the next tuple as a virtual tuple. */ , right?

* #include <sys/stat.h> is needed.

--
Itagaki Takahiro

On Wed, 16 Feb 2011 16:48:33 +0900
Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> wrote:

> On Tue, Feb 8, 2011 at 00:30, Shigeru HANADA <hanada(at)metrosystems(dot)co(dot)jp> wrote:
> > Fixed version is attached.
>
> I reviewed your latest git version, that is a bit newer than the attached patch.
> http://git.postgresql.org/gitweb?p=users/hanada/postgres.git;a=commit;h=0e1a1e1b0e168cb3d8ff4d637747d0ba8f7b8d55
>
> The code still works with small adjustment, but needs to be rebased on the
> latest master, especially for extension support and copy API changes.
>
> Here are a list of comments and suggestions:

Thanks for the comments. Revised version of patch has been attached.

> * You might forget some combination or unspecified options in
> file_fdw_validator().
> For example, format == NULL or !csv && header cases. I've not tested all
> cases, but please recheck validations used in BeginCopy().

Right, I've revised validation based on BeginCopy(), and added
regression tests about validation.

> * estimate_costs() needs own row estimation rather than using baserel.
> > > What value does baserel->tuples have?
> > > Foreign tables are never analyzed for now. Is the number correct?
> > No, baserel->tuples is always 0, and baserel->pages is 0 too.
> > In addition, width and rows are set to 100 and 1, as default values.
>
> It means baserel is not reliable at all, right?

Right, tables which has not been ANALYZEd have default stats in
baserel. But getting # of records needs another parsing for the file...

> If so, we need alternative
> solution in estimate_costs(). We adjust statistics with runtime relation
> size in estimate_rel_size(). Also, we use get_rel_data_width() for not
> analyzed tables. We could use similar technique in file_fdw, too.

Ah, using get_relation_data_width(), exported version of
get_rel_data_width(), seems to help estimation. I'll research around
it little more. By the way, adding ANALYZE support for foreign tables
is reasonable idea for this issue?

> * Should use int64 for file byte size (or, uint32 in blocks).
> unsigned long might be 32bit. ulong is not portable.
>
> * Oid List (list_make1_oid) might be more handy than Const to hold relid
> in FdwPlan.fdw_private.
>
> * I prefer FileFdwExecutionState to FileFdwPrivate, because there are
> two different 'fdw_private' variables in FdwPlan and FdwExecutionState.
>
> * The comment in fileIterate seems wrong. It should be
> /* Store the next tuple as a virtual tuple. */ , right?
>
> * #include <sys/stat.h> is needed.

Fixed all of above.

Regards,
--
Shigeru Hanada

Shigeru HANADA <hanada(at)metrosystems(dot)co(dot)jp> writes:
> [ 20110218-file_fdw.patch ]

I've adjusted this to fit the extensions infrastructure and the
committed version of the FDW API patch, and applied it.

>> * You might forget some combination or unspecified options in
>> file_fdw_validator().
>> For example, format == NULL or !csv && header cases. I've not tested all
>> cases, but please recheck validations used in BeginCopy().

> Right, I've revised validation based on BeginCopy(), and added
> regression tests about validation.

This approach struck me as entirely unmaintainable. I modified the core
COPY code to allow its option validation code to be called directly.

>> If so, we need alternative
>> solution in estimate_costs(). We adjust statistics with runtime relation
>> size in estimate_rel_size(). Also, we use get_rel_data_width() for not
>> analyzed tables. We could use similar technique in file_fdw, too.

> Ah, using get_relation_data_width(), exported version of
> get_rel_data_width(), seems to help estimation. I'll research around
> it little more. By the way, adding ANALYZE support for foreign tables
> is reasonable idea for this issue?

I did some quick hacking so that the numbers are at least a little bit
credible, but of course without ANALYZE support the qualification
selectivity estimates are likely to be pretty bogus. I am not sure
whether there's much of a use-case for supporting ANALYZE though.
I would think that if someone is going to read the same file in multiple
queries, they'd be far better off importing the data into a real table.
In any case, it's too late to worry about that for 9.1. I suggest
waiting to see what sort of real-world usage file_fdw gets before we
worry about whether it needs ANALYZE support.

regards, tom lane

Is this right?

postgres=# \d+ agg_text
Foreign table "public.agg_text"
Column | Type | Modifiers | Storage | Description
--------+----------+-----------+----------+-------------
a | smallint | | plain |
b | text | | extended |
Server: file_server
Has OIDs: no

It says the agg_text foreign table is using extended storage for the
text field. If it's in-file, how can it be classified as potentially
TOASTed?

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

Thom Brown <thom(at)linux(dot)com> writes:
> Is this right?
> postgres=# \d+ agg_text
> Foreign table "public.agg_text"
> Column | Type | Modifiers | Storage | Description
> --------+----------+-----------+----------+-------------
> a | smallint | | plain |
> b | text | | extended |
> Server: file_server
> Has OIDs: no

> It says the agg_text foreign table is using extended storage for the
> text field. If it's in-file, how can it be classified as potentially
> TOASTed?

It's meaningless, but I don't think it's worth trying to suppress the
meaningless column(s) ...

regards, tom lane