Re: [HACKERS] Runtime Partition Pruning

On Tue, Dec 12, 2017 at 4:57 PM, Beena Emerson <memissemerson(at)gmail(dot)com> wrote:
> Hello Robert,
> Thank you for the comments. I have started working on it.
>
> On Fri, Dec 8, 2017 at 9:27 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Thu, Dec 7, 2017 at 2:22 AM, Beena Emerson <memissemerson(at)gmail(dot)com> wrote:
>>> I have added the partition quals that are used for pruning.
>>>
>>> PFA the updated patch. I have changed the names of variables to make
>>> it more appropriate, along with adding more code comments and doing
>>> some refactoring and other code cleanups.
>>
>> - initClauses() seems to be a duplicate of the existing function
>> ExecInitExprList(), except for the extra NULL test, which isn't
>> necessary.
>
> The initClauses has been removed and ExecInitExprList has been used.
>
>> - The executor already has a system for making sure that relations get
>> opened and locked, and it's different from the new one scheme which
>> set_append_subplan_indexes() implements. Relations should be locked
>> during the executor initialization phase (i.e. ExecInitAppend) and not
>> when the first tuple is requested (i.e. ExecAppend). Also, there's
>> already code to lock both child relations (i.e. the scans of those
>> relations, see InitScanRelation, ExecInitIndexScan) and non-leaf
>> partitions (ExecLockNonLeafAppendTables). The call to
>> find_all_inheritors() will lock all of that same stuff again *plus*
>> the leaf partitions that were pruned during planning - moreover, if
>> the Append is rescanned, we'll walk the partitioning structure again
>> for every rescan. I think RelationGetPartitionDispatchInfo should be
>> called directly from ExecInitAppend after the existing code to take
>> locks has been called, and store a pointer to the PartitionDispatch
>> object in the AppendState for future use.
>
> I have moved the call to ExecInitAppend. This still uses the previous
> locking method, I will work on it in the next version of the patch.
>
>
>> - I am surprised that set_append_subplan_indexes() needs to worry
>> about multi-level partitioning directly. I would have thought that
>> Amit's patch would take care of that, just returning a Bitmapset of
>> indexes which this function could use directly. It also doesn't seem
>> like a very good idea to convert the Bitmapset (subplans) into a list
>> of integers (node->subplan_indexes), as set_append_subplan_indexes()
>> does at the bottom. The Bitmapset will be a lot more efficient; we
>> should be able to just iterate over that directly rather than
>> converting it into a List. Note that a Bitmapset can be created with
>> a single palloc, but an integer list needs one per list element plus
>> one for the list itself.
>
> The function get_partitions_from_clauses returns the Bitmap set of
> partitions for a level of partition. So when the BitmapSet that
> indicates a child partitioned table, set_append_subplan_indexes loops
> throgh again till it gets the list of all leaf indexes.
>
> I am working on the other comments and will post the patch along with
> rebasing to v14 of Amit's patch soon.
>
>
> --
>

PFA the updated patch, this can be applied over the v13 patches [1]
over commit 487a0c1518af2f3ae2d05b7fd23d636d687f28f3

[1] https://www.postgresql.org/message-id/df609168-b7fd-4c0b-e9b2-6e398d411e27%40lab.ntt.co.jp

--

Beena Emerson

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