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

On 2020/07/17 16:25, Fujii Masao wrote:
> 
> 
> On 2020/07/16 11:50, torikoshia wrote:
>> 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?
> 
> Thanks for updating the patch!
> I also applied the following minor changes to the patch.
> 
> -        Number of times generic plan was chosen
> +       Number of times generic plan was chosen
> -        Number of times custom plan was chosen
> +       Number of times custom plan was chosen
> 
> I got rid of one space character before those descriptions because
> they should start from the position of 7th character.
> 
>   -- but we can force a custom plan
>   set plan_cache_mode to force_custom_plan;
>   explain (costs off) execute test_mode_pp(2);
> +select name, generic_plans, custom_plans from pg_prepared_statements
> +  where  name = 'test_mode_pp';
> 
> In the regression test, I added the execution of pg_prepared_statements
> after the last execution of test query, to confirm that custom plan is used
> when force_custom_plan is set, by checking from pg_prepared_statements.
> 
> I changed the status of this patch to "Ready for Committer" in CF.
> 
> Barring any objection, I will commit this patch.

Committed. Thanks!

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION