Stats sender and 2pc minor problem
Hello.
Statistics sender logic during usual commit and two-phase commit do not strictly matches each other and that leads to
delta_live_tuples added to n_live_tup in case of truncate in two phase commit.
That can be see in following example:
CREATE TABLE trunc_stats_test5(id serial);
INSERT INTO trunc_stats_test5 DEFAULT VALUES;
INSERT INTO trunc_stats_test5 DEFAULT VALUES;
INSERT INTO trunc_stats_test5 DEFAULT VALUES;
BEGIN;
TRUNCATE trunc_stats_test5;
PREPARE TRANSACTION 'twophase_stats';
COMMIT PREPARED 'twophase_stats';
After that pg_stat_user_tables will have n_live_tup = 3 instead of 0.
Fix along with test is attached.
Attachments:
2pc-stats.patchapplication/octet-stream; name=2pc-stats.patchDownload
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index a9efee8..c7dee95 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -2227,6 +2227,13 @@ pgstat_twophase_postcommit(TransactionId xid, uint16 info,
pgstat_info->t_counts.t_tuples_deleted += rec->tuples_deleted;
pgstat_info->t_counts.t_truncated = rec->t_truncated;
+ /* forget live/dead stats seen by backend thus far */
+ if (rec->t_truncated)
+ {
+ pgstat_info->t_counts.t_delta_live_tuples = 0;
+ pgstat_info->t_counts.t_delta_dead_tuples = 0;
+ }
+
pgstat_info->t_counts.t_delta_live_tuples +=
rec->tuples_inserted - rec->tuples_deleted;
pgstat_info->t_counts.t_delta_dead_tuples +=
diff --git a/src/test/regress/expected/stats.out b/src/test/regress/expected/stats.out
index a811265..e4dc333 100644
--- a/src/test/regress/expected/stats.out
+++ b/src/test/regress/expected/stats.out
@@ -84,6 +84,7 @@ CREATE TABLE trunc_stats_test1(id serial);
CREATE TABLE trunc_stats_test2(id serial);
CREATE TABLE trunc_stats_test3(id serial);
CREATE TABLE trunc_stats_test4(id serial);
+CREATE TABLE trunc_stats_test5(id serial);
-- check that n_live_tup is reset to 0 after truncate
INSERT INTO trunc_stats_test DEFAULT VALUES;
INSERT INTO trunc_stats_test DEFAULT VALUES;
@@ -129,6 +130,14 @@ INSERT INTO trunc_stats_test4 DEFAULT VALUES;
TRUNCATE trunc_stats_test4;
INSERT INTO trunc_stats_test4 DEFAULT VALUES;
ROLLBACK;
+-- truncate and two-phase commit
+INSERT INTO trunc_stats_test5 DEFAULT VALUES;
+INSERT INTO trunc_stats_test5 DEFAULT VALUES;
+INSERT INTO trunc_stats_test5 DEFAULT VALUES;
+BEGIN;
+TRUNCATE trunc_stats_test5;
+PREPARE TRANSACTION 'twophase_stats';
+COMMIT PREPARED 'twophase_stats';
-- do a seqscan
SELECT count(*) FROM tenk2;
count
@@ -169,7 +178,8 @@ SELECT relname, n_tup_ins, n_tup_upd, n_tup_del, n_live_tup, n_dead_tup
trunc_stats_test2 | 1 | 0 | 0 | 1 | 0
trunc_stats_test3 | 4 | 0 | 0 | 2 | 2
trunc_stats_test4 | 2 | 0 | 0 | 0 | 2
-(5 rows)
+ trunc_stats_test5 | 3 | 0 | 0 | 0 | 0
+(6 rows)
SELECT st.seq_scan >= pr.seq_scan + 1,
st.seq_tup_read >= pr.seq_tup_read + cl.reltuples,
@@ -198,5 +208,5 @@ FROM prevstats AS pr;
t
(1 row)
-DROP TABLE trunc_stats_test, trunc_stats_test1, trunc_stats_test2, trunc_stats_test3, trunc_stats_test4;
+DROP TABLE trunc_stats_test, trunc_stats_test1, trunc_stats_test2, trunc_stats_test3, trunc_stats_test4, trunc_stats_test5;
-- End of Stats Test
diff --git a/src/test/regress/sql/stats.sql b/src/test/regress/sql/stats.sql
index b3e2efa..46f044b 100644
--- a/src/test/regress/sql/stats.sql
+++ b/src/test/regress/sql/stats.sql
@@ -80,6 +80,7 @@ CREATE TABLE trunc_stats_test1(id serial);
CREATE TABLE trunc_stats_test2(id serial);
CREATE TABLE trunc_stats_test3(id serial);
CREATE TABLE trunc_stats_test4(id serial);
+CREATE TABLE trunc_stats_test5(id serial);
-- check that n_live_tup is reset to 0 after truncate
INSERT INTO trunc_stats_test DEFAULT VALUES;
@@ -132,6 +133,15 @@ TRUNCATE trunc_stats_test4;
INSERT INTO trunc_stats_test4 DEFAULT VALUES;
ROLLBACK;
+-- truncate and two-phase commit
+INSERT INTO trunc_stats_test5 DEFAULT VALUES;
+INSERT INTO trunc_stats_test5 DEFAULT VALUES;
+INSERT INTO trunc_stats_test5 DEFAULT VALUES;
+BEGIN;
+TRUNCATE trunc_stats_test5;
+PREPARE TRANSACTION 'twophase_stats';
+COMMIT PREPARED 'twophase_stats';
+
-- do a seqscan
SELECT count(*) FROM tenk2;
-- do an indexscan
@@ -164,5 +174,5 @@ SELECT st.heap_blks_read + st.heap_blks_hit >= pr.heap_blks + cl.relpages,
SELECT pr.snap_ts < pg_stat_get_snapshot_timestamp() as snapshot_newer
FROM prevstats AS pr;
-DROP TABLE trunc_stats_test, trunc_stats_test1, trunc_stats_test2, trunc_stats_test3, trunc_stats_test4;
+DROP TABLE trunc_stats_test, trunc_stats_test1, trunc_stats_test2, trunc_stats_test3, trunc_stats_test4, trunc_stats_test5;
-- End of Stats Test
Stas Kelvich <s.kelvich@postgrespro.ru> writes:
Statistics sender logic during usual commit and two-phase commit do not
strictly matches each other and that leads to delta_live_tuples added to
n_live_tup in case of truncate in two phase commit.
Yeah, that code says it's supposed to match AtEOXact_PgStat, but it
doesn't.
I pushed this, but without the regression test case, which would have
failed outright in any test run with max_prepared_transactions = 0.
Timing sensitivity is another problem. In the commit that created this
discrepancy, d42358efb, Alvaro had tried to add regression coverage for
this area, but we ended up backing it out because it failed too often
in the buildfarm.
TBH, now that I look at it, I think that d42358efb was fundamentally
wrong and this patch is just continuing down the same wrong path.
Having the stats collector respond to a TRUNCATE like this cannot
work reliably, because the "it got truncated" flag will arrive at
the stats collector asynchronously, perhaps quite some time later
than the truncate occurred. When that happens, we may throw away
live/dead tuple count updates from transactions that actually happened
after the truncate but chanced to report first.
I wonder if we could make that better by making the stats collector
track stats by relfilenode rather than table OID. It'd be a pretty
major logic change, though, to serve a corner case.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Tom Lane wrote:
Stas Kelvich <s.kelvich@postgrespro.ru> writes:
Statistics sender logic during usual commit and two-phase commit do not
strictly matches each other and that leads to delta_live_tuples added to
n_live_tup in case of truncate in two phase commit.Yeah, that code says it's supposed to match AtEOXact_PgStat, but it
doesn't.
Hmm, oops.
I pushed this, but without the regression test case, which would have
failed outright in any test run with max_prepared_transactions = 0.
I agree that that was the right approach. Thanks for taking care of it!
I wonder if we could make that better by making the stats collector
track stats by relfilenode rather than table OID. It'd be a pretty
major logic change, though, to serve a corner case.
Hm, that's an idea.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers