On Wed, Nov 20, 2019 at 4:55 PM, Julien Rouhaud wrote:
> On Wed, Nov 20, 2019 at 2:06 AM [hidden email] <[hidden email]> wrote: > > > > On Tue, Nov 19, 2019 at 2:27 PM, Julien Rouhaud wrote: > > > On Fri, Nov 15, 2019 at 2:00 AM [hidden email] <[hidden email]> wrote: > > > > > > > > Actually I also don't have strong opinion but I thought someone > > > > would complain about renaming of those columns and > > > also some tools like monitoring which use those columns will not > > > work. If we use {total, min, max, mean, stddev}_time, someone might > > > mistakenly understand {total, min, max, mean, stddev}_time mean {total, min, max, mean, stddev} of planning and > execution. > > > > If I need to choose {total, min, max, mean, stddev}_time or > > > > {total, min, max, mean, stddev}_exec_time, I choose former > > > one because choosing best name is not worth destructing the existing scripts or tools. > > > > > > We could definitely keep (plan|exec)_time for the SRF, and have the > > > {total, min, max, mean, stddev}_time created by the view to be a sum > > > of planning + execution for each counter > > > > I might misunderstand but if we define {total, min, max, mean, > > stddev}_time is just a sum of planning + execution for each counter > > like "select total_plan_time + total_exec_time as total_time from > > pg_stat_statements", I wonder we can calculate stddev_time correctly. > > If we prepare variables in the codes to calculate those values, yes, > > we can correctly calculate those values even for the total_stddev. > > Yes you're right, this can't possibly work for most of the counters. > And also, since there's no guarantee that each execution will follow a planning, providing such global counters for > min/max/mean and stddev wouldn't make much sense. Ah, I see. Planning counts and execution counts differ. It might be difficult to redefine the meaning of {min, max, mean, stddev}_time precisely, and even if we can redefine it correctly, it would not be intuitive. -- Yoshikazu Imai |
On Fri, Nov 22, 2019 at 11:23 AM [hidden email]
<[hidden email]> wrote: > > On Wed, Nov 20, 2019 at 4:55 PM, Julien Rouhaud wrote: > > On Wed, Nov 20, 2019 at 2:06 AM [hidden email] <[hidden email]> wrote: > > > > > > On Tue, Nov 19, 2019 at 2:27 PM, Julien Rouhaud wrote: > > > > On Fri, Nov 15, 2019 at 2:00 AM [hidden email] <[hidden email]> wrote: > > > > > > > > > > Actually I also don't have strong opinion but I thought someone > > > > > would complain about renaming of those columns and > > > > also some tools like monitoring which use those columns will not > > > > work. If we use {total, min, max, mean, stddev}_time, someone might > > > > mistakenly understand {total, min, max, mean, stddev}_time mean {total, min, max, mean, stddev} of planning and > > execution. > > > > > If I need to choose {total, min, max, mean, stddev}_time or > > > > > {total, min, max, mean, stddev}_exec_time, I choose former > > > > one because choosing best name is not worth destructing the existing scripts or tools. > > > > > > > > We could definitely keep (plan|exec)_time for the SRF, and have the > > > > {total, min, max, mean, stddev}_time created by the view to be a sum > > > > of planning + execution for each counter > > > > > > I might misunderstand but if we define {total, min, max, mean, > > > stddev}_time is just a sum of planning + execution for each counter > > > like "select total_plan_time + total_exec_time as total_time from > > > pg_stat_statements", I wonder we can calculate stddev_time correctly. > > > If we prepare variables in the codes to calculate those values, yes, > > > we can correctly calculate those values even for the total_stddev. > > > > Yes you're right, this can't possibly work for most of the counters. > > And also, since there's no guarantee that each execution will follow a planning, providing such global counters for > > min/max/mean and stddev wouldn't make much sense. > > Ah, I see. Planning counts and execution counts differ. > It might be difficult to redefine the meaning of {min, max, mean, stddev}_time precisely, and even if we can redefine it correctly, it would not be intuitive. broken since 3fd40b628c7db4, which removed the queryString from ExecCreateTableAs. New patch version that re-add the queryString, no changes otherwise. ![]() ![]() |
Hi Julien,
I would like to create a link with https://www.postgresql.org/message-id/1577490124579-0.post@... where we met an ASSET FAILURE because query text was not initialized ... The question raised is: - should query text be always provided or - if not how to deal that case (in pgss). Regards PAscal -- Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html |
On Sun, Jan 5, 2020 at 4:11 PM legrand legrand
<[hidden email]> wrote: > > Hi Julien, > > I would like to create a link with > https://www.postgresql.org/message-id/1577490124579-0.post@... > > where we met an ASSET FAILURE because query text was not initialized ... > > The question raised is: > > - should query text be always provided > or > - if not how to deal that case (in pgss). I'd think that since the query text was until now always provided, there's no reason why this patch should change that. That being said, there has been other concerns raised wrt. temporary tables in the IVM patchset, so ISTM that there might be important architectural changes upcoming, so having to deal with this case in pgss is not rushed (especially since handling that in pgss would be trivial), and can help to catch issue with the query text pasing. |
Julien Rouhaud wrote
> On Sun, Jan 5, 2020 at 4:11 PM legrand legrand > < > legrand_legrand@ > > wrote: >> >> Hi Julien, >> >> I would like to create a link with >> https://www.postgresql.org/message-id/ > 1577490124579-0.post@.nabble >> >> where we met an ASSET FAILURE because query text was not initialized ... >> >> The question raised is: >> >> - should query text be always provided >> or >> - if not how to deal that case (in pgss). > > I'd think that since the query text was until now always provided, > there's no reason why this patch should change that. That being said, > there has been other concerns raised wrt. temporary tables in the IVM > patchset, so ISTM that there might be important architectural changes > upcoming, so having to deal with this case in pgss is not rushed > (especially since handling that in pgss would be trivial), and can > help to catch issue with the query text pasing. IVM revealed that ASSERT, but IVM works fine with pg_stat_statements.track_planning = off. There may be others parts of postgresql that would have workede fine as well. This means 2 things: - there is a (litle) risk to meet other assert failures when using planning counters in pgss, - we have an easy workarround to fix it (disabling track_planning). But I would have prefered this new feature to work the same way with or without track_planning activated ;o( -- Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html |
On Sun, Jan 5, 2020 at 7:02 PM legrand legrand
<[hidden email]> wrote: > > Julien Rouhaud wrote > > On Sun, Jan 5, 2020 at 4:11 PM legrand legrand > > < > > > legrand_legrand@ > > > > wrote: > >> > >> Hi Julien, > >> > >> I would like to create a link with > >> https://www.postgresql.org/message-id/ > > > 1577490124579-0.post@.nabble > > >> > >> where we met an ASSET FAILURE because query text was not initialized ... > >> > >> The question raised is: > >> > >> - should query text be always provided > >> or > >> - if not how to deal that case (in pgss). > > > > I'd think that since the query text was until now always provided, > > there's no reason why this patch should change that. That being said, > > there has been other concerns raised wrt. temporary tables in the IVM > > patchset, so ISTM that there might be important architectural changes > > upcoming, so having to deal with this case in pgss is not rushed > > (especially since handling that in pgss would be trivial), and can > > help to catch issue with the query text pasing. > > IVM revealed that ASSERT, > but IVM works fine with pg_stat_statements.track_planning = off. Yes, but on the other hand the current IVM patchset also adds the only pg_plan_query call that don't provide a query text. I didn't see any other possibility, and if there are other cases they're unfortunately not covered by the full regression tests. > There may be others parts of postgresql that would have workede fine as > well. > > This means 2 things: > - there is a (litle) risk to meet other assert failures when using planning > counters in pgss, > - we have an easy workarround to fix it (disabling track_planning). > > But I would have prefered this new feature to work the same way with or > without track_planning activated ;o( Definitely, but fixing the issue in pgss (ignoring planner calls when we don't have a query text) means that pgss won't give an exhaustive view of activity anymore, so a fix in IVM would be a better solution. Let's wait and see if Nagata-san and other people involved in that have an opinion on it. |
In reply to this post by Julien Rouhaud
Hi Julien,
bot is still unhappy https://travis-ci.org/postgresql-cfbot/postgresql/builds/638701399 portalcmds.c: In function ‘PerformCursorOpen’: portalcmds.c:93:7: error: ‘queryString’ may be used uninitialized in this function [-Werror=maybe-uninitialized] plan = pg_plan_query(query, queryString, cstmt->options, params); ^ portalcmds.c:50:8: note: ‘queryString’ was declared here char *queryString; ^ cc1: all warnings being treated as errors <builtin>: recipe for target 'portalcmds.o' failed make[3]: *** [portalcmds.o] Error 1 make[3]: Leaving directory '/home/travis/build/postgresql-cfbot/postgresql/src/backend/commands' common.mk:39: recipe for target 'commands-recursive' failed make[2]: *** [commands-recursive] Error 2 make[2]: *** Waiting for unfinished jobs.... regards PAscal -- Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html |
Hi,
On Sat, Jan 18, 2020 at 6:14 PM legrand legrand <[hidden email]> wrote: > > Hi Julien, > > bot is still unhappy > https://travis-ci.org/postgresql-cfbot/postgresql/builds/638701399 > > portalcmds.c: In function ‘PerformCursorOpen’: > portalcmds.c:93:7: error: ‘queryString’ may be used uninitialized in this > function [-Werror=maybe-uninitialized] > plan = pg_plan_query(query, queryString, cstmt->options, params); > ^ > portalcmds.c:50:8: note: ‘queryString’ was declared here > char *queryString; > ^ > cc1: all warnings being treated as errors > <builtin>: recipe for target 'portalcmds.o' failed > make[3]: *** [portalcmds.o] Error 1 > make[3]: Leaving directory > '/home/travis/build/postgresql-cfbot/postgresql/src/backend/commands' > common.mk:39: recipe for target 'commands-recursive' failed > make[2]: *** [commands-recursive] Error 2 > make[2]: *** Waiting for unfinished jobs.... ![]() ![]() |
In reply to this post by Julien Rouhaud
Hi Julien,
>> But I would have prefered this new feature to work the same way with or >> without track_planning activated ;o( > Definitely, but fixing the issue in pgss (ignoring planner calls when > we don't have a query text) means that pgss won't give an exhaustive > view of activity anymore, so a fix in IVM would be a better solution. > Let's wait and see if Nagata-san and other people involved in that > have an opinion on it. It seems IVM team does not consider this point as a priority ... We should not wait for them, if we want to keep a chance to be included in PG13. So we have to make this feature more robust, an assert failure being considered as a severe regression (even if this is not coming from pgss). I like the idea of adding a check for a non-zero queryId in the new pgss_planner_hook() (zero queryid shouldn't be reserved for utility_statements ?). Fixing the corner case where a query (with no sql text) can be planned without being parsed is an other subject that should be resolved in an other thread. This kind of query was ignored in pgss, it should be ignored in pgss with planning counters. Any thoughts ? Regards PAscal -- Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html |
Hi,
On Fri, Feb 28, 2020 at 4:06 PM legrand legrand <[hidden email]> wrote: > > It seems IVM team does not consider this point as a priority ... Well, IVM is a big project and I agree that fixing this issue isn't the most urgent one, especially since there's no guarantee that this pgss planning patch will be committed, or with the current behavior. > We should not wait for them, if we want to keep a chance to be > included in PG13. > > So we have to make this feature more robust, an assert failure being > considered as a severe regression (even if this is not coming from pgss). I'm still not convinced that handling NULL query string, as in sometimes ignoring planning counters, is the right solution here. For now all code is able to provide it (or at least all the code that goes through make installcheck). I'm wondering if it'd be better to instead add a similar assert in pg_plan_query, to make sure that this requirements is always met even without using pg_stat_statements, or any other extension that would also rely on that. I also realized that the last version of the patch I sent was a rebase of the wrong version, I'll send the correct version soon. > I like the idea of adding a check for a non-zero queryId in the new > pgss_planner_hook() (zero queryid shouldn't be reserved for > utility_statements ?). Some assert hit later, I can say that it's not always true. For instance a CREATE TABLE AS won't run parse analysis for the underlying query, as this has already been done for the original statement, but will still call the planner. I'll change pgss_planner_hook to ignore such cases, as pgss_store would otherwise think that it's a utility statement. That'll probably incidentally fix the IVM incompatibility. |
>> I like the idea of adding a check for a non-zero queryId in the new
>> pgss_planner_hook() (zero queryid shouldn't be reserved for >> utility_statements ?). > Some assert hit later, I can say that it's not always true. For > instance a CREATE TABLE AS won't run parse analysis for the underlying > query, as this has already been done for the original statement, but > will still call the planner. I'll change pgss_planner_hook to ignore > such cases, as pgss_store would otherwise think that it's a utility > statement. That'll probably incidentally fix the IVM incompatibility. Today with or without test on parse->queryId != UINT64CONST(0), CTAS is collected as a utility_statement without planning counter. This seems to me respectig the rule, not sure that this needs any new (risky) change to the actual (quite stable) patch. -- Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html |
On Sun, Mar 1, 2020 at 3:55 PM legrand legrand
<[hidden email]> wrote: > > >> I like the idea of adding a check for a non-zero queryId in the new > >> pgss_planner_hook() (zero queryid shouldn't be reserved for > >> utility_statements ?). > > > Some assert hit later, I can say that it's not always true. For > > instance a CREATE TABLE AS won't run parse analysis for the underlying > > query, as this has already been done for the original statement, but > > will still call the planner. I'll change pgss_planner_hook to ignore > > such cases, as pgss_store would otherwise think that it's a utility > > statement. That'll probably incidentally fix the IVM incompatibility. > > Today with or without test on parse->queryId != UINT64CONST(0), > CTAS is collected as a utility_statement without planning counter. > This seems to me respectig the rule, not sure that this needs any > new (risky) change to the actual (quite stable) patch. But the queryid ends up not being computed the same way: # select queryid, query, plans, calls from pg_stat_statements where query like 'create table%'; queryid | query | plans | calls ---------------------+--------------------------------+-------+------- 8275950546884151007 | create table test as select 1; | 1 | 0 7546197440584636081 | create table test as select 1 | 0 | 1 (2 rows) That's because CreateTableAsStmt->query doesn't have a query location/len, as transformTopLevelStmt is only setting that for the top level Query. That's probably an oversight in ab1f0c82257, but I'm not sure what's the best way to fix that. Should we pass that information to all transformXXX function, or let transformTopLevelStmt handle that. |
Julien Rouhaud wrote
> On Sun, Mar 1, 2020 at 3:55 PM legrand legrand > < > legrand_legrand@ > > wrote: >> >> >> I like the idea of adding a check for a non-zero queryId in the new >> >> pgss_planner_hook() (zero queryid shouldn't be reserved for >> >> utility_statements ?). >> >> > Some assert hit later, I can say that it's not always true. For >> > instance a CREATE TABLE AS won't run parse analysis for the underlying >> > query, as this has already been done for the original statement, but >> > will still call the planner. I'll change pgss_planner_hook to ignore >> > such cases, as pgss_store would otherwise think that it's a utility >> > statement. That'll probably incidentally fix the IVM incompatibility. >> >> Today with or without test on parse->queryId != UINT64CONST(0), >> CTAS is collected as a utility_statement without planning counter. >> This seems to me respectig the rule, not sure that this needs any >> new (risky) change to the actual (quite stable) patch. > > But the queryid ends up not being computed the same way: > > # select queryid, query, plans, calls from pg_stat_statements where > query like 'create table%'; > queryid | query | plans | calls > ---------------------+--------------------------------+-------+------- > 8275950546884151007 | create table test as select 1; | 1 | 0 > 7546197440584636081 | create table test as select 1 | 0 | 1 > (2 rows) > > That's because CreateTableAsStmt->query doesn't have a query > location/len, as transformTopLevelStmt is only setting that for the > top level Query. That's probably an oversight in ab1f0c82257, but I'm > not sure what's the best way to fix that. Should we pass that > information to all transformXXX function, or let transformTopLevelStmt > handle that. arf, this was not the case in my testing env (that is not up to date) :o( and would not have appeared at all with the proposed test on parse->queryId != UINT64CONST(0) ... -- Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html |
On Mon, Mar 2, 2020 at 1:01 PM legrand legrand
<[hidden email]> wrote: > > Julien Rouhaud wrote > > On Sun, Mar 1, 2020 at 3:55 PM legrand legrand > > < > > > legrand_legrand@ > > > > wrote: > >> > >> >> I like the idea of adding a check for a non-zero queryId in the new > >> >> pgss_planner_hook() (zero queryid shouldn't be reserved for > >> >> utility_statements ?). > >> > >> > Some assert hit later, I can say that it's not always true. For > >> > instance a CREATE TABLE AS won't run parse analysis for the underlying > >> > query, as this has already been done for the original statement, but > >> > will still call the planner. I'll change pgss_planner_hook to ignore > >> > such cases, as pgss_store would otherwise think that it's a utility > >> > statement. That'll probably incidentally fix the IVM incompatibility. > >> > >> Today with or without test on parse->queryId != UINT64CONST(0), > >> CTAS is collected as a utility_statement without planning counter. > >> This seems to me respectig the rule, not sure that this needs any > >> new (risky) change to the actual (quite stable) patch. > > > > But the queryid ends up not being computed the same way: > > > > # select queryid, query, plans, calls from pg_stat_statements where > > query like 'create table%'; > > queryid | query | plans | calls > > ---------------------+--------------------------------+-------+------- > > 8275950546884151007 | create table test as select 1; | 1 | 0 > > 7546197440584636081 | create table test as select 1 | 0 | 1 > > (2 rows) > > > > That's because CreateTableAsStmt->query doesn't have a query > > location/len, as transformTopLevelStmt is only setting that for the > > top level Query. That's probably an oversight in ab1f0c82257, but I'm > > not sure what's the best way to fix that. Should we pass that > > information to all transformXXX function, or let transformTopLevelStmt > > handle that. > > > arf, this was not the case in my testing env (that is not up to date) :o( > and would not have appeared at all with the proposed test on > parse->queryId != UINT64CONST(0) ... I'm not sure what was the exact behavior you had, but that shouldn't have changed since previous version. The underlying query isn't a top level statement, so maybe you didn't set pg_stat_statements.track = 'all'? |
Never mind ...
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). Regards PAscal -- Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html |
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. ![]() ![]() |
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? 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. -- Yoshikazu Imai |
Hi Imai-san,
On Thu, Mar 12, 2020 at 05:28:38AM +0000, [hidden email] 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? 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. ![]() ![]() |
On Thu, Mar 12, 2020 at 6:31 AM, Julien Rouhaud wrote:
> On Thu, Mar 12, 2020 at 05:28:38AM +0000, [hidden email] 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 |
On Thu, Mar 12, 2020 at 10:19 AM [hidden email]
<[hidden email]> wrote: > > I'll summary code review of this thread. Thanks for the summary! I just have some minor comments > [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] > > * 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. The bufusage should reflect the sum of planning and execution usage if track_planning is enabled. Did I miss something there? > [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. There's at least the current version of IVM patchset that lacks the querytext. Looking at various extensions, I see that pg_background and pglogical call pg_plan_query internally but shouldn't have any issue providing the query text. But there's also citus extension, which don't keep around the query string at least when distributing plans, which makes sense since it's of no use and they're heavily modifying the original Query. I think that citus folks opinion on the subject would be useful, so I'm Cc-ing Marco. |
Free forum by Nabble | Edit this page |