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

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.

Regards,

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