From: | torikoshia <torikoshia(at)oss(dot)nttdata(dot)com> |
---|---|
To: | Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> |
Cc: | Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, ikedamsh(at)oss(dot)nttdata(dot)com, atorik(at)gmail(dot)com, pgsql-hackers(at)postgresql(dot)org, legrand_legrand(at)hotmail(dot)com, tatsuro(dot)yamada(dot)tf(at)nttcom(dot)co(dot)jp |
Subject: | Re: Is it useful to record whether plans are generic or custom? |
Date: | 2020-07-20 04:57:42 |
Message-ID: | df9d3b221267336a760349e12e6dce5f@oss.nttdata.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 2020-07-20 11:57, Fujii Masao wrote:
> 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!
Thanks!
As I proposed earlier in this thread, I'm now trying to add information
about generic/cudstom plan to pg_stat_statements.
I'll share the idea and the poc patch soon.
Regards,
--
Atsushi Torikoshi
NTT DATA CORPORATION
From | Date | Subject | |
---|---|---|---|
Next Message | Thomas Munro | 2020-07-20 05:28:53 | Re: Comment simplehash/dynahash trade-offs |
Previous Message | Thomas Munro | 2020-07-20 04:57:34 | Re: psql - add SHOW_ALL_RESULTS option |