Planning counters in pg_stat_statements (using pgss_store)

On Wed, Nov 20, 2019 at 4:55 PM, Julien Rouhaud wrote:
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: Thomas' automatic patch tester just warned me that the patchset is
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:
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
> &lt;

> legrand_legrand@

> &gt; wrote:
>>
>> Hi Julien,
>>
>> I would like to create a link with
>> https://www.postgresql.org/message-id/

> 1577490124579-0.post@.nabble

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:
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.

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.

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: Indeed, thanks for the report!  PFA rebased v4 version of the patchset.

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:
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
> &lt;

> legrand_legrand@


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:
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: 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.

On Thu, Mar 12, 2020 at 6:31 AM, Julien Rouhaud wrote:
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

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.