Обсуждение: relfilenode statistics
Hi hackers, Please find attached a POC patch to implement $SUBJECT. Adding relfilenode statistics has been proposed in [1]. The idea is to allow tracking dirtied blocks, written blocks,... on a per relation basis. The attached patch is not in a fully "polished" state yet: there is more places we should add relfilenode counters, create more APIS to retrieve the relfilenode stats.... But I think that it is in a state that can be used to discuss the approach it is implementing (so that we can agree or not on it) before moving forward. The approach that is implemented in this patch is the following: - A new PGSTAT_KIND_RELFILENODE is added - A new attribute (aka relfile) has been added to PgStat_HashKey so that we can record (dboid, spcOid and relfile) to identify a relfilenode entry - pgstat_create_transactional() is used in RelationCreateStorage() - pgstat_drop_transactional() is used in RelationDropStorage() - RelationPreserveStorage() will remove the entry from the list of dropped stats The current approach to deal with table rewrite is to: - copy the relfilenode stats in table_relation_set_new_filelocator() from the relfilenode stats entry to the shared table stats entry - in the pg_statio_all_tables view: add the table stats entry (that contains "previous" relfilenode stats (due to the above) that were linked to this relation ) to the current relfilenode stats linked to the relation An example is done in the attached patch for the new heap_blks_written field in pg_statio_all_tables. Outcome is: " postgres=# create table bdt (a int); CREATE TABLE postgres=# select heap_blks_written from pg_statio_all_tables where relname = 'bdt'; heap_blks_written ------------------- 0 (1 row) postgres=# insert into bdt select generate_series(1,10000); INSERT 0 10000 postgres=# select heap_blks_written from pg_statio_all_tables where relname = 'bdt'; heap_blks_written ------------------- 0 (1 row) postgres=# checkpoint; CHECKPOINT postgres=# select heap_blks_written from pg_statio_all_tables where relname = 'bdt'; heap_blks_written ------------------- 45 (1 row) postgres=# truncate table bdt; TRUNCATE TABLE postgres=# select heap_blks_written from pg_statio_all_tables where relname = 'bdt'; heap_blks_written ------------------- 45 (1 row) postgres=# insert into bdt select generate_series(1,10000); INSERT 0 10000 postgres=# select heap_blks_written from pg_statio_all_tables where relname = 'bdt'; heap_blks_written ------------------- 45 (1 row) postgres=# checkpoint; CHECKPOINT postgres=# select heap_blks_written from pg_statio_all_tables where relname = 'bdt'; heap_blks_written ------------------- 90 (1 row) " Some remarks: - My first attempt has been to call the pgstat_create_transactional() and pgstat_drop_transactional() at the same places it is done for the relations but that did not work well (mainly due to corner cases in case of rewrite). - Please don't take care of the pgstat_count_buffer_read() and pgstat_count_buffer_hit() calls in pgstat_report_relfilenode_buffer_read() and pgstat_report_relfilenode_buffer_hit(). Those stats will follow the same flow as the one done and explained above for the new heap_blks_written one ( should we agree on it). Looking forward to your comments, feedback. Regards, [1]: https://www.postgresql.org/message-id/20231113204439.r4lmys73tessqmak%40awork3.anarazel.de -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Вложения
Hi Bertrand, It would be helpful to me if the reasons why we're splitting out relfilenodestats could be more clearly spelled out. I see Andres's comment in the thread to which you linked, but it's pretty vague about why we should do this ("it's not nice") and whether we should do this ("I wonder if this is an argument for") and maybe that's all fine if Andres is going to be the one to review and commit this, but even if then it would be nice if the rest of us could follow along from home, and right now I can't. The commit message is often a good place to spell this kind of thing out, because then it's included with every version of the patch you post, and may be of some use to the eventual committer in writing their commit message. The body of the email where you post the patch set can be fine, too. ...Robert
Hi Robert, On Mon, May 27, 2024 at 09:10:13AM -0400, Robert Haas wrote: > Hi Bertrand, > > It would be helpful to me if the reasons why we're splitting out > relfilenodestats could be more clearly spelled out. I see Andres's > comment in the thread to which you linked, but it's pretty vague about > why we should do this ("it's not nice") and whether we should do this > ("I wonder if this is an argument for") and maybe that's all fine if > Andres is going to be the one to review and commit this, but even if > then it would be nice if the rest of us could follow along from home, > and right now I can't. Thanks for the feedback! You’re completely right, my previous message is missing clear explanation as to why I think that relfilenode stats could be useful. Let me try to fix this. The main argument is that we currently don’t have writes counters for relations. The reason is that we don’t have the relation OID when writing buffers out. Tracking writes per relfilenode would allow us to track/consolidate writes per relation (example in the v1 patch and in the message up-thread). I think that adding instrumentation in this area (writes counters) could be beneficial (like it is for the ones we currently have for reads). Second argument is that this is also beneficial for the "Split index and table statistics into different types of stats" thread (mentioned in the previous message). It would allow us to avoid additional branches in some situations (like the one mentioned by Andres in the link I provided up-thread). If we agree that the main argument makes sense to think about having relfilenode stats then I think using them as proposed in the second argument makes sense too: We’d move the current buffer read and buffer hit counters from the relation stats to the relfilenode stats (while still being able to retrieve them from the pg_statio_all_tables/indexes views: see the example for the new heap_blks_written stat added in the patch). Generally speaking, I think that tracking counters at a common level (i.e relfilenode level instead of table or index level) is beneficial (avoid storing/allocating space for the same counters in multiple structs) and sounds more intuitive to me. Also I think this is open door for new ideas: for example, with relfilenode statistics in place, we could probably also start thinking about tracking checksum errors per relfllenode. > The commit message is often a good place to spell this kind of thing > out, because then it's included with every version of the patch you > post, and may be of some use to the eventual committer in writing > their commit message. The body of the email where you post the patch > set can be fine, too. > Yeah, I’ll update the commit message in V2 with better explanations once I get feedback on V1 (should we decide to move on with the relfilenode stats idea). Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Mon, Jun 3, 2024 at 7:11 AM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote: > The main argument is that we currently don’t have writes counters for relations. > The reason is that we don’t have the relation OID when writing buffers out. OK. > Second argument is that this is also beneficial for the "Split index and > table statistics into different types of stats" thread (mentioned in the previous > message). It would allow us to avoid additional branches in some situations (like > the one mentioned by Andres in the link I provided up-thread). OK. > We’d move the current buffer read and buffer hit counters from the relation stats > to the relfilenode stats (while still being able to retrieve them from the > pg_statio_all_tables/indexes views: see the example for the new heap_blks_written > stat added in the patch). Generally speaking, I think that tracking counters at > a common level (i.e relfilenode level instead of table or index level) is > beneficial (avoid storing/allocating space for the same counters in multiple > structs) and sounds more intuitive to me. Hmm. So if I CLUSTER or VACUUM FULL the relation, the relfilenode changes. Does that mean I lose all of those stats? Is that a problem? Or is it good? Or what? I also thought about the other direction. Suppose I drop the a relation and create a new one that gets a different relation OID but the same relfilenode. But I don't think that's a problem: dropping the relation should forcibly remove the old stats, so there won't be any conflict in this case. -- Robert Haas EDB: http://www.enterprisedb.com
On Tue, Jun 04, 2024 at 09:26:27AM -0400, Robert Haas wrote: > On Mon, Jun 3, 2024 at 7:11 AM Bertrand Drouvot > <bertranddrouvot.pg@gmail.com> wrote: > > We’d move the current buffer read and buffer hit counters from the relation stats > > to the relfilenode stats (while still being able to retrieve them from the > > pg_statio_all_tables/indexes views: see the example for the new heap_blks_written > > stat added in the patch). Generally speaking, I think that tracking counters at > > a common level (i.e relfilenode level instead of table or index level) is > > beneficial (avoid storing/allocating space for the same counters in multiple > > structs) and sounds more intuitive to me. > > Hmm. So if I CLUSTER or VACUUM FULL the relation, the relfilenode > changes. Does that mean I lose all of those stats? Is that a problem? > Or is it good? Or what? I think we should keep the stats in the relation during relfilenode changes. As a POC, v1 implemented a way to do so during TRUNCATE (see the changes in table_relation_set_new_filelocator() and in pg_statio_all_tables): as you can see in the example provided up-thread the new heap_blks_written statistic has been preserved during the TRUNCATE. Please note that the v1 POC only takes care of the new heap_blks_written stat and that the logic used in table_relation_set_new_filelocator() would probably need to be applied in rebuild_relation() or such to deal with CLUSTER or VACUUM FULL. For the relation, the new counter "blocks_written" has been added to the PgStat_StatTabEntry struct (it's not needed in the PgStat_TableCounts one as the relfilenode stat takes care of it). It's added in PgStat_StatTabEntry only to copy/preserve the relfilenode stats during rewrite operations and to retrieve the stats in pg_statio_all_tables. Then, if later we split the relation stats to index/table stats, we'd have blocks_written defined in less structs (as compare to doing the split without relfilenode stat in place). As mentioned up-thread, the new logic has been implemented in v1 only for the new blocks_written stat (we'd need to do the same for the existing buffer read / buffer hit if we agree on the approach implemented in v1). > I also thought about the other direction. Suppose I drop the a > relation and create a new one that gets a different relation OID but > the same relfilenode. But I don't think that's a problem: dropping the > relation should forcibly remove the old stats, so there won't be any > conflict in this case. Yeah. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Hi, On Mon, Jun 03, 2024 at 11:11:46AM +0000, Bertrand Drouvot wrote: > Yeah, I’ll update the commit message in V2 with better explanations once I get > feedback on V1 (should we decide to move on with the relfilenode stats idea). > Please find attached v2, mandatory rebase due to cd312adc56. In passing it provides a more detailed commit message (also making clear that the goal of this patch is to start the discussion and agree on the design before moving forward.) Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Вложения
On Wed, Jun 5, 2024 at 1:52 AM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote: > I think we should keep the stats in the relation during relfilenode changes. > As a POC, v1 implemented a way to do so during TRUNCATE (see the changes in > table_relation_set_new_filelocator() and in pg_statio_all_tables): as you can > see in the example provided up-thread the new heap_blks_written statistic has > been preserved during the TRUNCATE. Yeah, I think there's something weird about this design. Somehow we're ending up with both per-relation and per-relfilenode counters: + pg_stat_get_blocks_written(C.oid) + pg_stat_get_relfilenode_blocks_written(d.oid, CASE WHEN C.reltablespace <> 0 THEN C.reltablespace ELSE d.dattablespace END, C.relfilenode) AS heap_blks_written, I'll defer to Andres if he thinks that's awesome, but to me it does not seem right to track some blocks written in a per-relation counter and others in a per-relfilenode counter. -- Robert Haas EDB: http://www.enterprisedb.com
Hi, On 2024-06-06 12:27:49 -0400, Robert Haas wrote: > On Wed, Jun 5, 2024 at 1:52 AM Bertrand Drouvot > <bertranddrouvot.pg@gmail.com> wrote: > > I think we should keep the stats in the relation during relfilenode changes. > > As a POC, v1 implemented a way to do so during TRUNCATE (see the changes in > > table_relation_set_new_filelocator() and in pg_statio_all_tables): as you can > > see in the example provided up-thread the new heap_blks_written statistic has > > been preserved during the TRUNCATE. > > Yeah, I think there's something weird about this design. Somehow we're > ending up with both per-relation and per-relfilenode counters: > > + pg_stat_get_blocks_written(C.oid) + > pg_stat_get_relfilenode_blocks_written(d.oid, CASE WHEN > C.reltablespace <> 0 THEN C.reltablespace ELSE d.dattablespace END, > C.relfilenode) AS heap_blks_written, > > I'll defer to Andres if he thinks that's awesome, but to me it does > not seem right to track some blocks written in a per-relation counter > and others in a per-relfilenode counter. It doesn't immediately sound awesome. Nor really necessary? If we just want to keep prior stats upon arelation rewrite, we can just copy the stats from the old relfilenode. Or we can decide that those stats don't really make sense anymore, and start from scratch. I *guess* I could see an occasional benefit in having both counter for "prior relfilenodes" and "current relfilenode" - except that stats get reset manually and upon crash anyway, making this less useful than if it were really "lifetime" stats. Greetings, Andres Freund
Hi, On 2024-06-03 11:11:46 +0000, Bertrand Drouvot wrote: > The main argument is that we currently don’t have writes counters for relations. > The reason is that we don’t have the relation OID when writing buffers out. > Tracking writes per relfilenode would allow us to track/consolidate writes per > relation (example in the v1 patch and in the message up-thread). > > I think that adding instrumentation in this area (writes counters) could be > beneficial (like it is for the ones we currently have for reads). > > Second argument is that this is also beneficial for the "Split index and > table statistics into different types of stats" thread (mentioned in the previous > message). It would allow us to avoid additional branches in some situations (like > the one mentioned by Andres in the link I provided up-thread). I think there's another *very* significant benefit: Right now physical replication doesn't populate statistics fields like n_dead_tup, which can be a huge issue after failovers, because there's little information about what autovacuum needs to do. Auto-analyze *partially* can fix it at times, if it's lucky enough to see enough dead tuples - but that's not a given and even if it works, is often wildly inaccurate. Once we put things like n_dead_tup into per-relfilenode stats, we can populate them during WAL replay. Thus after a promotion autovacuum has much better data. This also is important when we crash: We've been talking about storing a snapshot of the stats alongside each REDO pointer. Combined with updating stats during crash recovery, we'll have accurate dead-tuple stats once recovey has finished. Greetings, Andres Freund
Hi, On Thu, Jun 06, 2024 at 08:38:06PM -0700, Andres Freund wrote: > Hi, > > On 2024-06-03 11:11:46 +0000, Bertrand Drouvot wrote: > > The main argument is that we currently don’t have writes counters for relations. > > The reason is that we don’t have the relation OID when writing buffers out. > > Tracking writes per relfilenode would allow us to track/consolidate writes per > > relation (example in the v1 patch and in the message up-thread). > > > > I think that adding instrumentation in this area (writes counters) could be > > beneficial (like it is for the ones we currently have for reads). > > > > Second argument is that this is also beneficial for the "Split index and > > table statistics into different types of stats" thread (mentioned in the previous > > message). It would allow us to avoid additional branches in some situations (like > > the one mentioned by Andres in the link I provided up-thread). > > I think there's another *very* significant benefit: > > Right now physical replication doesn't populate statistics fields like > n_dead_tup, which can be a huge issue after failovers, because there's little > information about what autovacuum needs to do. > > Auto-analyze *partially* can fix it at times, if it's lucky enough to see > enough dead tuples - but that's not a given and even if it works, is often > wildly inaccurate. > > > Once we put things like n_dead_tup into per-relfilenode stats, Hm - I had in mind to populate relfilenode stats only with stats that are somehow related to I/O activities. Which ones do you have in mind to put in relfilenode stats? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Hi, On Thu, Jun 06, 2024 at 08:17:36PM -0700, Andres Freund wrote: > Hi, > > On 2024-06-06 12:27:49 -0400, Robert Haas wrote: > > On Wed, Jun 5, 2024 at 1:52 AM Bertrand Drouvot > > <bertranddrouvot.pg@gmail.com> wrote: > > > I think we should keep the stats in the relation during relfilenode changes. > > > As a POC, v1 implemented a way to do so during TRUNCATE (see the changes in > > > table_relation_set_new_filelocator() and in pg_statio_all_tables): as you can > > > see in the example provided up-thread the new heap_blks_written statistic has > > > been preserved during the TRUNCATE. > > > > Yeah, I think there's something weird about this design. Somehow we're > > ending up with both per-relation and per-relfilenode counters: > > > > + pg_stat_get_blocks_written(C.oid) + > > pg_stat_get_relfilenode_blocks_written(d.oid, CASE WHEN > > C.reltablespace <> 0 THEN C.reltablespace ELSE d.dattablespace END, > > C.relfilenode) AS heap_blks_written, > > > > I'll defer to Andres if he thinks that's awesome, but to me it does > > not seem right to track some blocks written in a per-relation counter > > and others in a per-relfilenode counter. > > It doesn't immediately sound awesome. Nor really necessary? > > If we just want to keep prior stats upon arelation rewrite, we can just copy > the stats from the old relfilenode. Agree, that's another option. But I think that would be in another field like "cumulative_XXX" to ensure one could still retrieve stats that are "dedicated" to this particular "new" relfilenode. Thoughts? > Or we can decide that those stats don't > really make sense anymore, and start from scratch. > > > I *guess* I could see an occasional benefit in having both counter for "prior > relfilenodes" and "current relfilenode" - except that stats get reset manually > and upon crash anyway, making this less useful than if it were really > "lifetime" stats. Right but currently they are not lost during a relation rewrite. If we decide to not keep the relfilenode stats during a rewrite then things like heap_blks_read would stop surviving a rewrite (if we move it to relfilenode stats) while it currently does. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Thu, Jun 6, 2024 at 11:17 PM Andres Freund <andres@anarazel.de> wrote: > If we just want to keep prior stats upon arelation rewrite, we can just copy > the stats from the old relfilenode. Or we can decide that those stats don't > really make sense anymore, and start from scratch. I think we need to think carefully about what we want the user experience to be here. "Per-relfilenode stats" could mean "sometimes I don't know the relation OID so I want to use the relfilenumber instead, without changing the user experience" or it could mean "some of these stats actually properly pertain to the relfilenode rather than the relation so I want to associate them with the right object and that will affect how the user sees things." We need to decide which it is. If it's the former, then we need to examine whether the goal of hiding the distinction between relfilenode stats and relation stats from the user is in fact feasible. If it's the latter, then we need to make sure the whole patch reflects that design, which would include e.g. NOT copying stats from the old to the new relfilenode, and which would also include documenting the behavior in a way that will be understandable to users. In my experience, the worst thing you can do in cases like this is be somewhere in the middle. Then you tend to end up with stuff like: the difference isn't supposed to be something that the user knows or cares about, except that they do have to know and care because you haven't thoroughly covered up the deception, and often they have to reverse engineer the behavior because you didn't document what was really happening because you imagined that they wouldn't notice. -- Robert Haas EDB: http://www.enterprisedb.com
Hi, On Fri, Jun 07, 2024 at 09:24:41AM -0400, Robert Haas wrote: > On Thu, Jun 6, 2024 at 11:17 PM Andres Freund <andres@anarazel.de> wrote: > > If we just want to keep prior stats upon arelation rewrite, we can just copy > > the stats from the old relfilenode. Or we can decide that those stats don't > > really make sense anymore, and start from scratch. > > I think we need to think carefully about what we want the user > experience to be here. "Per-relfilenode stats" could mean "sometimes I > don't know the relation OID so I want to use the relfilenumber > instead, without changing the user experience" or it could mean "some > of these stats actually properly pertain to the relfilenode rather > than the relation so I want to associate them with the right object > and that will affect how the user sees things." We need to decide > which it is. If it's the former, then we need to examine whether the > goal of hiding the distinction between relfilenode stats and relation > stats from the user is in fact feasible. If it's the latter, then we > need to make sure the whole patch reflects that design, which would > include e.g. NOT copying stats from the old to the new relfilenode, > and which would also include documenting the behavior in a way that > will be understandable to users. Thanks for sharing your thoughts! Let's take the current heap_blks_read as an example: it currently survives a relation rewrite and I guess we don't want to change the existing user experience for it. Now say we want to add "heap_blks_written" (like in this POC patch) then I think that it makes sense for the user to 1) query this new stat from the same place as the existing heap_blks_read: from pg_statio_all_tables and 2) to have the same experience as far the relation rewrite is concerned (keep the previous stats). To achieve the rewrite behavior we could: 1) copy the stats from the OLD relfilenode to the relation (like in the POC patch) 2) copy the stats from the OLD relfilenode to the NEW one (could be in a dedicated field) The PROS of 1) is that the behavior is consistent with the current heap_blks_read and that the user could still see the current relfilenode stats (through a new API) if he wants to. > In my experience, the worst thing you can do in cases like this is be > somewhere in the middle. Then you tend to end up with stuff like: the > difference isn't supposed to be something that the user knows or cares > about, except that they do have to know and care because you haven't > thoroughly covered up the deception, and often they have to reverse > engineer the behavior because you didn't document what was really > happening because you imagined that they wouldn't notice. My idea was to move all that is in pg_statio_all_tables to relfilenode stats and 1) add new stats to pg_statio_all_tables (like heap_blks_written), 2) ensure the user can still retrieve the stats from pg_statio_all_tables in such a way that it survives a rewrite, 3) provide dedicated APIs to retrieve relfilenode stats but only for the current relfilenode, 4) document this behavior. This is what the POC patch is doing for heap_blks_written (would need to do the same for heap_blks_read and friends) except for the documentation part. What do you think? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
At Mon, 10 Jun 2024 08:09:56 +0000, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote in > Hi, > > On Fri, Jun 07, 2024 at 09:24:41AM -0400, Robert Haas wrote: > > On Thu, Jun 6, 2024 at 11:17 PM Andres Freund <andres@anarazel.de> wrote: > > > If we just want to keep prior stats upon arelation rewrite, we can just copy > > > the stats from the old relfilenode. Or we can decide that those stats don't > > > really make sense anymore, and start from scratch. > > > > I think we need to think carefully about what we want the user > > experience to be here. "Per-relfilenode stats" could mean "sometimes I > > don't know the relation OID so I want to use the relfilenumber > > instead, without changing the user experience" or it could mean "some > > of these stats actually properly pertain to the relfilenode rather > > than the relation so I want to associate them with the right object > > and that will affect how the user sees things." We need to decide > > which it is. If it's the former, then we need to examine whether the > > goal of hiding the distinction between relfilenode stats and relation > > stats from the user is in fact feasible. If it's the latter, then we > > need to make sure the whole patch reflects that design, which would > > include e.g. NOT copying stats from the old to the new relfilenode, > > and which would also include documenting the behavior in a way that > > will be understandable to users. > > Thanks for sharing your thoughts! > > Let's take the current heap_blks_read as an example: it currently survives > a relation rewrite and I guess we don't want to change the existing user > experience for it. > > Now say we want to add "heap_blks_written" (like in this POC patch) then I think > that it makes sense for the user to 1) query this new stat from the same place > as the existing heap_blks_read: from pg_statio_all_tables and 2) to have the same > experience as far the relation rewrite is concerned (keep the previous stats). > > To achieve the rewrite behavior we could: > > 1) copy the stats from the OLD relfilenode to the relation (like in the POC patch) > 2) copy the stats from the OLD relfilenode to the NEW one (could be in a dedicated > field) > > The PROS of 1) is that the behavior is consistent with the current heap_blks_read > and that the user could still see the current relfilenode stats (through a new API) > if he wants to. > > > In my experience, the worst thing you can do in cases like this is be > > somewhere in the middle. Then you tend to end up with stuff like: the > > difference isn't supposed to be something that the user knows or cares > > about, except that they do have to know and care because you haven't > > thoroughly covered up the deception, and often they have to reverse > > engineer the behavior because you didn't document what was really > > happening because you imagined that they wouldn't notice. > > My idea was to move all that is in pg_statio_all_tables to relfilenode stats > and 1) add new stats to pg_statio_all_tables (like heap_blks_written), 2) ensure > the user can still retrieve the stats from pg_statio_all_tables in such a way > that it survives a rewrite, 3) provide dedicated APIs to retrieve > relfilenode stats but only for the current relfilenode, 4) document this > behavior. This is what the POC patch is doing for heap_blks_written (would > need to do the same for heap_blks_read and friends) except for the documentation > part. What do you think? In my opinion, it is certainly strange that bufmgr is aware of relation kinds, but introducing relfilenode stats to avoid this skew doesn't seem to be the best way, as it invites inconclusive arguments like the one raised above. The fact that we transfer counters from old relfilenodes to new ones indicates that we are not really interested in counts by relfilenode. If that's the case, wouldn't it be simpler to call pgstat_count_relation_buffer_read() from bufmgr.c and then branch according to relkind within that function? If you're concerned about the additional branch, some ingenuity may be needed. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Hi, On Tue, Jun 11, 2024 at 03:35:23PM +0900, Kyotaro Horiguchi wrote: > At Mon, 10 Jun 2024 08:09:56 +0000, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote in > > > > My idea was to move all that is in pg_statio_all_tables to relfilenode stats > > and 1) add new stats to pg_statio_all_tables (like heap_blks_written), 2) ensure > > the user can still retrieve the stats from pg_statio_all_tables in such a way > > that it survives a rewrite, 3) provide dedicated APIs to retrieve > > relfilenode stats but only for the current relfilenode, 4) document this > > behavior. This is what the POC patch is doing for heap_blks_written (would > > need to do the same for heap_blks_read and friends) except for the documentation > > part. What do you think? > > In my opinion, Thanks for looking at it! > it is certainly strange that bufmgr is aware of > relation kinds, but introducing relfilenode stats to avoid this skew > doesn't seem to be the best way, as it invites inconclusive arguments > like the one raised above. The fact that we transfer counters from old > relfilenodes to new ones indicates that we are not really interested > in counts by relfilenode. If that's the case, wouldn't it be simpler > to call pgstat_count_relation_buffer_read() from bufmgr.c and then > branch according to relkind within that function? If you're concerned > about the additional branch, some ingenuity may be needed. That may be doable for "read" activities but what about write activities? Do you mean not relying on relfilenode stats for reads but relying on relfilenode stats for writes? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com