The patch compiles and make check-world doesn't show any failures. >> > > > I have tried it. Attached separate patch for it. > However I have noticed that istoplevel is always false (at-least for the > testcase we have, I get it false always). And I also think that taking > this decision only on PlanState is enough. Does that in attached patch. > To fix this, I have passed deparse_namespace to the private argument and > accessed it in get_special_variable(). Changes looks very simple. Please > have a look and let me know your views. > This issue is not introduced by the changes done for the aggregate push > down, it only got exposed as we now ship expressions in the target list. > Thus I think it will be good if we make these changes separately as new > patch, if required. >
The patch looks good and pretty simple.
+ * expression. However if underneath PlanState is ForeignScanState, then + * don't force parentheses. We need to explain why it's safe not to add paranthesis. The reason this function adds paranthesis so as to preserve any operator precedences imposed by the expression tree of which this IndexVar is part. For ForeignScanState, the Var may represent the root of the expression tree, and thus not need paranthesis. But we need to verify this and explain it in the comments.
As you have explained this is an issue exposed by this patch; something not must have in this patch. If committers feel that aggregate pushdown needs this fix as well, please provide a patch addressing the above comment.
Sure.
Ok. PFA patch with cosmetic changes (mostly rewording comments). Let me know if those look good. I am marking this patch is ready for committer.
Changes look good to me.
Thanks for the detailed review.
--
Jeevan B Chalke Principal Software Engineer, Product Development EnterpriseDB Corporation The Enterprise PostgreSQL Company
From:
Aleksander Alekseev Date: Subject:
File content logging during execution of COPY queries (was: Better
logging of COPY queries if log_statement='all')
