Counters hook

[munakoiso]

This patch is related to #30

It's currently not possible to use calculated counters in other pg extensions.

I decided to move some structures to header and add a hook that will be called immediately after calculating the counters. And in other extensions it is possible to use calculated counters while calling the hook

[rjuju]

Hi,

I'm not opposed with either exposing some structures in a .h file and/or adding a hook in pg_stat_kcache, but I'd like to clarify some points.

First, for exposing internal data, I think we should be more selective. At the very least we shouldn't move all #include in the .h, but only add the one that are required for the header file itself, as it's done in postgres code. Then, I'm a bit worried about exposing everything. As-is, any extension could mess with the queryids array for instance, which doesn't seem like a legit thing to do. Arguably you can't already do that given how ShmemInitStruct() work, but there's a difference between copying the various struct in your own extension and have pg_stat_kcache officially allowing you to do so.

I was thinking that only exposing a few things would be enough, like maybe:

• pgskStoreKind
• pgskCounters

Additionally, if you have a use case for accessing the hmap entries directly, maybe we could also expose additional things, although I'd rather add some new API that returns the wanted entry and ensure proper locking.

Then for the hook, I think it would require a bit more comments. Unless I'm missing something the hook won't work unless you make sure that shared_preload_libraries in configured in such a way that your custom extension is loaded after pg_stat_kcache, so this should be made very clear, and probably add some extra comments hinting how third-party authors are supposed to use it.

[munakoiso]

Hi.

I decided to expose all (or almost all) of the structures just because maybe someday it will be useful for someone. PgskStoreKind and pgskCounters are enough for me.

Thanks for your point about order of shared_preload_libraries. I checked that case. It works in any order.

 ~ # cat /var/lib/postgresql/12/data/conf.d/postgresql.conf | grep shared_preload_libraries
shared_preload_libraries = 'pg_stat_statements,pg_comment_stats,pg_stat_kcache,repl_mon,logerrors'   # (change requires restart)
 ~ # cat /var/log/postgresql/postgresql-12-data.log | grep _PG_init
[ 2021-09-30 09:48:31.958 MSK ,,,1607600,01000 ]:WARNING:  pg_comment_stats: _PG_init
[ 2021-09-30 09:48:31.959 MSK ,,,1607600,01000 ]:WARNING:  pg_stat_kcache: _PG_init
 ~ # psql
could not change directory to "/root": Permission denied
Type "help" for help.

postgres M # /* a: 1*/ select 1;
 ?column?
----------
        1
(1 row)

Time: 0.416 ms
postgres M # select comment_keys, query_count from pgcs_get_stats();
NOTICE:  00000: pgcs: Show stats from '2021-09-30 09:57:50.46668+03' to '2021-09-30 09:58:09.7155+03'
LOCATION:  pgcs_internal_get_stats_time_interval, pg_comment_stats.c:848
 comment_keys | query_count
--------------+-------------
 {"a": "1"}   |           1
(1 row)

Time: 3.877 ms

[munakoiso]

@rjuju, could you take another look at this PR?

[rjuju]

Thanks a lot @munakoiso. The code looks good to me now. Could you rebase the patchset to keep only the first 2 commits and dispatch the changes in the other 2 commits? I can do it if you prefer, but I don't think that I can do that as of the pull request so I prefer to keep the PR history clean too.

[rjuju]

Oh, also please update the CONTRIBUTORS.md file to add either your name or your github username!

[munakoiso]

I did rebase and update CONTRIBUTORS.md

Thank you :)

[rjuju]

Thanks a lot, merged!