On 2016/11/22 18:28, Ashutosh Bapat wrote:
> The comments should explain why is the assertion true.
> + /* Shouldn't be NIL */
> + Assert(tlist != NIL);
I noticed that I was wrong; in the Assertion the tlist can be empty. An
example for such a case is:
SELECT 1 FROM (SELECT c1 FROM ft4 WHERE c1 between 50 and 60) t1 FULL
JOIN (SELECT c1 FROM ft5 WHERE c1 between 50 and 60) t2 ON (TRUE);
In this example any columns of the relations ft4 and ft5 wouldn't be
needed for the join or the final output, so both the tlists for the
relations created in deparseRangeTblRef would be empty. Attached is an
updated version fixing this bug. Changes are:
* I removed make_outerrel_subquery/make_innerrel_subquery from fpinfo
and added a new member "is_subquery_rel" to fpinfo, as a flag on whether
to deparse the relation as a subquery.
* I modified deparseRangeTblRef, deparseSelectSql, and is_subquery_var
to see the is_subquery_rel, not
make_outerrel_subquery/make_innerrel_subquery.
* I modified appendSubselectAlias to handle the case where ncols = 0
properly.
* I renamed subquery_rels in fpinfo to lower_subquery_rels, to make it
easy to distinguish this from is_subquery_rel clearly.
* I added regression tests for that.
The rest of the patch is the same as the previous version, but:
> + /* Should be same length */
> + Assert(list_length(tlist) ==
> list_length(foreignrel->reltarget->exprs));
I added comments explaining the reason.
> The name get_subselect_alias_id() seems to suggest that the function returns
> identifier for subselect alias, which isn't correct. It returns the alias
> identifiers for deparsing a Var node. But I guess, we have left this to the
> committer's judgement.
I agree that the name isn't good, so I changed it to
get_relation_column_alias_ids(). Let me know if it may be better.
I also revised code and comments a bit, just for consistency.
I will update another patch for PHVs on top of the attached one.
Best regards,
Etsuro Fujita