Horton

Re: Partition-wise join for join between (declaratively) partitioned tables

Robert Haas <robertmhaas@gmail.com>

View thread View on postgr.es
From: Robert Haas <robertmhaas@gmail.com>
To: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
Cc: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>, Rafia Sabih <rafia.sabih@enterprisedb.com>, Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2017-03-28T16:54:09Z
Lists: pgsql-hackers 
On Mon, Mar 27, 2017 at 8:36 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.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

Commits

  1. Basic partition-wise join functionality.

  2. Assorted preparatory refactoring for partition-wise join.

  3. Teach adjust_appendrel_attrs(_multilevel) to do multiple translations.

  4. Stamp 10beta2.

  5. Eat XIDs more efficiently in recovery TAP test.

  6. Abstract logic to allow for multiple kinds of child rels.

  7. Implement SortSupport for macaddr data type

  8. Attempt to stabilize grouping sets regression test plans.

  9. Teach xlogreader to follow timeline switches

  10. Don't scan partitioned tables.

  11. Fix grammar.

  12. postgres_fdw: Push down FULL JOINs with restriction clauses.

  13. Some preliminary refactoring towards partitionwise join.

  14. contrib/amcheck needs RecentGlobalXmin to be PGDLLIMPORT'ified.

  15. Print test parameters like "foo: 123", and results like "foo = 123".