Обсуждение: [PATCH] pg_statio_all_tables: several rows per table due to invalid TOAST index

Hi, hackers!

It seems we have a problem in pg_statio_all_tables view defenition.
According to the documentation and identification fields, this view
must have exact one row per a table.
The view definition contains an x.indexrelid as the last field in its
GROUP BY list:

    <...>
    GROUP BY c.oid, n.nspname, c.relname, t.oid, x.indexrelid

Which is the oid of a TOAST-index.

However it is possible that the TOAST table will have more than one
index. For example, this happens when REINDEX CONCURRENTLY operation
lefts an index in invalid state (indisvalid = false) due to some kind
of a failure. It's often sufficient to interrupt REINDEX CONCURRENTLY
operation right after start.

Such index will cause the second row to appear in a
pg_statio_all_tables view which obvious is unexpected behaviour.

Now we can have several regular indexes and several TOAST-indexes for
the same table. Statistics for the regular and TOAST indexes is to be
calculated the same way so I've decided to use a CTE here.

The proposed view definition follows:

CREATE VIEW pg_statio_all_tables AS
    WITH indstat AS (
        SELECT
            indrelid,
            sum(pg_stat_get_blocks_fetched(indexrelid) -
                pg_stat_get_blocks_hit(indexrelid))::bigint
            AS idx_blks_read,
            sum(pg_stat_get_blocks_hit(indexrelid))::bigint
            AS idx_blks_hit
        FROM
            pg_index
        GROUP BY indrelid
    )
    SELECT
            C.oid AS relid,
            N.nspname AS schemaname,
            C.relname AS relname,
            pg_stat_get_blocks_fetched(C.oid) -
                    pg_stat_get_blocks_hit(C.oid) AS heap_blks_read,
            pg_stat_get_blocks_hit(C.oid) AS heap_blks_hit,
            I.idx_blks_read AS idx_blks_read,
            I.idx_blks_hit AS idx_blks_hit,
            pg_stat_get_blocks_fetched(T.oid) -
                    pg_stat_get_blocks_hit(T.oid) AS toast_blks_read,
            pg_stat_get_blocks_hit(T.oid) AS toast_blks_hit,
            X.idx_blks_read AS tidx_blks_read,
            X.idx_blks_read AS tidx_blks_hit
    FROM pg_class C LEFT JOIN
            indstat I ON C.oid = I.indrelid LEFT JOIN
            pg_class T ON C.reltoastrelid = T.oid LEFT JOIN
            indstat X ON T.oid = X.indrelid
            LEFT JOIN pg_namespace N ON (N.oid = C.relnamespace)
    WHERE C.relkind IN ('r', 't', 'm');

Reported by Sergey Grinko.

Regards.
-- 
Andrei Zubkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Mon, Nov 29, 2021 at 05:04:29PM +0300, Andrei Zubkov wrote:
> However it is possible that the TOAST table will have more than one
> index. For example, this happens when REINDEX CONCURRENTLY operation
> lefts an index in invalid state (indisvalid = false) due to some kind
> of a failure. It's often sufficient to interrupt REINDEX CONCURRENTLY
> operation right after start.
>
> Such index will cause the second row to appear in a
> pg_statio_all_tables view which obvious is unexpected behaviour.

Indeed.  I can see that.

> Now we can have several regular indexes and several TOAST-indexes for
> the same table. Statistics for the regular and TOAST indexes is to be
> calculated the same way so I've decided to use a CTE here.

Hmm.  Why should we care about invalid indexes at all, including
pg_statio_all_indexes?
--
Michael

Hi, Michael

Thank you for your attention!

On Tue, 2021-11-30 at 17:29 +0900, Michael Paquier wrote:
> Hmm.  Why should we care about invalid indexes at all, including
> pg_statio_all_indexes?
> 

I think we should care about them at least because they are exists and
can consume resources. For example, invalid index is to be updated by
DML operations.
Of course we can exclude such indexes from a view using isvalid,
isready, islive fields. But in such case we should mention this in the
docs, and more important is that the new such states of indexes can
appear in the future causing change in a view definition. Counting all
indexes regardless of states seems more reasonable to me.

-- 
Andrei Zubkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

It seems we need to bump catalog version here.
-- 
Andrei Zubkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Andrei Zubkov <zubkov@moonset.ru> writes:
> On Tue, 2021-11-30 at 17:29 +0900, Michael Paquier wrote:
>> Hmm.  Why should we care about invalid indexes at all, including
>> pg_statio_all_indexes?

> I think we should care about them at least because they are exists and
> can consume resources. For example, invalid index is to be updated by
> DML operations.
> Of course we can exclude such indexes from a view using isvalid,
> isready, islive fields. But in such case we should mention this in the
> docs, and more important is that the new such states of indexes can
> appear in the future causing change in a view definition. Counting all
> indexes regardless of states seems more reasonable to me.

Yeah, I agree, especially since we do it like that for the table's
own indexes.  I have a couple of comments though:

1. There's a silly typo in the view definition (it outputs tidx_blks_read
twice).  Fixed in the attached v2.

2. Historically, if you put any constraints on the view output, like
    select * from pg_statio_all_tables where relname like 'foo%';
you'd get a commensurate reduction in the amount of work done.  With
this version, you don't: the CTE will get computed in its entirety
even if we just need one row of its result.  This seems pretty bad,
especially for installations with many tables --- I suspect many
users would think this cure is worse than the disease.

I'm not quite sure what to do about #2.  I thought of just removing
X.indexrelid from the GROUP BY clause and summing over the toast
index(es) as we do for the table's index(es).  But that doesn't
work: if there are N > 1 table indexes, then the counts for
the toast index(es) will be multiplied by N, and conversely if
there are multiple toast indexes then the counts for the table
indexes will be scaled up.  We need to sum separately over the
table indexes and toast indexes, and I don't immediately see how
to do that without creating an optimization fence.

            regards, tom lane

diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index bb1ac30cd1..a977efd3c5 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -710,6 +710,18 @@ CREATE VIEW pg_stat_xact_user_tables AS
           schemaname !~ '^pg_toast';

 CREATE VIEW pg_statio_all_tables AS
+    WITH indstat AS (
+        SELECT
+            indrelid,
+            sum(pg_stat_get_blocks_fetched(indexrelid) -
+                pg_stat_get_blocks_hit(indexrelid))::bigint
+            AS idx_blks_read,
+            sum(pg_stat_get_blocks_hit(indexrelid))::bigint
+            AS idx_blks_hit
+        FROM
+            pg_index
+        GROUP BY indrelid
+    )
     SELECT
             C.oid AS relid,
             N.nspname AS schemaname,
@@ -717,22 +729,19 @@ CREATE VIEW pg_statio_all_tables AS
             pg_stat_get_blocks_fetched(C.oid) -
                     pg_stat_get_blocks_hit(C.oid) AS heap_blks_read,
             pg_stat_get_blocks_hit(C.oid) AS heap_blks_hit,
-            sum(pg_stat_get_blocks_fetched(I.indexrelid) -
-                    pg_stat_get_blocks_hit(I.indexrelid))::bigint AS idx_blks_read,
-            sum(pg_stat_get_blocks_hit(I.indexrelid))::bigint AS idx_blks_hit,
+            I.idx_blks_read AS idx_blks_read,
+            I.idx_blks_hit AS idx_blks_hit,
             pg_stat_get_blocks_fetched(T.oid) -
                     pg_stat_get_blocks_hit(T.oid) AS toast_blks_read,
             pg_stat_get_blocks_hit(T.oid) AS toast_blks_hit,
-            pg_stat_get_blocks_fetched(X.indexrelid) -
-                    pg_stat_get_blocks_hit(X.indexrelid) AS tidx_blks_read,
-            pg_stat_get_blocks_hit(X.indexrelid) AS tidx_blks_hit
+            X.idx_blks_read AS tidx_blks_read,
+            X.idx_blks_hit AS tidx_blks_hit
     FROM pg_class C LEFT JOIN
-            pg_index I ON C.oid = I.indrelid LEFT JOIN
+            indstat I ON C.oid = I.indrelid LEFT JOIN
             pg_class T ON C.reltoastrelid = T.oid LEFT JOIN
-            pg_index X ON T.oid = X.indrelid
+            indstat X ON T.oid = X.indrelid
             LEFT JOIN pg_namespace N ON (N.oid = C.relnamespace)
-    WHERE C.relkind IN ('r', 't', 'm')
-    GROUP BY C.oid, N.nspname, C.relname, T.oid, X.indexrelid;
+    WHERE C.relkind IN ('r', 't', 'm');

 CREATE VIEW pg_statio_sys_tables AS
     SELECT * FROM pg_statio_all_tables
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index ac468568a1..df1a3c1297 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -2256,24 +2256,30 @@ pg_statio_all_sequences| SELECT c.oid AS relid,
    FROM (pg_class c
      LEFT JOIN pg_namespace n ON ((n.oid = c.relnamespace)))
   WHERE (c.relkind = 'S'::"char");
-pg_statio_all_tables| SELECT c.oid AS relid,
+pg_statio_all_tables| WITH indstat AS (
+         SELECT pg_index.indrelid,
+            (sum((pg_stat_get_blocks_fetched(pg_index.indexrelid) -
pg_stat_get_blocks_hit(pg_index.indexrelid))))::bigintAS idx_blks_read, 
+            (sum(pg_stat_get_blocks_hit(pg_index.indexrelid)))::bigint AS idx_blks_hit
+           FROM pg_index
+          GROUP BY pg_index.indrelid
+        )
+ SELECT c.oid AS relid,
     n.nspname AS schemaname,
     c.relname,
     (pg_stat_get_blocks_fetched(c.oid) - pg_stat_get_blocks_hit(c.oid)) AS heap_blks_read,
     pg_stat_get_blocks_hit(c.oid) AS heap_blks_hit,
-    (sum((pg_stat_get_blocks_fetched(i.indexrelid) - pg_stat_get_blocks_hit(i.indexrelid))))::bigint AS idx_blks_read,
-    (sum(pg_stat_get_blocks_hit(i.indexrelid)))::bigint AS idx_blks_hit,
+    i.idx_blks_read,
+    i.idx_blks_hit,
     (pg_stat_get_blocks_fetched(t.oid) - pg_stat_get_blocks_hit(t.oid)) AS toast_blks_read,
     pg_stat_get_blocks_hit(t.oid) AS toast_blks_hit,
-    (pg_stat_get_blocks_fetched(x.indexrelid) - pg_stat_get_blocks_hit(x.indexrelid)) AS tidx_blks_read,
-    pg_stat_get_blocks_hit(x.indexrelid) AS tidx_blks_hit
+    x.idx_blks_read AS tidx_blks_read,
+    x.idx_blks_hit AS tidx_blks_hit
    FROM ((((pg_class c
-     LEFT JOIN pg_index i ON ((c.oid = i.indrelid)))
+     LEFT JOIN indstat i ON ((c.oid = i.indrelid)))
      LEFT JOIN pg_class t ON ((c.reltoastrelid = t.oid)))
-     LEFT JOIN pg_index x ON ((t.oid = x.indrelid)))
+     LEFT JOIN indstat x ON ((t.oid = x.indrelid)))
      LEFT JOIN pg_namespace n ON ((n.oid = c.relnamespace)))
-  WHERE (c.relkind = ANY (ARRAY['r'::"char", 't'::"char", 'm'::"char"]))
-  GROUP BY c.oid, n.nspname, c.relname, t.oid, x.indexrelid;
+  WHERE (c.relkind = ANY (ARRAY['r'::"char", 't'::"char", 'm'::"char"]));
 pg_statio_sys_indexes| SELECT pg_statio_all_indexes.relid,
     pg_statio_all_indexes.indexrelid,
     pg_statio_all_indexes.schemaname,

I wrote:
> ... We need to sum separately over the
> table indexes and toast indexes, and I don't immediately see how
> to do that without creating an optimization fence.

After a bit of further fooling, I found that we could make that
work with LEFT JOIN LATERAL.  This formulation has a different
problem, which is that if you do want most or all of the output,
computing each sub-aggregation separately is probably less
efficient than it could be.  But this is probably the better way
to go unless someone has an even better idea.

            regards, tom lane

diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index bb1ac30cd1..e319b99a9d 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -717,22 +717,31 @@ CREATE VIEW pg_statio_all_tables AS
             pg_stat_get_blocks_fetched(C.oid) -
                     pg_stat_get_blocks_hit(C.oid) AS heap_blks_read,
             pg_stat_get_blocks_hit(C.oid) AS heap_blks_hit,
-            sum(pg_stat_get_blocks_fetched(I.indexrelid) -
-                    pg_stat_get_blocks_hit(I.indexrelid))::bigint AS idx_blks_read,
-            sum(pg_stat_get_blocks_hit(I.indexrelid))::bigint AS idx_blks_hit,
+            I.idx_blks_read AS idx_blks_read,
+            I.idx_blks_hit AS idx_blks_hit,
             pg_stat_get_blocks_fetched(T.oid) -
                     pg_stat_get_blocks_hit(T.oid) AS toast_blks_read,
             pg_stat_get_blocks_hit(T.oid) AS toast_blks_hit,
-            pg_stat_get_blocks_fetched(X.indexrelid) -
-                    pg_stat_get_blocks_hit(X.indexrelid) AS tidx_blks_read,
-            pg_stat_get_blocks_hit(X.indexrelid) AS tidx_blks_hit
+            X.idx_blks_read AS tidx_blks_read,
+            X.idx_blks_hit AS tidx_blks_hit
     FROM pg_class C LEFT JOIN
-            pg_index I ON C.oid = I.indrelid LEFT JOIN
-            pg_class T ON C.reltoastrelid = T.oid LEFT JOIN
-            pg_index X ON T.oid = X.indrelid
+            pg_class T ON C.reltoastrelid = T.oid
             LEFT JOIN pg_namespace N ON (N.oid = C.relnamespace)
-    WHERE C.relkind IN ('r', 't', 'm')
-    GROUP BY C.oid, N.nspname, C.relname, T.oid, X.indexrelid;
+            LEFT JOIN LATERAL (
+              SELECT sum(pg_stat_get_blocks_fetched(indexrelid) -
+                         pg_stat_get_blocks_hit(indexrelid))::bigint
+                     AS idx_blks_read,
+                     sum(pg_stat_get_blocks_hit(indexrelid))::bigint
+                     AS idx_blks_hit
+              FROM pg_index WHERE indrelid = C.oid ) I ON true
+            LEFT JOIN LATERAL (
+              SELECT sum(pg_stat_get_blocks_fetched(indexrelid) -
+                         pg_stat_get_blocks_hit(indexrelid))::bigint
+                     AS idx_blks_read,
+                     sum(pg_stat_get_blocks_hit(indexrelid))::bigint
+                     AS idx_blks_hit
+              FROM pg_index WHERE indrelid = T.oid ) X ON true
+    WHERE C.relkind IN ('r', 't', 'm');

 CREATE VIEW pg_statio_sys_tables AS
     SELECT * FROM pg_statio_all_tables
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index ac468568a1..dd54d71098 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -2261,19 +2261,24 @@ pg_statio_all_tables| SELECT c.oid AS relid,
     c.relname,
     (pg_stat_get_blocks_fetched(c.oid) - pg_stat_get_blocks_hit(c.oid)) AS heap_blks_read,
     pg_stat_get_blocks_hit(c.oid) AS heap_blks_hit,
-    (sum((pg_stat_get_blocks_fetched(i.indexrelid) - pg_stat_get_blocks_hit(i.indexrelid))))::bigint AS idx_blks_read,
-    (sum(pg_stat_get_blocks_hit(i.indexrelid)))::bigint AS idx_blks_hit,
+    i.idx_blks_read,
+    i.idx_blks_hit,
     (pg_stat_get_blocks_fetched(t.oid) - pg_stat_get_blocks_hit(t.oid)) AS toast_blks_read,
     pg_stat_get_blocks_hit(t.oid) AS toast_blks_hit,
-    (pg_stat_get_blocks_fetched(x.indexrelid) - pg_stat_get_blocks_hit(x.indexrelid)) AS tidx_blks_read,
-    pg_stat_get_blocks_hit(x.indexrelid) AS tidx_blks_hit
+    x.idx_blks_read AS tidx_blks_read,
+    x.idx_blks_hit AS tidx_blks_hit
    FROM ((((pg_class c
-     LEFT JOIN pg_index i ON ((c.oid = i.indrelid)))
      LEFT JOIN pg_class t ON ((c.reltoastrelid = t.oid)))
-     LEFT JOIN pg_index x ON ((t.oid = x.indrelid)))
      LEFT JOIN pg_namespace n ON ((n.oid = c.relnamespace)))
-  WHERE (c.relkind = ANY (ARRAY['r'::"char", 't'::"char", 'm'::"char"]))
-  GROUP BY c.oid, n.nspname, c.relname, t.oid, x.indexrelid;
+     LEFT JOIN LATERAL ( SELECT (sum((pg_stat_get_blocks_fetched(pg_index.indexrelid) -
pg_stat_get_blocks_hit(pg_index.indexrelid))))::bigintAS idx_blks_read, 
+            (sum(pg_stat_get_blocks_hit(pg_index.indexrelid)))::bigint AS idx_blks_hit
+           FROM pg_index
+          WHERE (pg_index.indrelid = c.oid)) i ON (true))
+     LEFT JOIN LATERAL ( SELECT (sum((pg_stat_get_blocks_fetched(pg_index.indexrelid) -
pg_stat_get_blocks_hit(pg_index.indexrelid))))::bigintAS idx_blks_read, 
+            (sum(pg_stat_get_blocks_hit(pg_index.indexrelid)))::bigint AS idx_blks_hit
+           FROM pg_index
+          WHERE (pg_index.indrelid = t.oid)) x ON (true))
+  WHERE (c.relkind = ANY (ARRAY['r'::"char", 't'::"char", 'm'::"char"]));
 pg_statio_sys_indexes| SELECT pg_statio_all_indexes.relid,
     pg_statio_all_indexes.indexrelid,
     pg_statio_all_indexes.schemaname,

I wrote:
> After a bit of further fooling, I found that we could make that
> work with LEFT JOIN LATERAL.  This formulation has a different
> problem, which is that if you do want most or all of the output,
> computing each sub-aggregation separately is probably less
> efficient than it could be.  But this is probably the better way
> to go unless someone has an even better idea.

Hearing no better ideas, pushed.

            regards, tom lane

Hi Tom!
On Thu, 2022-03-24 at 16:34 -0400, Tom Lane wrote:
> I wrote:
> > After a bit of further fooling, I found that we could make that
> > work with LEFT JOIN LATERAL.  This formulation has a different
> > problem, which is that if you do want most or all of the output,
> > computing each sub-aggregation separately is probably less
> > efficient than it could be.  But this is probably the better way
> > to go unless someone has an even better idea.
> 
> Hearing no better ideas, pushed.
> 
>                         regards, tom lane

Thank you for your attention and for the problem resolution. However
I'm worry a little about possible performance issues related to
monitoring solutions performing regular sampling of statistic views to
find out the most intensive objects in a database. They obviously will
query all rows from statistic views and their impact will only depend
on the sampling frequency. Is it seems reasonable to avoid using
pg_statio_all_tables view by such monitoring tools? But it seems that
the only way to do so is using statistic functions directly in a
sampling query. Is it seems reliable? Maybe we should think about a
little bit different statio view for that? For example, a plain view
for all tables (regular and TOASTs)...
-- 
Regards, Andrei Zubkov

Andrei Zubkov <zubkov@moonset.ru> writes:
> Thank you for your attention and for the problem resolution. However
> I'm worry a little about possible performance issues related to
> monitoring solutions performing regular sampling of statistic views to
> find out the most intensive objects in a database.

There's no actual evidence that the new formulation is meaningfully
worse for such cases.  Sure, it's probably *somewhat* less efficient,
but that could easily be swamped by pgstat or other costs.  I wouldn't
care to worry about this unless some evidence is presented that we've
created a big problem.

            regards, tom lane