Re: FDW for PostgreSQL

2012/11/15 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
> Hi Kaigai-san,
>
> Sorry for delayed response. I updated the patch, although I didn't change
> any about timing issue you and Fujita-san concern.
>
> 1) add some FDW options for cost estimation. Default behavior is not
> changed.
> 2) get rid of array of libpq option names, similary to recent change of
> dblink
> 3) enhance document, especially remote query optimization
> 4) rename to postgres_fdw, to avoid naming conflict with the validator which
> exists in core
> 5) cope with changes about error context handling
>
> On Tue, Nov 6, 2012 at 7:36 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>
>> Isn't it possible to pick-up only columns to be used in targetlist or
>> local qualifiers,
>> without modification of baserestrictinfo?
>
>
> IMO, it's possible. postgres_fdw doesn't modify baserestrictinfo at all; it
> just create two new lists which exclusively point RestrictInfo elements in
> baserestrictinfo. Pulling vars up from conditions which can't be pushed
> down would gives us list of necessary columns. Am I missing something?
>
Hanada-san,

Let me comments on several points randomly.

I also think the new "use_remote_explain" option is good. It works fine
when we try to use this fdw over the network with latency more or less.
It seems to me its default is "false", thus, GetForeignRelSize will return
the estimated cost according to ANALYZE, instead of remote EXPLAIN.
Even though you mention the default behavior was not changed, is it
an expected one? My preference is the current one, as is.

The deparseFuncExpr() still has case handling whether it is explicit case,
implicit cast or regular functions. If my previous proposition has no flaw,
could you fix up it using regular function invocation manner? In case when
remote node has incompatible implicit-cast definition, this logic can make
a problem.

At InitPostgresFdwOptions(), the source comment says we don't use
malloc() here for simplification of code. Hmm. I'm not sure why it is more
simple. It seems to me we have no reason why to avoid malloc here, even
though libpq options are acquired using malloc().

Regarding to the regression test.

[kaigai(at)iwashi sepgsql]$ cat contrib/postgres_fdw/regression.diffs
*** /home/kaigai/repo/sepgsql/contrib/postgres_fdw/expected/postgres_fdw.out
Sat Nov 17 21:31:19 2012
--- /home/kaigai/repo/sepgsql/contrib/postgres_fdw/results/postgres_fdw.out
Tue Nov 20 13:53:32 2012
***************
*** 621,627 ****
-- ===================================================================
ALTER FOREIGN TABLE ft1 ALTER COLUMN c5 TYPE int;
SELECT * FROM ft1 WHERE c1 = 1; -- ERROR
! ERROR: invalid input syntax for integer: "1970-01-02 00:00:00"
CONTEXT: column c5 of foreign table ft1
ALTER FOREIGN TABLE ft1 ALTER COLUMN c5 TYPE timestamp;
-- ===================================================================
--- 621,627 ----
-- ===================================================================
ALTER FOREIGN TABLE ft1 ALTER COLUMN c5 TYPE int;
SELECT * FROM ft1 WHERE c1 = 1; -- ERROR
! ERROR: invalid input syntax for integer: "Fri Jan 02 00:00:00 1970"
CONTEXT: column c5 of foreign table ft1
ALTER FOREIGN TABLE ft1 ALTER COLUMN c5 TYPE timestamp;
-- ===================================================================

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

I guess this test tries to check a case when remote column has incompatible
data type with local side. Please check timestamp_out(). Its output
format follows
"datestyle" setting of GUC, and it affects OS configuration on initdb time.
(Please grep "datestyle" at initdb.c !) I'd like to recommend to use
another data type
for this regression test to avoid false-positive detection.

Elsewhere, I could not find problems right now.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>