Re: [HACKERS] moving some partitioning code to executor

2017-09-25 Thread Amit Langote 
On 2017/09/14 16:13, Amit Langote wrote:
> Hi.
> 
> It seems to me that some of the code in partition.c is better placed
> somewhere under the executor directory.  There was even a suggestion
> recently [1] to introduce a execPartition.c to house some code around
> tuple-routing.
> 
> IMO, catalog/partition.c should present an interface for handling
> operations on a *single* partitioned table and avoid pretending to support
> any operations on the whole partition tree.  For example, the
> PartitionDispatch structure embeds some knowledge about the partition tree
> it's part of, which is useful when used for tuple-routing, because of the
> way it works now (lock and form ResultRelInfos of *all* leaf partitions
> before the first input row is processed).
> 
> So, let's move that structure, along with the code that creates and
> manipulates the same, out of partition.c/h and to execPartition.c/h.
> Attached patch attempts to do that.
> 
> While doing the same, I didn't move *all* of get_partition_for_tuple() out
> to execPartition.c, instead modified its signature as shown below:
> 
> -extern int get_partition_for_tuple(PartitionDispatch *pd,
> -TupleTableSlot *slot,
> -EState *estate,
> -PartitionDispatchData **failed_at,
> -TupleTableSlot **failed_slot);
> +extern int get_partition_for_tuple(Relation relation, Datum *values,
> +bool *isnull);
> 
> That way, we keep the core partition bound comparison logic inside
> partition.c and move rest of the stuff to its caller ExecFindPartition(),
> which includes navigating the enveloping PartitionDispatch's.
> 
> Thoughts?
> 
> PS: 0001 of the attached is the patch from [2] which is here to be applied
> on HEAD before applying the main patch (0002) itself

Since that 0001 patch was committed [1], here is the rebased patch.  Will
add this to the November commit-fest.

Thanks,
Amit

[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=77b6b5e9c
From 08b337436b5862b0ee593314864d6ba98f95e6c0 Mon Sep 17 00:00:00 2001
From: amit 
Date: Fri, 8 Sep 2017 19:07:38 +0900
Subject: [PATCH] Move certain partitioning code to the executor

---
 src/backend/catalog/partition.c| 438 +-
 src/backend/commands/copy.c|   1 +
 src/backend/executor/Makefile  |   2 +-
 src/backend/executor/execMain.c| 265 +---
 src/backend/executor/execPartition.c   | 559 +
 src/backend/executor/nodeModifyTable.c |   1 +
 src/include/catalog/partition.h|  48 +--
 src/include/executor/execPartition.h   |  65 
 src/include/executor/executor.h|  14 +-
 9 files changed, 708 insertions(+), 685 deletions(-)
 create mode 100644 src/backend/executor/execPartition.c
 create mode 100644 src/include/executor/execPartition.h

diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 1ab6dba7ae..903c8c4def 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -147,8 +147,6 @@ static int32 partition_bound_cmp(PartitionKey key,
 static int partition_bound_bsearch(PartitionKey key,
PartitionBoundInfo boundinfo,
void *probe, bool 
probe_is_bound, bool *is_equal);
-static void get_partition_dispatch_recurse(Relation rel, Relation parent,
-  List **pds, List 
**leaf_part_oids);
 
 /*
  * RelationBuildPartitionDesc
@@ -1193,148 +1191,6 @@ get_partition_qual_relid(Oid relid)
return result;
 }
 
-/*
- * RelationGetPartitionDispatchInfo
- * Returns information necessary to route tuples down a partition 
tree
- *
- * The number of elements in the returned array (that is, the number of
- * PartitionDispatch objects for the partitioned tables in the partition tree)
- * is returned in *num_parted and a list of the OIDs of all the leaf
- * partitions of rel is returned in *leaf_part_oids.
- *
- * All the relations in the partition tree (including 'rel') must have been
- * locked (using at least the AccessShareLock) by the caller.
- */
-PartitionDispatch *
-RelationGetPartitionDispatchInfo(Relation rel,
-int 
*num_parted, List **leaf_part_oids)
-{
-   List   *pdlist = NIL;
-   PartitionDispatchData **pd;
-   ListCell   *lc;
-   int i;
-
-   Assert(rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE);
-
-   *num_parted = 0;
-   *leaf_part_oids = NIL;
-
-   get_partition_dispatch_recurse(rel, NULL, , leaf_part_oids);
-   *num_parted = list_length(pdlist);
-   pd = (PartitionDispatchData **) palloc(*num_parted *
-

Re: [HACKERS] moving some partitioning code to executor

2017-09-14 Thread Amit Langote 
Repeating links for better accessibility:

On 2017/09/14 16:13, Amit Langote wrote:
> [1]

https://www.postgresql.org/message-id/CA%2BTgmoafr%3DhUrM%3Dcbx-k%3DBDHOF2OfXaw95HQSNAK4mHBwmSjtw%40mail.gmail.com

> [2]

https://www.postgresql.org/message-id/7fe0007b-7ad1-a593-df11-ad05364ebce4%40lab.ntt.co.jp

Thanks,
Amit



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] moving some partitioning code to executor

2017-09-14 Thread Amit Langote 
Hi.

It seems to me that some of the code in partition.c is better placed
somewhere under the executor directory.  There was even a suggestion
recently [1] to introduce a execPartition.c to house some code around
tuple-routing.

IMO, catalog/partition.c should present an interface for handling
operations on a *single* partitioned table and avoid pretending to support
any operations on the whole partition tree.  For example, the
PartitionDispatch structure embeds some knowledge about the partition tree
it's part of, which is useful when used for tuple-routing, because of the
way it works now (lock and form ResultRelInfos of *all* leaf partitions
before the first input row is processed).

So, let's move that structure, along with the code that creates and
manipulates the same, out of partition.c/h and to execPartition.c/h.
Attached patch attempts to do that.

While doing the same, I didn't move *all* of get_partition_for_tuple() out
to execPartition.c, instead modified its signature as shown below:

-extern int get_partition_for_tuple(PartitionDispatch *pd,
-TupleTableSlot *slot,
-EState *estate,
-PartitionDispatchData **failed_at,
-TupleTableSlot **failed_slot);
+extern int get_partition_for_tuple(Relation relation, Datum *values,
+bool *isnull);

That way, we keep the core partition bound comparison logic inside
partition.c and move rest of the stuff to its caller ExecFindPartition(),
which includes navigating the enveloping PartitionDispatch's.

Thoughts?

PS: 0001 of the attached is the patch from [2] which is here to be applied
on HEAD before applying the main patch (0002) itself

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/CA%2BTgmoafr%3DhUrM%3Dcbx-k%3DBDHOF2OfXa
w95HQSNAK4mHBwmSjtw%40mail.gmail.com

[2]
https://www.postgresql.org/message-id/7fe0007b-7ad1-a593-df11-ad05364ebce4%40l
ab.ntt.co.jp
From cb16fb6c89f60a8cf6b8f713ca9c831fe3824b2d Mon Sep 17 00:00:00 2001
From: amit 
Date: Fri, 8 Sep 2017 17:35:10 +0900
Subject: [PATCH 1/2] Make RelationGetPartitionDispatch expansion order
 depth-first

This is so as it matches what the planner is doing with partitioning
inheritance expansion.  Matching with planner order helps because
it helps ease matching the executor's per-partition objects with
planner-created per-partition nodes.
---
 src/backend/catalog/partition.c | 242 
 1 file changed, 99 insertions(+), 143 deletions(-)

diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 73eff17202..ddb46a80cb 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -147,6 +147,8 @@ static int32 partition_bound_cmp(PartitionKey key,
 static int partition_bound_bsearch(PartitionKey key,
PartitionBoundInfo boundinfo,
void *probe, bool 
probe_is_bound, bool *is_equal);
+static void get_partition_dispatch_recurse(Relation rel, Relation parent,
+  List **pds, List 
**leaf_part_oids);
 
 /*
  * RelationBuildPartitionDesc
@@ -1192,21 +1194,6 @@ get_partition_qual_relid(Oid relid)
 }
 
 /*
- * Append OIDs of rel's partitions to the list 'partoids' and for each OID,
- * append pointer rel to the list 'parents'.
- */
-#define APPEND_REL_PARTITION_OIDS(rel, partoids, parents) \
-   do\
-   {\
-   int i;\
-   for (i = 0; i < (rel)->rd_partdesc->nparts; i++)\
-   {\
-   (partoids) = lappend_oid((partoids), 
(rel)->rd_partdesc->oids[i]);\
-   (parents) = lappend((parents), (rel));\
-   }\
-   } while(0)
-
-/*
  * RelationGetPartitionDispatchInfo
  * Returns information necessary to route tuples down a partition 
tree
  *
@@ -1222,151 +1209,120 @@ PartitionDispatch *
 RelationGetPartitionDispatchInfo(Relation rel,
 int 
*num_parted, List **leaf_part_oids)
 {
+   List   *pdlist;
PartitionDispatchData **pd;
-   List   *all_parts = NIL,
-  *all_parents = NIL,
-  *parted_rels,
-  *parted_rel_parents;
-   ListCell   *lc1,
-  *lc2;
-   int i,
-   k,
-   offset;
+   ListCell *lc;
+   int i;
 
-   /*
-* We rely on the relcache to traverse the partition tree to build both
-* the leaf partition OIDs list and the array of PartitionDispatch 
objects
-* for the partitioned tables in the tree.  That means every partitioned
-* table in the tree must be locked, which is fine since we require the
-