From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com> |
Cc: | Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>, Rafia Sabih <rafia(dot)sabih(at)enterprisedb(dot)com>, Rajkumar Raghuwanshi <rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Partition-wise join for join between (declaratively) partitioned tables |
Date: | 2017-03-28 16:54:09 |
Message-ID: | CA+TgmoagTnF2yqR3PT2rv=om=wJiZ4-A+ATwdnriTGku1CLYxA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Mon, Mar 27, 2017 at 8:36 AM, Ashutosh Bapat
<ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
> I have gone through the patch, and it looks good to me. Here's the set
> of patches with this patch included. Fixed the testcase failures.
> Rebased the patchset on de4da168d57de812bb30d359394b7913635d21a9.
This version of 0001 looks much better to me, but I still have some concerns.
I think we should also introduce IS_UPPER_REL() at the same time, for
symmetry and because partitionwise aggregate will need it, and use it
in place of direct tests against RELOPT_UPPER_REL.
I think it would make sense to change the test in deparseFromExpr() to
check for IS_JOIN_REL() || IS_SIMPLE_REL(). There's no obvious reason
why that shouldn't be OK, and it would remove the last direct test
against RELOPT_JOINREL in the tree, and it will probably need to be
changed for partitionwise aggregate anyway.
Could set_append_rel_size Assert(IS_SIMPLE_REL(rel))? I notice that
you did this in some other places such as
generate_implied_equalities_for_column(), and I like that. If for
some reason that's not going to work, then it's doubtful whether
Assert(childrel->reloptkind == RELOPT_OTHER_MEMBER_REL) is going to
survive either.
Similarly, I think relation_excluded_by_constraints() would also
benefit from Assert(IS_SIMPLE_REL(rel)).
Why not set top_parent_relids earlier, when actually creating the
RelOptInfo? I think you could just change build_simple_rel() so that
instead of passing RelOptKind reloptkind, you instead pass RelOptInfo
*parent. I think postponing that work until set_append_rel_size()
just introduces possible bugs resulting from it not being set early
enough.
Apart from the above, I think 0001 is in good shape.
Regarding 0002, I think the parts that involve factoring out
find_param_path_info() are uncontroversial. Regarding the changes to
adjust_appendrel_attrs(), my main question is whether we wouldn't be
better off using an array representation rather than a List
representation. In other words, this function could take PlannerInfo
*root, Node *node, int nappinfos, AppendRelInfo **appinfos. Existing
callers doing adjust_appendrel_attrs(root, whatever, appinfo) could
just do adjust_appendrel_attrs(root, whatever, 1, &appinfo), not
needing to allocate. To make this work, adjust_child_relids() and
find_appinfos_by_relids() would need to be adjusted to use a similar
argument-passing convention. I suspect this makes iterating over the
AppendRelInfos mildly faster, too, apart from the memory savings.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
From | Date | Subject | |
---|---|---|---|
Next Message | Robert Haas | 2017-03-28 16:55:36 | Re: Monitoring roles patch |
Previous Message | Paul Jungwirth | 2017-03-28 16:47:40 | Postgres Permissions Article |