On Tue, Dec 21, 2010 at 20:14, Shigeru HANADA <han...@metrosystems.co.jp> wrote: > Attached is the revised version of file_fdw patch. This patch is > based on Itagaki-san's copy_export-20101220.diff patch.
#1. Don't you have per-tuple memory leak? I added GetCopyExecutorState() because the caller needs to reset the per-tuple context periodically. Or, if you eventually make a HeapTuple from values and nulls arrays, you could modify NextCopyFrom() to return a HeapTuple instead of values, nulls, and tupleOid. The reason I didn't use HeapTuple is that I've seen arrays were used in the proposed FDW APIs. But we don't have to use such arrays if you use HeapTuple based APIs. IMHO, I prefer HeapTuple because we can simplify NextCopyFrom and keep EState private in copy.c. #2. Can you avoid making EXPLAIN text in fplan->explainInfo on non-EXPLAIN cases? It's a waste of CPU cycles in normal executions. I doubt whether FdwPlan.explainInfo field is the best design. How do we use the EXPLAIN text for XML or JSON explain formats? Instead, we could have an additional routine for EXPLAIN. #3. Why do you re-open a foreign table in estimate_costs() ? Since the caller seems to have the options for them, you can pass them directly, no? In addition, passing a half-initialized fplan to estimate_costs() is a bad idea. If you think it is an OUT parameter, the OUT params should be *startup_cost and *total_cost. #4. It'a minor cosmetic point, but our coding conventions would be we don't need (void *) when we cast a pointer to void *, but need (Type *) when we cast a void pointer to another type. -- Itagaki Takahiro -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
