On Wed, Feb 3, 2016 at 12:08 PM, Ashutosh Bapat <ashutosh.ba...@enterprisedb.com> wrote: > The patch implements your algorithm to deparse a query as described in > previous mail. The logic is largely coded in deparseFromExprForRel() and > foreign_join_ok(). The later one pulls up the clauses from joining relations > and first one deparses the FROM clause recursively.
Cool! + /* Add outer relation. */ + appendStringInfo(buf, "(%s", join_sql_o.data); + + /* Add join type */ + appendStringInfo(buf, " %s JOIN ", get_jointype_name(fpinfo->jointype)); + + /* Add inner relation */ + appendStringInfo(buf, "%s", join_sql_i.data); + + /* Append ON clause; ON (TRUE) in case empty join clause list */ + appendStringInfoString(buf, " ON "); Uh, couldn't that all be done as a single appendStringInfo? It seems a little tortured the way you're passing "relations" all the way down the callstack from deparseSelectStmtForRel, and at each level it might be NULL. If you made a rule that the caller MUST pass a StringInfo, then you could get rid of some conditional logic in deparseFromExprForRel. By the way, deparseSelectSql()'s header comment could use an update to mention this additional argument. Generally, it's helpful to say in each relevant function header comment something like "May be NULL" or "Must not be NULL" in cases like this to clarify the API contract. Similarly, I would be inclined to continue to require that deparseTargetList() have retrieved_attrs != NULL. If the caller doesn't want the value, it can pass a dummy variable and ignore the return value. This is of course slightly more cycles, but I think it's unlikely to matter, and making the code simpler would be good. + * Function is the entry point to deparse routines while constructing + * ForeignScan plan or estimating cost and size for ForeignPath. It is called + * recursively to build SELECT statements for joining relations of a pushed down + * foreign join. "This function is the entrypoint to the routines, either when constructing ForeignScan plan or when estimating" etc. + * tuple descriptor for the corresponding foreign scan. For a base relation, + * which is not part of a pushed down join, fpinfo->attrs_used can be used to + * construct SELECT clause, thus the function doesn't need tlist. Hence when + * tlist passed, the function assumes that it's constructing the SELECT + * statement to be part of a pushed down foreign join. I thought you got rid of that assumption. I think it should be gotten rid of, and then the comment can go too. If we're keeping the comment for some reason, should be "doesn't need the tlist" and when "when the tlist is passed". + * 1, since those are the attribute numbers are in the corresponding scan. Extra "are". Should be: "Those are the attribute numbers in the corresponding scan." Would it be nuts to set fdw_scan_tlist in all cases? Would the code come out simpler than what we have now? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
