Metrics: add new metrics for the upcoming Postgres v14 release. #399
Conversation
Some new views are more nische, so don't add the to default configs though.
|
@carlosbrb @markwort Hey, please review:) Can't break anything for old setups though.. |
There was a problem hiding this comment.
Dear @kmoppel-cognite , thanks for your PR, I hope you're doing well.
I've added a few comments. Primarily, I'm unsure about the conversion from floating point numbers to integers, maybe you can enlighten us with your thought processes?
Do you forsee any issues with Grafana and floats? Or is this about some of the metrics storage DB options?
| (extract(epoch from now()) * 1e9)::int8 as epoch_ns, | ||
| wal_records, | ||
| wal_fpi, | ||
| (wal_bytes / 1000)::int8 as wal_bytes_kb, |
There was a problem hiding this comment.
In wal_size the size is measured in bytes, I've seen several other cases (data dir sizze, table bloat e.g.) where bytes are used.
I think we should leave this unit as is (in bytes) and not convert it. It is cheap (and consistent with other usage) to do the conversion in e.g. Grafana when displaying the data.
If you're worried that this might soon exceed bigint sizes, then I would at least vote to make the divisor be 1024, not 1000.
There was a problem hiding this comment.
Hey Julian, thanks for the review! And I think you're right - was worrying too much here :) if there would be a real danger of overflow then the PG core team would have used some other unit, will change to bytes..
| @@ -2,7 +2,7 @@ select | |||
| (extract(epoch from now()) * 1e9)::int8 as epoch_ns, | |||
| wal_records, | |||
| wal_fpi, | |||
| (wal_bytes / 1000)::int8 as wal_bytes_kb, | |||
| wal_bytes, | |||
There was a problem hiding this comment.
Thanks for the quick fix, but I think you forgot something that you mentioned in another comment...
For some reason, the wal_bytes are of data type numeric, so a cast to bigint or similar might indeed be necessary?
There was a problem hiding this comment.
Ah crap, missed that it was numeric, good that someone is paying attention :) ...so indeed core devs considered an overflow possibility..so thatswhy I probably did the division thing in the 1st place...I guess to be on the safe side let's divide by 1024 then? Maybe some data warehouse is writing a petabyte per day indeed - then they have "only" 25 years with byte units :)
There was a problem hiding this comment.
Well, the time counters in pg_stat_database would overflow after 29 years as well...
I guess the assumption is that users will run through at least one statistics reset before that anyway?
I don't feel confident making a decision here. I guess whoever constructs the dashboards to display this data should look up the units anyway, and then the name of the field wal_bytes_kb would indicate what kind of value they are getting anyway.
So if you think that converting to kilobytes is a good idea then I am fine with it.
There was a problem hiding this comment.
Yes I think kB units are just fine like that when they're explicit - the metric is not part of default configs / dashboards anyways and better be as future proof as possible - this metrics will stay valid for all future PG versions as well if no breaking catalog changes to pg_stat_wal
Some new views are more niche, so don't add the to default configs though