Re: WIP: Collecting statistics on CSV file data

(2011/12/12 19:33), Shigeru Hanada wrote:
> (2011/12/09 21:16), Etsuro Fujita wrote:
>> I updated the patch. Please find attached a patch.
>
> I've examined v5 patch, and got reasonable EXPLAIN results which reflect
> collected statistics! As increasing STATISTICS option, estimated rows
> become better. Please see attached stats_*.txt for what I
> tested.
>
> stats_none.txt : before ANALYZE
> stats_100.txt : SET STATISTICS = 100 for all columns, and ANALYZE
> stats_10000.txt : SET STATISTICS = 10000 for all columns, and ANALYZE
>
> I think that this patch become ready for committer after some
> minor corrections:
>
> Couldn't set n_distinct
> =======================
> I couldn't set n_distinct to columns of foreign tables. With some
> research, I noticed that ATSimplePermissions should accept
> ATT_FOREIGN_TABLE too for that case. In addition, regression tests for
> ALTER FOREIGN TABLE should be added to detect this kind of problem.
>
> Showing stats target
> ====================
> We can see stats target of ordinary tables with \d+, but it is not
> available for foreign tables. I think "Stats target" column should be
> added even though output of \d+ for foreign tables become wider. One
> possible idea is to remove useless "Storage" column instead, but views
> have that column even though their source could come from foreign tables.
>
> Please see attached patch for these two items.
>
> Comments of FdwRoutine
> ======================
> Mention about optional handler is obsolete. We should clearly say
> AnalyzeForeignTable is optional (can be set to NULL) and rest are
> required. IMO separating them with comment would help FDW authors to
> understand requirements, e.g.:
>
> typedef struct FdwRoutine
> {
> NodeTag type;
>
> /*
> * These Handlers are required to execute simple scan on a foreign
> * table. If any of them was set to NULL, scan on a foreign table
> * managed by such FDW would fail.
> */
> PlanForeignScan_function PlanForeignScan;
> ExplainForeignScan_function ExplainForeignScan;
> BeginForeignScan_function BeginForeignScan;
> IterateForeignScan_function IterateForeignScan;
> ReScanForeignScan_function ReScanForeignScan;
> EndForeignScan_function EndForeignScan;
>
> /*
> * Handlers below are optional. You can set any of them to
> * NULL to tell PostgreSQL that the FDW doesn't have the
> * capability.
> */
> AnalyzeForeignTable_function AnalyzeForeignTable;
> } FdwRoutine;
>
> Code formatting
> ===============
> Some code lines go past 80 columns.
>
> Message style
> =============
> The terms 'cannot support option "n_distinct"...' used in
> ATPrepSetOptions seems little unusual in PostgreSQL. Should we say
> 'cannot set "n_distinct_inherited" for foreign tables' for that case?
>
> Typo
> ====
> Typo "spcify" is in document of analyze.

Thank you for your effectiveness experiments and proposals for
improvements. I updated the patch according to your proposals.
Attached is the updated version of the patch.

Best regards,
Etsuro Fujita

> Regards,
>
>
>
>