From: | Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> |
---|---|
To: | Shigeru HANADA <hanada(at)metrosystems(dot)co(dot)jp> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: SQL/MED - file_fdw |
Date: | 2011-02-16 07:48:33 |
Message-ID: | AANLkTimX7xJgA3QCYAXf-OvaYvjb3OG5-y=x6NxAg1E7@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Lukas Eder | 2011-02-16 08:30:43 | Re: Fwd: [JDBC] Weird issues when reading UDT from stored function |
Previous Message | Simon Riggs | 2011-02-16 07:27:59 | Re: XMin Hot Standby Feedback patch |