RE: Planning counters in pg_stat_statements (using pgss_store)
От | imai.yoshikazu@fujitsu.com |
---|---|
Тема | RE: Planning counters in pg_stat_statements (using pgss_store) |
Дата | |
Msg-id | OSBPR01MB461661BDBBF8C5CB477AE47494FD0@OSBPR01MB4616.jpnprd01.prod.outlook.com обсуждение исходный текст |
Ответ на | Re: Planning counters in pg_stat_statements (using pgss_store) (Julien Rouhaud <rjuju123@gmail.com>) |
Ответы |
Re: Planning counters in pg_stat_statements (using pgss_store)
|
Список | pgsql-hackers |
On Thu, Mar 12, 2020 at 6:31 AM, Julien Rouhaud wrote: > On Thu, Mar 12, 2020 at 05:28:38AM +0000, imai.yoshikazu@fujitsu.com wrote: > > Hi Julien, > > > > On Mon, Mar 9, 2020 at 10:32 AM, Julien Rouhaud wrote: > > > On Thu, Mar 05, 2020 at 01:26:19PM -0700, legrand legrand wrote: > > > > Please consider PG13 shortest path ;o) > > > > > > > > My one is parse->queryId != UINT64CONST(0) in pgss_planner_hook(). > > > > It fixes IVM problem (verified), > > > > and keep CTAS equal to pgss without planning counters (verified too). > > > > > > I still disagree that hiding this problem is the right fix, but since no one > > > objected here's a v5 with that behavior. Hopefully this will be fixed in v14. > > > > Is there any case that query_text will be NULL when executing pg_plan_query? > > With current sources, there are no cases where the query text isn't provided > AFAICS. > > > If query_text will be NULL, we need to add codes to avoid errors in > > pgss_store like as current patch. If query_text will not be NULL, we should > > add Assert in pg_plan_query so that other developers can notice that they > > would not mistakenly set query_text as NULL even without using pgss_planning > > counter. > > I totally agree. I already had such assert locally, and regression tests pass > without any problem. I'm attaching a v6 with that extra assert. If the > first patch is committed, it'll now be a requirement to provide it. Or if > people think it's not, it'll make sure that we'll discuss it. I see. I also don't come up with any case of query_text is NULL. Now we need other people's opinion about here. I'll summary code review of this thread. [Performance] If track_planning is not enabled, performance will drop 0.2-0.6% which can be ignored. If track_planning is enabled, performance will drop 0-2.2%. 2.2% is a bit large but I think it is still acceptable because people using this feature might take account that some overhead will happen for additional calling of a gettime function. https://www.postgresql.org/message-id/CY4PR20MB12227E5CE199FFBB90C68A13BCB40%40CY4PR20MB1222.namprd20.prod.outlook.com [Values in each row] * Rows for planner time are added as {total/min/max/mean/stddev}_plan_time. These are enough statistics for users who want to investigate the planning time. * Rows for executor time are changed from {total/min/max/mean/stddev}_time to {total/min/max/mean/stddev}_exec_time. Because of changing the name of the rows, there's no backward compatibility. Thus some users needs to modify scripts which using previous version of the pg_stat_statements. I believe it is not expensive to rewrite scripts along this change and it would be better to give an appropriate name to a row for future users. I also haven't seen big opposition about losing backward compatibility so far. * We don't provide {total/min/max/mean/stddev}_time. Users can calculate total_time as total_plan_time + total_exec_time on their own. About {min/max/mean/stddev}_time, it will not make much sense because it is not ensured that executor follows planner and each counter value will be different largely between planner and executor. * bufusage still only counts the buffer usage during executor. Now we have the ability to count the buffer usage during planner but we keep the bufusage count the buffer usage during executor for now. [Coding] * We add Assert in pg_plan_query so that query_text will not be NULL when executing planner. There's no case query_text will be NULL with current sources. It is not ensured there will be any case query_text will be possibly NULL in the future though. Some considerations are needed by other people about this. I don't have any other comments for now. After looking patches over again and if there are no other comments about this patch, I'll set this patch as ready for committer for getting more opinion. -- Yoshikazu Imai
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Kyotaro HoriguchiДата:
Сообщение: Re: [PATCH] Skip llvm bytecode generation if LLVM is missing
Следующее
От: Dilip KumarДата:
Сообщение: Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager