Message-ID: <4F3E06DA.5060108@lab.ntt.co.jp>
Date: Fri, 17 Feb 2012 16:50:50 +0900
From: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; ja;
	rv:1.9.2.9) Gecko/20100915 Thunderbird/3.1.4
MIME-Version: 1.0
To: Shigeru Hanada <shigeru.hanada@gmail.com>
CC: pgsql-hackers@postgresql.org
Subject: Re: WIP: Collecting statistics on CSV file data
References: <4E6DE18B.4010505@lab.ntt.co.jp> <4E7861C1.2000607@gmail.com>
	<4E8EC1D8.3080700@lab.ntt.co.jp> <4E9C6573.8050507@gmail.com>
	<4E9FF04B.3050504@lab.ntt.co.jp> <4EB7C07A.5030205@gmail.com>
	<4EC60883.2050905@lab.ntt.co.jp> <4EC648CA.5050904@gmail.com>
	<4EC9CBC5.3070500@lab.ntt.co.jp>
	<4EE1FC38.5080707@lab.ntt.co.jp> <4EE5D862.1080809@gmail.com>
	<4EE74C5F.3000309@lab.ntt.co.jp> <4EE84360.90800@gmail.com>
	<4EE95BC7.9000101@lab.ntt.co.jp> <4F351626.4030500@gmail.com>
In-Reply-To: <4F351626.4030500@gmail.com>
Content-Type: text/plain; charset=ISO-2022-JP
Content-Transfer-Encoding: 7bit

Hi Hanada-san,

Sorry for the late response.

(2012/02/10 22:05), Shigeru Hanada wrote:
> (2011/12/15 11:30), Etsuro Fujita wrote:
>> (2011/12/14 15:34), Shigeru Hanada wrote:
>>> I think this patch could be marked as "Ready for committer" with some
>>> minor fixes.  Please find attached a revised patch (v6.1).
> 
> I've tried to make pgsql_fdw work with this feature, and found that few
> static functions to be needed to exported to implement ANALYZE handler
> in short-cut style.  The "Short-cut style" means the way to generate
> statistics (pg_class and pg_statistic) for foreign tables without
> retrieving sample data from foreign server.

That's great!  Here is my review.

The patch applies with some modifications and compiles cleanly.  But
regression tests on subqueries failed in addition to role related tests
as discussed earlier.

While I've not looked at the patch in detail, I have some comments:

1. The patch might need codes to handle the irregular case where
ANALYZE-related catalog data such as attstattarget are different between
the local and the remote. (Although we don't have the options to set
such a data on a foreign table in ALTER FOREIGN TABLE.)  For example,
while attstattarget = -1 for some column on the local, attstattarget = 0
for that column on the remote meaning that there can be no stats
available for that column.  In such a case it would be better to inform
the user of it.

2. It might be better for the FDW to estimate the costs of a remote
query for itself without doing EXPLAIN if stats were available using
this feature.  While this approach is less accurate compared to the
EXPLAIN approach due to the lack of information such as seq_page_cost or
randam_page_cost on the remote, it is cheaper!  I think such a
information may be added to generic options for a foreign table, which
may have been previously discussed.

3.
> In implementing ANALYZE handler, hardest part was copying anyarray
> values from remote to local.  If we can make it common in core, it would
> help FDW authors who want to implement ANALYZE handler without
> retrieving sample rows from remote server.

+1 from me.

Best regards,
Etsuro Fujita