From: | Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> |
---|---|
To: | torikoshia <torikoshia(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 02:57:39 |
Message-ID: | 129588a9-9d31-1fa7-2486-6f7f47dcf4c8@oss.nttdata.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Kapila | 2020-07-20 04:42:22 | Re: pg_ctl behavior on Windows |
Previous Message | Amit Kapila | 2020-07-20 01:46:08 | Re: Patch for nodeIncrementalSort comment correction. |