On Sun, 16 Jan 2011 01:55:19 +0100
Jan Urbański <wulczer@wulczer.org> wrote:
<snip>
> In general, the feature looks great and I hope it'll make it into 9.1.
> And it we'd get the possibility to write FDW handlers in other PLs than
> C, it would rock so hard...
>
> I'm going to mark this a Waiting for Author because of the typos, the
> BeginScan/EndScan issue, and the nested loop stopping halfway issue. The
> rest are suggestions or just thoughts, and if you don't see them as
> justified, I'll mark the next patch Ready for Committer.
Thanks a lot for the comments. I've (hopefully) fixed issues above.
Please find attached patches.
== patch list ==
1) 20110118-no_fdw_perm_check.patch - This patch is not included in
last post. This had been proposed on 2011-01-05 first, but maybe has
not been reviewd yet. I re-propose this patch for SQL standard
conformance. This patch removes permission check that requires USAGE
on the foreign-data wrapper at CREATE FOREIGN TABLE.
Please see original post for details.
http://archives.postgresql.org/message-id/20110105145206.30FD.6989961C@metrosystems.co.jp
2) 20110118-fdw_catalog_lookup.patch - This patch adds GetForeignTables.
Fixed lack of pg_foreign_table.h inclusion.
3) 20110118-fdw_handler.patch - This patch adds support for HANDLER
option to FOREIGN DATA WRAPPER object.
4) 20110118-foreign_scan.patch - This patch adds ForeignScan executor
node and FDW API hooks based on Heikki's proposal. As Itagaki-san
suggested on 2010-12-21, FDW must generate information for EXPLAIN
VERBOSE every PlanRelScan() call. It's better to avoid such overhead,
so new EXPLAIN hook would be needed. I'll try to make it cleaner.
================
And I'll reply to the rest of your comments.
> We could use comments about how to return tuples from Iterate and how to
> finish returning them. I had to look at the example to figure out that
> you need ExecClearTuple(slot) in your last Iterate. If Iterate's
> interface were to change, we could just return NULL instead of a tuple
> to say that we're done.
I've added some comments for FDW-developer to fdwapi.h, though they
wouldn't be enough.
> We could be a bit more explicit about how to allocate objects, for
> instance if I'm allocating a FdwPlan in my PlanRelScan with a palloc,
> will it not go away too soon, or too late (IOW leak)?
For that example, the answer is no. Objects are allocated in
MessageContext if you don't switch context and released when the query
has been finished. I agree that more documentation or comments for
FDW-developers should be added.
> Maybe PlanNative should get the foreign table OID, not the server OID,
> to resemble PlanRelScan more. The server OID is just a syscache lookup
> away, anyway.
You would missed a case that multiple foreign tables are used in a
query. Main purpose of PlanNative is to support pass-through
execution of remote query. In pass-through mode, you can use syntax
as if you have directly connected to external server, so can't use
PostgreSQL's parser.
> If you do EXPLAIN SELECT * FROM foregintable, BeginScan is not called,
> but EndScan is. That's weird, and I noticed because I got a segfault
> when EndScan tried to free things that BeginScan allocated. Maybe just
> don't call EndScan for EXPLAIN?
Fixed to not call EndScan if it was EXPLAIN execution.
Regards,
--
Shigeru Hanada
