moving some partitioning code to executor

Started by Amit Langoteover 8 years ago7 messageshackers
Jump to latest
#1Amit Langote
Langote_Amit_f8@lab.ntt.co.jp

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]/messages/by-id/CA+Tgmoafr=hUrM=cbx-k=BDHOF2OfXa w95HQSNAK4mHBwmSjtw%40mail.gmail.com 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]/messages/by-id/7fe0007b-7ad1-a593-df11-ad05364ebce4@l ab.ntt.co.jp which is here to be applied
on HEAD before applying the main patch (0002) itself

Thanks,
Amit

[1]: /messages/by-id/CA+Tgmoafr=hUrM=cbx-k=BDHOF2OfXa w95HQSNAK4mHBwmSjtw%40mail.gmail.com
/messages/by-id/CA+Tgmoafr=hUrM=cbx-k=BDHOF2OfXa
w95HQSNAK4mHBwmSjtw%40mail.gmail.com

[2]: /messages/by-id/7fe0007b-7ad1-a593-df11-ad05364ebce4@l ab.ntt.co.jp
/messages/by-id/7fe0007b-7ad1-a593-df11-ad05364ebce4@l
ab.ntt.co.jp

Attachments:

0001-Make-RelationGetPartitionDispatch-expansion-order-de.patchtext/plain; charset=UTF-8; name=0001-Make-RelationGetPartitionDispatch-expansion-order-de.patchDownload+99-144
0002-Move-certain-partitioning-code-to-the-executor.patchtext/plain; charset=UTF-8; name=0002-Move-certain-partitioning-code-to-the-executor.patchDownload+698-676
#2Amit Langote
Langote_Amit_f8@lab.ntt.co.jp
In reply to: Amit Langote (#1)
Re: moving some partitioning code to executor

Repeating links for better accessibility:

On 2017/09/14 16:13, Amit Langote wrote:

[1]

/messages/by-id/CA+Tgmoafr=hUrM=cbx-k=BDHOF2OfXaw95HQSNAK4mHBwmSjtw@mail.gmail.com

[2]

/messages/by-id/7fe0007b-7ad1-a593-df11-ad05364ebce4@lab.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

#3Amit Langote
Langote_Amit_f8@lab.ntt.co.jp
In reply to: Amit Langote (#1)
Re: moving some partitioning code to executor

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]https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=77b6b5e9c, 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

Attachments:

0001-Move-certain-partitioning-code-to-the-executor.patchtext/plain; charset=UTF-8; name=0001-Move-certain-partitioning-code-to-the-executor.patchDownload+708-686
#4Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Amit Langote (#3)
Re: [HACKERS] moving some partitioning code to executor

Amit Langote wrote:

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

Please rebase once more -- 1aba8e651ac3 seems to have broken things
in this area pretty thoroughly.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#5Amit Langote
Langote_Amit_f8@lab.ntt.co.jp
In reply to: Alvaro Herrera (#4)
Re: [HACKERS] moving some partitioning code to executor

On 2017/11/15 2:09, Alvaro Herrera wrote:

Amit Langote wrote:

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

Please rebase once more -- 1aba8e651ac3 seems to have broken things
in this area pretty thoroughly.

Thanks, done.

Regards,
Amit

Attachments:

0001-Move-certain-partitioning-code-to-the-executor.patchtext/plain; charset=UTF-8; name=0001-Move-certain-partitioning-code-to-the-executor.patchDownload+717-696
#6Robert Haas
robertmhaas@gmail.com
In reply to: Amit Langote (#5)
Re: [HACKERS] moving some partitioning code to executor

On Tue, Nov 14, 2017 at 7:59 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

On 2017/11/15 2:09, Alvaro Herrera wrote:

Amit Langote wrote:

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

Please rebase once more -- 1aba8e651ac3 seems to have broken things
in this area pretty thoroughly.

Thanks, done.

Committed.

(Alvaro, hope that's not stepping your toes ...)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#7Amit Langote
Langote_Amit_f8@lab.ntt.co.jp
In reply to: Robert Haas (#6)
Re: [HACKERS] moving some partitioning code to executor

On 2017/11/16 0:29, Robert Haas wrote:

On Tue, Nov 14, 2017 at 7:59 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

On 2017/11/15 2:09, Alvaro Herrera wrote:

Amit Langote wrote:

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

Please rebase once more -- 1aba8e651ac3 seems to have broken things
in this area pretty thoroughly.

Thanks, done.

Committed.

Thank you.

Regards,
Amit