Re: Is it useful to record whether plans are generic or custom?

On 2020-07-15 11:44, Fujii Masao wrote:
> On 2020/07/14 21:24, torikoshia wrote:
>> On 2020-07-10 10:49, torikoshia wrote:
>>> On 2020-07-08 16:41, Fujii Masao wrote:
>>>> On 2020/07/08 10:14, torikoshia wrote:
>>>>> On 2020-07-06 22:16, Fujii Masao wrote:
>>>>>> On 2020/06/11 14:59, torikoshia wrote:
>>>>>>> On 2020-06-10 18:00, Kyotaro Horiguchi wrote:
>>>>>>> 
>>>>>>>> 
>>>>>>>> +    TupleDescInitEntry(tupdesc, (AttrNumber) 8, "last_plan",
>>>>>>>> 
>>>>>>>> This could be a problem if we showed the last plan in this view. 
>>>>>>>> I
>>>>>>>> think "last_plan_type" would be better.
>>>>>>>> 
>>>>>>>> +            if (prep_stmt->plansource->last_plan_type == 
>>>>>>>> PLAN_CACHE_TYPE_CUSTOM)
>>>>>>>> +                values[7] = CStringGetTextDatum("custom");
>>>>>>>> +            else if (prep_stmt->plansource->last_plan_type == 
>>>>>>>> PLAN_CACHE_TYPE_GENERIC)
>>>>>>>> +                values[7] = CStringGetTextDatum("generic");
>>>>>>>> +            else
>>>>>>>> +                nulls[7] = true;
>>>>>>>> 
>>>>>>>> Using swith-case prevents future additional type (if any) from 
>>>>>>>> being
>>>>>>>> unhandled.  I think we are recommending that as a convension.
>>>>>>> 
>>>>>>> Thanks for your reviewing!
>>>>>>> 
>>>>>>> I've attached a patch that reflects your comments.
>>>>>> 
>>>>>> Thanks for the patch! Here are the comments.
>>>>> 
>>>>> Thanks for your review!
>>>>> 
>>>>>> +        Number of times generic plan was choosen
>>>>>> +        Number of times custom plan was choosen
>>>>>> 
>>>>>> Typo: "choosen" should be "chosen"?
>>>>> 
>>>>> Thanks, fixed them.
>>>>> 
>>>>>> +      <entry role="catalog_table_entry"><para 
>>>>>> role="column_definition">
>>>>>> +       <structfield>last_plan_type</structfield> 
>>>>>> <type>text</type>
>>>>>> +      </para>
>>>>>> +      <para>
>>>>>> +        Tells the last plan type was generic or custom. If the 
>>>>>> prepared
>>>>>> +        statement has not executed yet, this field is null
>>>>>> +      </para></entry>
>>>>>> 
>>>>>> Could you tell me how this information is expected to be used?
>>>>>> I think that generic_plans and custom_plans are useful when 
>>>>>> investigating
>>>>>> the cause of performance drop by cached plan mode. But I failed to 
>>>>>> get
>>>>>> how much useful last_plan_type is.
>>>>> 
>>>>> This may be an exceptional case, but I once had a case needed
>>>>> to ensure whether generic or custom plan was chosen for specific
>>>>> queries in a development environment.
>>>> 
>>>> In your case, probably you had to ensure that the last multiple (or 
>>>> every)
>>>> executions chose generic or custom plan? If yes, I'm afraid that 
>>>> displaying
>>>> only the last plan mode is not enough for your case. No?
>>>> So it seems better to check generic_plans or custom_plans columns in 
>>>> the
>>>> view rather than last_plan_type even in your case. Thought?
>>> 
>>> Yeah, I now feel last_plan is not so necessary and only the numbers 
>>> of
>>> generic/custom plan is enough.
>>> 
>>> If there are no objections, I'm going to remove this column and 
>>> related codes.
>> 
>> As mentioned, I removed last_plan column.
> 
> Thanks for updating the patch! It basically looks good to me.
> 
> I have one comment; you added the regression tests for generic and
> custom plans into prepare.sql. But the similar tests already exist in
> plancache.sql. So isn't it better to add the tests for generic_plans 
> and
> custom_plans columns, into plancache.sql?


Thanks for your comments!

Agreed.
I removed tests on prepare.sql and added them to plancache.sql.
Thoughts?


Regards,


--
Atsushi Torikoshi
NTT DATA CORPORATION