relfilenode statistics

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

On Sat, May 25, 2024 at 07:52:02AM +0000, Bertrand Drouvot wrote:

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.

I have read through the patch to get an idea of how things are done,
and I am troubled by the approach taken (mentioned down by you), but
that's invasive compared to how pgstats wants to be transparent with
its stats kinds.

+   Oid         objoid;         /* object ID, either table or function
or tablespace. */
+   RelFileNumber relfile;      /* relfilenumber for RelFileLocator. */
 } PgStat_HashKey;

This adds a relfilenode component to the central hash key used for the
dshash of pgstats, which is something most stats types don't care
about. That looks like the incorrect thing to do to me, particularly
seeing a couple of lines down that a stats kind is assigned so the
HashKey uniqueness is ensured by the KindInfo:
+ [PGSTAT_KIND_RELFILENODE] = {
+ .name = "relfilenode",

FWIW, I have on my stack of patches something to switch the objoid to
8 bytes, actually, which is something that would be required for
pg_stat_statements as query IDs are wider than that and affect all
databases, FWIW. Relfilenodes are 4 bytes, okay still Robert has
proposed a couple of years ago a patch set to bump that to 56 bits,
change reverted in a448e49bcbe4. The objoid is also not something
specific to OIDs, see replication slots with their idx for example.

What you would be looking instead is to use the relfilenode as an
objoid and keep track of the OID of the original relation in each
PgStat_StatRelFileNodeEntry so as it is possible to know where a past
relfilenode was used? That makes looking back at the past relation's
elfilenodes stats more complicated as it would be necessary to keep a
list of the past relfilenodes for a relation, as well. Perhaps with
some kind of cache that maintains a mapping between the relation and
its relfilenode history?
--
Michael

Hi,

On Wed, Jul 10, 2024 at 03:02:34PM +0900, Michael Paquier wrote:

On Sat, May 25, 2024 at 07:52:02AM +0000, Bertrand Drouvot wrote:

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.

I have read through the patch to get an idea of how things are done,

Thanks!

and I am troubled by the approach taken (mentioned down by you), but
that's invasive compared to how pgstats wants to be transparent with
its stats kinds.

+   Oid         objoid;         /* object ID, either table or function
or tablespace. */
+   RelFileNumber relfile;      /* relfilenumber for RelFileLocator. */
} PgStat_HashKey;

This adds a relfilenode component to the central hash key used for the
dshash of pgstats, which is something most stats types don't care
about.

That's right but that's an existing behavior without the patch as:

PGSTAT_KIND_DATABASE does not care care about the objoid
PGSTAT_KIND_REPLSLOT does not care care about the dboid
PGSTAT_KIND_SUBSCRIPTION does not care care about the dboid

That's 3 kinds out of the 5 non fixed stats kind.

Not saying it's good, just saying that's an existing behavior.

That looks like the incorrect thing to do to me, particularly
seeing a couple of lines down that a stats kind is assigned so the
HashKey uniqueness is ensured by the KindInfo:
+ [PGSTAT_KIND_RELFILENODE] = {
+ .name = "relfilenode",

You mean, just rely on kind, dboid and relfile to ensure uniqueness?

I'm not sure that would work as there is this comment in relfilelocator.h:

"
* Notice that relNumber is only unique within a database in a particular
* tablespace.
"

So, I think it makes sense to link the hashkey to all the RelFileLocator
fields, means:

dboid (linked to RelFileLocator's dbOid)
objoid (linked to RelFileLocator's spcOid)
relfile (linked to RelFileLocator's relNumber)

FWIW, I have on my stack of patches something to switch the objoid to
8 bytes, actually, which is something that would be required for
pg_stat_statements as query IDs are wider than that and affect all
databases, FWIW. Relfilenodes are 4 bytes, okay still Robert has
proposed a couple of years ago a patch set to bump that to 56 bits,
change reverted in a448e49bcbe4.

Right, but it really looks like this extra field is needed to ensure
uniqueness (see above).

What you would be looking instead is to use the relfilenode as an
objoid

Not sure that works, as it looks like uniqueness won't be ensured (see above).

and keep track of the OID of the original relation in each
PgStat_StatRelFileNodeEntry so as it is possible to know where a past
relfilenode was used? That makes looking back at the past relation's
elfilenodes stats more complicated as it would be necessary to keep a
list of the past relfilenodes for a relation, as well. Perhaps with
some kind of cache that maintains a mapping between the relation and
its relfilenode history?

Yeah, I also thought about keeping a list of "previous" relfilenodes stats for a
relation but that would lead to:

1. Keep previous relfilnode stats
2. A more complicated way to look at relation stats (as you said)
3. Extra memory usage

I think the only reason "previous" relfilenode stats are needed is to provide
accurate stats for the relation. Outside of this need, I don't think we would
want to retrieve "individual" previous relfilenode stats in the past.

That's why the POC patch "simply" copies the stats to the relation during a
rewrite (before getting rid of the "previous" relfilenode stats).

What do you think?

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Wed, Jul 10, 2024 at 01:38:06PM +0000, Bertrand Drouvot wrote:

On Wed, Jul 10, 2024 at 03:02:34PM +0900, Michael Paquier wrote:

and I am troubled by the approach taken (mentioned down by you), but
that's invasive compared to how pgstats wants to be transparent with
its stats kinds.

+   Oid         objoid;         /* object ID, either table or function
or tablespace. */
+   RelFileNumber relfile;      /* relfilenumber for RelFileLocator. */
} PgStat_HashKey;

This adds a relfilenode component to the central hash key used for the
dshash of pgstats, which is something most stats types don't care
about.

That's right but that's an existing behavior without the patch as:

PGSTAT_KIND_DATABASE does not care care about the objoid
PGSTAT_KIND_REPLSLOT does not care care about the dboid
PGSTAT_KIND_SUBSCRIPTION does not care care about the dboid

That's 3 kinds out of the 5 non fixed stats kind.

I'd like to think that this is just going to increase across time.

That looks like the incorrect thing to do to me, particularly
seeing a couple of lines down that a stats kind is assigned so the
HashKey uniqueness is ensured by the KindInfo:
+ [PGSTAT_KIND_RELFILENODE] = {
+ .name = "relfilenode",

You mean, just rely on kind, dboid and relfile to ensure uniqueness?

Or table OID for the objid, with a hardcoded number of past
relfilenodes stats stored, to limit bloating the dshash with too much
past stats. See below.

So, I think it makes sense to link the hashkey to all the RelFileLocator
fields, means:

dboid (linked to RelFileLocator's dbOid)
objoid (linked to RelFileLocator's spcOid)
relfile (linked to RelFileLocator's relNumber)

Hmm. How about using the table OID as objoid, but store in the stats
of the new KindInfo an array of entries with the relfilenodes (current
and past, perhaps with more data than the relfilenode to ensure the
uniqueness tracking) and each of its stats? The number of past
relfilenodes would be fixed, meaning that there would be a strict
control with the retention of the past stats. When a table is
dropped, removing its relfilenode stats would be as cheap as when its
PGSTAT_KIND_RELATION is dropped.

Yeah, I also thought about keeping a list of "previous" relfilenodes stats for a
relation but that would lead to:

1. Keep previous relfilnode stats
2. A more complicated way to look at relation stats (as you said)
3. Extra memory usage

I think the only reason "previous" relfilenode stats are needed is to provide
accurate stats for the relation. Outside of this need, I don't think we would
want to retrieve "individual" previous relfilenode stats in the past.

That's why the POC patch "simply" copies the stats to the relation during a
rewrite (before getting rid of the "previous" relfilenode stats).

Hmm. Okay.
--
Michael

Hi,

On Thu, Jul 11, 2024 at 01:58:19PM +0900, Michael Paquier wrote:

On Wed, Jul 10, 2024 at 01:38:06PM +0000, Bertrand Drouvot wrote:

So, I think it makes sense to link the hashkey to all the RelFileLocator
fields, means:

dboid (linked to RelFileLocator's dbOid)
objoid (linked to RelFileLocator's spcOid)
relfile (linked to RelFileLocator's relNumber)

Hmm. How about using the table OID as objoid,

The issue is that we don't have the relation OID when writing buffers out (that's
one of the reason explained in [1]/messages/by-id/Zl2k8u4HDTUW6QlC@ip-10-97-1-34.eu-west-3.compute.internal).

[1]: /messages/by-id/Zl2k8u4HDTUW6QlC@ip-10-97-1-34.eu-west-3.compute.internal

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Hi,

On Thu, Jul 11, 2024 at 06:10:23AM +0000, Bertrand Drouvot wrote:

Hi,

On Thu, Jul 11, 2024 at 01:58:19PM +0900, Michael Paquier wrote:

On Wed, Jul 10, 2024 at 01:38:06PM +0000, Bertrand Drouvot wrote:

So, I think it makes sense to link the hashkey to all the RelFileLocator
fields, means:

dboid (linked to RelFileLocator's dbOid)
objoid (linked to RelFileLocator's spcOid)
relfile (linked to RelFileLocator's relNumber)

Hmm. How about using the table OID as objoid,

The issue is that we don't have the relation OID when writing buffers out (that's
one of the reason explained in [1]).

[1]: /messages/by-id/Zl2k8u4HDTUW6QlC@ip-10-97-1-34.eu-west-3.compute.internal

Regards,

Please find attached a mandatory rebase due to the recent changes around
statistics.

As mentioned up-thread:

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

It is in a state that can be used to discuss the approach it is implementing (as
we have done so far) before moving forward.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com