Feedback-ID: id4a34324:Fastmail
Date: Wed, 13 Sep 2023 11:59:39 -0700
From: Andres Freund <andres@anarazel.de>
To: Karina Litskevich <litskevichkarina@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
	Nazir Bilal Yavuz <byavuz81@gmail.com>
Subject: Re: BufferUsage counters' values have changed
Message-ID: <20230913185939.zt62s6p5o36yup3o@awork3.anarazel.de>
References: 
 <CACiT8ibxXA6+0amGikbeFhm8B84XdQVo6D0Qfd1pQ1s8zpsnxQ@mail.gmail.com>
 <CACiT8iZvy5W8DzEd3WJimhxK-9==gLhUjotZ+Ooun3Vs3UCT6w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="nzwecxcpdlcarucj"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CACiT8iZvy5W8DzEd3WJimhxK-9==gLhUjotZ+Ooun3Vs3UCT6w@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20230913185939.zt62s6p5o36yup3o%40awork3.anarazel.de>
Precedence: bulk


--nzwecxcpdlcarucj
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

Hi,

On 2023-09-11 09:23:59 +0300, Karina Litskevich wrote:
> On Mon, Sep 11, 2023 at 9:08 AM Karina Litskevich <
> litskevichkarina@gmail.com> wrote:
> 
> > I found a bug that causes one of the differences. Wrong counter is
> > incremented
> > in ExtendBufferedRelLocal(). The attached patch fixes it and should be
> > applied
> > to REL_16_STABLE and master.
> >
> 
>  I've forgotten to attach the patch. Here it is.

> From 999a3d533a9b74c8568cc8a3d715c287de45dd2c Mon Sep 17 00:00:00 2001
> From: Karina Litskevich <litskevichkarina@gmail.com>
> Date: Thu, 7 Sep 2023 17:44:40 +0300
> Subject: [PATCH v1] Fix local_blks_written counter incrementation
> 
> ---
>  src/backend/storage/buffer/localbuf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c
> index 1735ec7141..567b8d15ef 100644
> --- a/src/backend/storage/buffer/localbuf.c
> +++ b/src/backend/storage/buffer/localbuf.c
> @@ -431,7 +431,7 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr,
>  
>  	*extended_by = extend_by;
>  
> -	pgBufferUsage.temp_blks_written += extend_by;
> +	pgBufferUsage.local_blks_written += extend_by;
>  
>  	return first_block;
>  }
> -- 
> 2.34.1
> 

Ugh, you're right.

The naming of local vs temp here is pretty unfortunate imo. I wonder if we
ought to at least dd a comment to BufferUsage clarifying the situation? Just
reading the comments therein one would be hard pressed to figure out which of
the variables temp table activity should be added to.

I don't think we currently can write a test for this in the core tests, as the
relevant data isn't visible anywhere, iirc. Thus I added a test to
pg_stat_statements. Afaict it should be stable?

Running the attached patch through CI, planning to push after that succeeds,
unless somebody has a comment?

Greetings,

Andres Freund

--nzwecxcpdlcarucj
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0001-Fix-tracking-of-temp-table-relation-extensions-as.patch"

From ae7b170e9032af0fd257a39953fe0b6ef7e9637c Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Wed, 13 Sep 2023 11:46:45 -0700
Subject: [PATCH v2] Fix tracking of temp table relation extensions as writes

Karina figured out that I (Andres) confused BufferUsage.temp_blks_written with
BufferUsage.local_blks_written in fcdda1e4b5.

Tests in core PG can't easily test this, as BufferUsage is just used for
EXPLAIN (ANALYZE, BUFFERS) and pg_stat_statements. Thus this commit adds tests
for this to pg_stat_statements.

Reported-by: Karina Litskevich <litskevichkarina@gmail.com>
Author: Karina Litskevich <litskevichkarina@gmail.com>
Author: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/CACiT8ibxXA6+0amGikbeFhm8B84XdQVo6D0Qfd1pQ1s8zpsnxQ@mail.gmail.com
Backpatch: 16-, where fcdda1e4b5 was merged
---
 src/backend/storage/buffer/localbuf.c       |  2 +-
 contrib/pg_stat_statements/expected/dml.out | 27 +++++++++++++++++++++
 contrib/pg_stat_statements/sql/dml.sql      | 19 +++++++++++++++
 3 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c
index 1735ec71419..567b8d15ef0 100644
--- a/src/backend/storage/buffer/localbuf.c
+++ b/src/backend/storage/buffer/localbuf.c
@@ -431,7 +431,7 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr,
 
 	*extended_by = extend_by;
 
-	pgBufferUsage.temp_blks_written += extend_by;
+	pgBufferUsage.local_blks_written += extend_by;
 
 	return first_block;
 }
diff --git a/contrib/pg_stat_statements/expected/dml.out b/contrib/pg_stat_statements/expected/dml.out
index 7b9c8f979ee..ede47a71acc 100644
--- a/contrib/pg_stat_statements/expected/dml.out
+++ b/contrib/pg_stat_statements/expected/dml.out
@@ -139,6 +139,33 @@ SELECT calls, rows, query FROM pg_stat_statements ORDER BY query COLLATE "C";
      1 |    1 | SELECT pg_stat_statements_reset()
 (10 rows)
 
+-- check that [temp] table relation extensions are tracked as writes
+CREATE TABLE pgss_extend_tab (a int, b text);
+CREATE TEMP TABLE pgss_extend_temp_tab (a int, b text);
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset 
+--------------------------
+ 
+(1 row)
+
+INSERT INTO pgss_extend_tab (a, b) SELECT generate_series(1, 1000), 'something';
+INSERT INTO pgss_extend_temp_tab (a, b) SELECT generate_series(1, 1000), 'something';
+WITH sizes AS (
+  SELECT
+    pg_relation_size('pgss_extend_tab') / current_setting('block_size')::int8 AS rel_size,
+    pg_relation_size('pgss_extend_temp_tab') / current_setting('block_size')::int8 AS temp_rel_size
+)
+SELECT
+    SUM(local_blks_written) >= (SELECT temp_rel_size FROM sizes) AS temp_written_ok,
+    SUM(local_blks_dirtied) >= (SELECT temp_rel_size FROM sizes) AS temp_dirtied_ok,
+    SUM(shared_blks_written) >= (SELECT rel_size FROM sizes) AS written_ok,
+    SUM(shared_blks_dirtied) >= (SELECT rel_size FROM sizes) AS dirtied_ok
+FROM pg_stat_statements;
+ temp_written_ok | temp_dirtied_ok | written_ok | dirtied_ok 
+-----------------+-----------------+------------+------------
+ t               | t               | t          | t
+(1 row)
+
 SELECT pg_stat_statements_reset();
  pg_stat_statements_reset 
 --------------------------
diff --git a/contrib/pg_stat_statements/sql/dml.sql b/contrib/pg_stat_statements/sql/dml.sql
index af2f9fcf73b..3b5d2afb858 100644
--- a/contrib/pg_stat_statements/sql/dml.sql
+++ b/contrib/pg_stat_statements/sql/dml.sql
@@ -73,4 +73,23 @@ MERGE INTO pgss_dml_tab USING pgss_dml_tab st ON (st.a = pgss_dml_tab.a AND st.a
 DROP TABLE pgss_dml_tab;
 
 SELECT calls, rows, query FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- check that [temp] table relation extensions are tracked as writes
+CREATE TABLE pgss_extend_tab (a int, b text);
+CREATE TEMP TABLE pgss_extend_temp_tab (a int, b text);
+SELECT pg_stat_statements_reset();
+INSERT INTO pgss_extend_tab (a, b) SELECT generate_series(1, 1000), 'something';
+INSERT INTO pgss_extend_temp_tab (a, b) SELECT generate_series(1, 1000), 'something';
+WITH sizes AS (
+  SELECT
+    pg_relation_size('pgss_extend_tab') / current_setting('block_size')::int8 AS rel_size,
+    pg_relation_size('pgss_extend_temp_tab') / current_setting('block_size')::int8 AS temp_rel_size
+)
+SELECT
+    SUM(local_blks_written) >= (SELECT temp_rel_size FROM sizes) AS temp_written_ok,
+    SUM(local_blks_dirtied) >= (SELECT temp_rel_size FROM sizes) AS temp_dirtied_ok,
+    SUM(shared_blks_written) >= (SELECT rel_size FROM sizes) AS written_ok,
+    SUM(shared_blks_dirtied) >= (SELECT rel_size FROM sizes) AS dirtied_ok
+FROM pg_stat_statements;
+
 SELECT pg_stat_statements_reset();
-- 
2.38.0


--nzwecxcpdlcarucj--