(2012/03/09 1:18), Tom Lane wrote:
> I've been looking at this patch a little bit over the past day or so.
> I'm pretty unhappy with deparse.c --- it seems like a real kluge,
> inefficient and full of corner-case bugs. After some thought I believe
> that you're ultimately going to have to abandon depending on ruleutils.c
> for reverse-listing services, and it would be best to bite that bullet
> now and rewrite this code from scratch.
Thanks for the review. Agreed to write own depraser for pgsql_fdw
which handles nodes which can be pushed down. Every SQL-based FDW
which constructs SQL statement for each local query would need such
module inside.
BTW, pgsql_fdw pushes only built-in objects which have no collation
effect down to remote side, because user-defined objects might have
different semantics on remote end. In future, such deparser will
need some mechanism to map local object (or expression?) to remote
one, like ROUTINE MAPPING, as discussed before. But it seems ok to
assume that built-in objects have same name and semantics on remote
end.
> There are a couple of other points that make me think we need to revisit
> the PlanForeignScan API definition some more, too. First, deparse.c is
> far from cheap. While this doesn't matter greatly as long as there's
> only one possible path for a foreign table, as soon as you want to
> create more than one it's going to be annoying to do all that work N
> times and then throw away N-1 of the results.
Indeed deprase.c is not cheap, but I think that pgsql_fdw can avoid
redundant works by deparsing SQL statement separately, unless we need
to consider join-push-down. Possible parts are SELECT, FROM, WHERE
and ORDER BY clauses. Simplest path uses SELECT and FROM, and other
paths can be built by copying necessary clauses into individual
buffers.
Comments to the rest part are in my another reply to your recent post.
Regards,
--
Shigeru Hanada
