Re: FDW for PostgreSQL

2012/11/21 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
> Thank for the comment!
>
> On Tue, Nov 20, 2012 at 10:23 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>
>> 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.
>
>
> Oops, I must have focused on only cost factors.
> I too think that using local statistics is better as default behavior,
> because foreign tables would be used for relatively stable tables.
> If target tables are updated often, it would cause problems about
> consistency, unless we provide full-fledged transaction mapping.
>
>>
>> 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.
>
>
> Sorry, I overlooked this issue. Fixed to use function call notation
> for all of explicit function calls, explicit casts, and implicit casts.
>
>> 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().
>
>
> I used "simple" because using palloc avoids null-check and error handling.
> However, many backend code use malloc to allocate memory which lives
> as long as backend process itself, so I fixed.
>
>>
>> Regarding to the regression test.
>
> [snip]
>>
>> 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.
>
>
> Good catch. :)
> I fixed the test to use another data type, user defined enum.
>
Hanada-san,

I checked the v4 patch, and I have nothing to comment anymore.

So, could you update the remaining EXPLAIN with VERBOSE option
stuff?

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