On Thu, Mar 29, 2018 at 12:55 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> I think that is under acceptable range. I am seeing few regression
> failures with the patch series. The order of targetlist seems to have
> changed for Remote SQL. Kindly find the failure report attached. I
> have requested my colleague Ashutosh Sharma to cross-verify this and
> he is also seeing the same failures.
Oops. Those just require an expected output change.
> It seems UPPERREL_TLIST is redundant in the patch now. I think we can
> remove it unless you have something else in mind.
Yes.
> I think the handling of partitioned rels looks okay, but we might want
> to once check the overhead of the same unless you are sure that this
> shouldn't be a problem. If you think, we should check it once, then
> is it possible that we can do it as a separate patch as this doesn't
> look to be directly linked to the main patch. It can be treated as an
> optimization for partitionwise aggregates. I think we can treat it
> along with the main patch as well, but it might be somewhat simpler to
> verify it if we do it separately.
I don't think it should be a problem, although you're welcome to test
it if you're concerned about it. I think it would probably be
penny-wise and pound-foolish to worry about the overhead of
eliminating the Result nodes, which can occur not only with
partition-wise aggregate but also with partition-wise join and, I
think, really any case where the top scan/join plan would be an Append
node. We're talking about a very small amount of additional planning
time to potentially get a better plan.
I've committed all of these now.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
