Reply-To: jesper.pedersen@redhat.com
Subject: Re: [HACKERS] Runtime Partition Pruning
To: David Rowley <david.rowley@2ndquadrant.com>
Cc: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>,
 Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com>,
 Beena Emerson <memissemerson@gmail.com>,
 Alvaro Herrera <alvherre@alvh.no-ip.org>, Robert Haas
 <robertmhaas@gmail.com>, amul sul <sulamul@gmail.com>,
 "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>,
 Dilip Kumar <dilipbalaut@gmail.com>
References: 
 <CAKJS1f-zohKp_ANHYMe49nNje4vb81nF9s9EicK8KsjqQNkQ1A@mail.gmail.com>
 <CAKJS1f8rF+7WOu95WHH_pFu6NFh0iyF3oi+JZKGwaZbDXnJQ3A@mail.gmail.com>
 <CAKJS1f_bzRdbUutwc8iy5Bs5Krc9couEQK9NH1SUgMJvvnvWxg@mail.gmail.com>
 <31e27e19-eaf6-642f-8eb6-9cc7528fa57b@lab.ntt.co.jp>
 <CAKJS1f8+p-mXfFUiwR4xZ37STvgJeYF44yAjo5Rfxf92cFJyYQ@mail.gmail.com>
 <2dbe3de6-ad07-655f-1278-356709cbf185@lab.ntt.co.jp>
 <CAKJS1f_rPuzKvTg+uO95hepEoyx9zHe1rriC4zAO2z8ga7VgyA@mail.gmail.com>
 <CAKJS1f9xAKLew74v57zXBp4H5=g=V=TixbrnaYGLrquTD_5BRA@mail.gmail.com>
 <c3eb3284-24c8-eff0-f930-a33984d1168a@redhat.com>
 <CAKJS1f-A7wMdDZEyAQU=jY-AUgi1iGfmrmPXirV39sKdJrDu2A@mail.gmail.com>
 <95dec133-7654-ce27-a7af-c91b0e58ad27@redhat.com>
 <CAKJS1f8dq1BcvNEMTitp3O7Gc_cN1Dmn9vxxacb-kiX+mmyGSg@mail.gmail.com>
 <8658bbec-9afe-8246-c0ff-254768b1bbe7@lab.ntt.co.jp>
 <ae3375a4-be68-b101-36c3-1fa7af5dea10@lab.ntt.co.jp>
 <CAKJS1f8KDowMcMdff9+gr5pTzsrVucA+xeCH5ck05bAzCTXNMg@mail.gmail.com>
From: Jesper Pedersen <jesper.pedersen@redhat.com>
Organization: Red Hat
Message-ID: <8770beb8-12d4-e625-0ac1-a9021371c642@redhat.com>
Date: Thu, 5 Apr 2018 15:47:12 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.2
MIME-Version: 1.0
In-Reply-To: 
 <CAKJS1f8KDowMcMdff9+gr5pTzsrVucA+xeCH5ck05bAzCTXNMg@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi David,

First of all: Solid patch set with good documentation.

On 04/05/2018 09:41 AM, David Rowley wrote:
> Seems mostly fair. I'm not a fan of using the term "unpruned" though.
> I'll have a think.  The "all" is meant in terms of what exists as
> subnodes.
>

'included_parts' / 'excluded_parts' probably isn't better...

> subplan_indexes and parent_indexes seem like better names, I agree.
>

More clear.

>> * Also in make_partition_pruneinfo()
>>
>>      /* Initialize to -1 to indicate the rel was not found */
>>      for (i = 0; i < root->simple_rel_array_size; i++)
>>      {
>>          allsubnodeindex[i] = -1;
>>          allsubpartindex[i] = -1;
>>      }
>>
>> Maybe, allocate the arrays above mentioned using palloc0 and don't do this
>> initialization.  Instead make the indexes that are stored in these start
>> with 1 and consider 0 as invalid entries.
> 
> 0 is a valid subplan index. It is possible to make this happen, but
> I'd need to subtract 1 everywhere I used the map. That does not seem
> very nice. Seems more likely to result in bugs where we might forget
> to do the - 1.
> 
> Did you want this because you'd rather have the palloc0() than the for
> loop setting the array elements to -1? Or is there another reason?
>

I think that doing palloc0 would be confusing; -1 is more clear, 
especially since it is used with 'allpartindexes' which is a Bitmapset.

Doing the variable renames as Amit suggests would be good.

I ran some tests (v50_v20) (make check-world passes), w/ and w/o 
choose_custom_plan() being false, and seeing good performance results 
without running into issues.

Maybe 0005 should be expanded in partition_prune.sql with the supported 
cases to make those more clear.

Thanks !

Best regards,
  Jesper