Re: [HACKERS] REVIEW: Track TRUNCATE via pgstat
Pushed, thanks. I reviewed the test results and concluded that the comments were wrong and the code was right, so I updated the comments to match reality. -- Álvaro Herrerahttp://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
Re: [HACKERS] REVIEW: Track TRUNCATE via pgstat
On Sat, Jan 24, 2015 at 5:58 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Here's v0.5. (Why did you use that weird decimal versioning scheme? You could just say v4 and save a couple of keystrokes). This patch makes perfect sense to me now. I was ready to commit, but I checked the regression test you added and noticed that you're only reading results for the last set of operations because they all use the same table and so each new set clobbers the values for the previous one. So I modified them to use one table for each set, and report the counters for all tables. In doing this I noticed that the one for trunc_stats_test3 is at odds with what your comment in the .sql file says; would you review it please? Thanks. (I didn't update the expected file.) BTW you forgot to update expected/prepared_xact_1.out, for the case when prep xacts are disabled. If some other committer decides to give this a go, please remember to bump catversion before pushing. Alex, this patch seems nicely backed. Could you review the changes of Alvaro? This thread is waiting for your input for 3 weeks. -- Michael
Re: [HACKERS] REVIEW: Track TRUNCATE via pgstat
Here's v0.5. (Why did you use that weird decimal versioning scheme? You
could just say v4 and save a couple of keystrokes). This patch makes
perfect sense to me now. I was ready to commit, but I checked the
regression test you added and noticed that you're only reading results
for the last set of operations because they all use the same table and
so each new set clobbers the values for the previous one. So I modified
them to use one table for each set, and report the counters for all
tables. In doing this I noticed that the one for trunc_stats_test3 is
at odds with what your comment in the .sql file says; would you review
it please? Thanks.
(I didn't update the expected file.)
BTW you forgot to update expected/prepared_xact_1.out, for the case when
prep xacts are disabled.
If some other committer decides to give this a go, please remember to
bump catversion before pushing.
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training Services
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 66d5083..b2993b8 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -71,6 +71,7 @@
#include parser/parse_type.h
#include parser/parse_utilcmd.h
#include parser/parser.h
+#include pgstat.h
#include rewrite/rewriteDefine.h
#include rewrite/rewriteHandler.h
#include rewrite/rewriteManip.h
@@ -1220,6 +1221,8 @@ ExecuteTruncate(TruncateStmt *stmt)
*/
reindex_relation(heap_relid, REINDEX_REL_PROCESS_TOAST);
}
+
+ pgstat_count_truncate(rel);
}
/*
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 827ad71..afe1da2 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -197,8 +197,12 @@ typedef struct TwoPhasePgStatRecord
PgStat_Counter tuples_inserted; /* tuples inserted in xact */
PgStat_Counter tuples_updated; /* tuples updated in xact */
PgStat_Counter tuples_deleted; /* tuples deleted in xact */
+ PgStat_Counter inserted_pre_trunc; /* tuples inserted prior to truncate */
+ PgStat_Counter updated_pre_trunc; /* tuples updated prior to truncate */
+ PgStat_Counter deleted_pre_trunc; /* tuples deleted prior to truncate */
Oid t_id; /* table's OID */
bool t_shared; /* is it a shared catalog? */
+ bool t_truncated; /* was the relation truncated? */
} TwoPhasePgStatRecord;
/*
@@ -1859,6 +1863,59 @@ pgstat_count_heap_delete(Relation rel)
}
/*
+ * pgstat_table_record_truncate - remember i/u/d counts accumulated so far
+ */
+static void
+pgstat_table_record_truncate(PgStat_TableXactStatus *trans)
+{
+ if (!trans-truncated)
+ {
+ trans-inserted_pre_trunc = trans-tuples_inserted;
+ trans-updated_pre_trunc = trans-tuples_updated;
+ trans-deleted_pre_trunc = trans-tuples_deleted;
+ }
+}
+
+/*
+ * pgstat_table_restore_truncate - restore counters when a truncate aborts
+ */
+static void
+pgstat_table_restore_truncate(PgStat_TableXactStatus *trans)
+{
+ if (trans-truncated)
+ {
+ trans-tuples_inserted = trans-inserted_pre_trunc;
+ trans-tuples_updated = trans-updated_pre_trunc;
+ trans-tuples_deleted = trans-deleted_pre_trunc;
+ }
+}
+
+/*
+ * pgstat_count_truncate - update tuple counters due to truncate
+ */
+void
+pgstat_count_truncate(Relation rel)
+{
+ PgStat_TableStatus *pgstat_info = rel-pgstat_info;
+
+ if (pgstat_info != NULL)
+ {
+ /* We have to log the effect at the proper transactional level */
+ int nest_level = GetCurrentTransactionNestLevel();
+
+ if (pgstat_info-trans == NULL ||
+ pgstat_info-trans-nest_level != nest_level)
+ add_tabstat_xact_level(pgstat_info, nest_level);
+
+ pgstat_table_record_truncate(pgstat_info-trans);
+ pgstat_info-trans-truncated = true;
+ pgstat_info-trans-tuples_inserted = 0;
+ pgstat_info-trans-tuples_updated = 0;
+ pgstat_info-trans-tuples_deleted = 0;
+ }
+}
+
+/*
* pgstat_update_heap_dead_tuples - update dead-tuples count
*
* The semantics of this are that we are reporting the nontransactional
@@ -1916,12 +1973,22 @@ AtEOXact_PgStat(bool isCommit)
Assert(trans-upper == NULL);
tabstat = trans-parent;
Assert(tabstat-trans == trans);
+ /* restore pre-truncate stats (if any) in case of aborted xact */
+ if (!isCommit)
+pgstat_table_restore_truncate(trans);
/* count attempted actions regardless of commit/abort */
tabstat-t_counts.t_tuples_inserted += trans-tuples_inserted;
tabstat-t_counts.t_tuples_updated += trans-tuples_updated;
tabstat-t_counts.t_tuples_deleted += trans-tuples_deleted;
if (isCommit)
{
+tabstat-t_counts.t_truncated = trans-truncated;
+if (trans-truncated)
+{
+ /* forget live/dead stats seen by backend thus far */
+ tabstat-t_counts.t_delta_live_tuples = 0;
+ tabstat-t_counts.t_delta_dead_tuples = 0;
+}
/* insert adds a live tuple, delete removes one */
tabstat-t_counts.t_delta_live_tuples +=
Re: [HACKERS] REVIEW: Track TRUNCATE via pgstat
Alvaro Herrera alvhe...@2ndquadrant.com writes:
Even with the current approach of checking the stats after every
isolated case it's sometimes takes quite a little more than a glance
to verify correctness due to side-effects of rollback (ins/upd/del
counters are still updated), and the way stats are affecting the dead
tuples counter.
Honestly I think pg_regress is not the right tool to test stat counter
updates. It kind-of works today, but only because we don't stress it
too much. If you want to create a new test framework for pgstats, and
include some tests for truncate, be my guest.
OK, I think I have now all bases covered, though the updated patch is
not that pretty.
The problem is that we don't know in advance if the (sub)transaction is
going to succeed or abort, and in case of aborted truncate we need to
use the stats gathered prior to truncate. Thus the need to track
insert/update/deletes that happened before first truncate separately.
To the point of making a dedicated pgstat testing tool: let's have
another TODO item?
--
Alex
From 0b3161191a3ddb999cd9d0da08e1b6088ce07a84 Mon Sep 17 00:00:00 2001
From: Alex Shulgin a...@commandprompt.com
Date: Tue, 9 Dec 2014 16:35:14 +0300
Subject: [PATCH] WIP: track TRUNCATEs in pgstat transaction stats.
The n_live_tup and n_dead_tup counters need to be set to 0 after a
TRUNCATE on the relation. We can't issue a special message to the stats
collector because the xact might be later aborted, so we track the fact
that the relation was truncated during the xact (and reset this xact's
insert/update/delete counters). When xact is committed, we use the
`truncated' flag to reset the n_live_tup and n_dead_tup counters.
---
src/backend/commands/tablecmds.c | 3 +
src/backend/postmaster/pgstat.c | 98 ++--
src/include/pgstat.h | 14 ++--
src/test/regress/expected/prepared_xacts.out | 50 ++
src/test/regress/expected/stats.out | 63 ++
src/test/regress/sql/prepared_xacts.sql | 27
src/test/regress/sql/stats.sql | 68 +++
7 files changed, 315 insertions(+), 8 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
new file mode 100644
index 1e737a0..4f0e3d8
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***
*** 71,76
--- 71,77
#include parser/parse_type.h
#include parser/parse_utilcmd.h
#include parser/parser.h
+ #include pgstat.h
#include rewrite/rewriteDefine.h
#include rewrite/rewriteHandler.h
#include rewrite/rewriteManip.h
*** ExecuteTruncate(TruncateStmt *stmt)
*** 1224,1229
--- 1225,1232
*/
reindex_relation(heap_relid, REINDEX_REL_PROCESS_TOAST);
}
+
+ pgstat_count_heap_truncate(rel);
}
/*
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
new file mode 100644
index f71fdeb..9d19cf9
*** a/src/backend/postmaster/pgstat.c
--- b/src/backend/postmaster/pgstat.c
*** typedef struct TwoPhasePgStatRecord
*** 199,206
--- 199,210
PgStat_Counter tuples_inserted; /* tuples inserted in xact */
PgStat_Counter tuples_updated; /* tuples updated in xact */
PgStat_Counter tuples_deleted; /* tuples deleted in xact */
+ PgStat_Counter inserted_pre_trunc; /* tuples inserted prior to truncate */
+ PgStat_Counter updated_pre_trunc; /* tuples updated prior to truncate */
+ PgStat_Counter deleted_pre_trunc; /* tuples deleted prior to truncate */
Oid t_id; /* table's OID */
bool t_shared; /* is it a shared catalog? */
+ bool t_truncated; /* was the relation truncated? */
} TwoPhasePgStatRecord;
/*
*** pgstat_count_heap_delete(Relation rel)
*** 1863,1868
--- 1867,1919
}
}
+ static void
+ record_xact_truncate_stats(PgStat_TableXactStatus *trans)
+ {
+ if (!trans-truncated)
+ {
+ trans-inserted_pre_trunc = trans-tuples_inserted;
+ trans-updated_pre_trunc = trans-tuples_updated;
+ trans-deleted_pre_trunc = trans-tuples_deleted;
+ }
+ }
+
+ static void
+ restore_xact_truncate_stats(PgStat_TableXactStatus *trans)
+ {
+ if (trans-truncated)
+ {
+ trans-tuples_inserted = trans-inserted_pre_trunc;
+ trans-tuples_updated = trans-updated_pre_trunc;
+ trans-tuples_deleted = trans-deleted_pre_trunc;
+ }
+ }
+
+ /*
+ * pgstat_count_heap_truncate - update tuple counters due to truncate
+ */
+ void
+ pgstat_count_heap_truncate(Relation rel)
+ {
+ PgStat_TableStatus *pgstat_info = rel-pgstat_info;
+
+ if (pgstat_info != NULL)
+ {
+ /* We have to log the effect at the proper transactional level */
+ int nest_level = GetCurrentTransactionNestLevel();
+
+ if (pgstat_info-trans == NULL ||
+ pgstat_info-trans-nest_level != nest_level)
+ add_tabstat_xact_level(pgstat_info, nest_level);
+
+ record_xact_truncate_stats(pgstat_info-trans);
Re: [HACKERS] REVIEW: Track TRUNCATE via pgstat
Alex Shulgin wrote: OK, I think I have now all bases covered, though the updated patch is not that pretty. The problem is that we don't know in advance if the (sub)transaction is going to succeed or abort, and in case of aborted truncate we need to use the stats gathered prior to truncate. Thus the need to track insert/update/deletes that happened before first truncate separately. Ugh, this is messy indeed. I grant that TRUNCATE is a tricky case to handle correctly, so some complexity is expected. Can you please explain in detail how this works? To the point of making a dedicated pgstat testing tool: let's have another TODO item? Sure. -- Álvaro Herrerahttp://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
Re: [HACKERS] REVIEW: Track TRUNCATE via pgstat
Alvaro Herrera alvhe...@2ndquadrant.com writes: Alex Shulgin wrote: OK, I think I have now all bases covered, though the updated patch is not that pretty. The problem is that we don't know in advance if the (sub)transaction is going to succeed or abort, and in case of aborted truncate we need to use the stats gathered prior to truncate. Thus the need to track insert/update/deletes that happened before first truncate separately. Ugh, this is messy indeed. I grant that TRUNCATE is a tricky case to handle correctly, so some complexity is expected. Can you please explain in detail how this works? The main idea is that aborted transaction can leave dead tuples behind (that is every insert and update), but when TRUNCATE is issued we need to reset insert/update/delete counters to 0: otherwise we won't get accurate live and dead counts at commit time. If the transaction that issued TRUNCATE is instead aborted, the insert/update counters that we were incrementing *after* truncate are not relevant to accurate calculation of dead tuples in the original relfilenode we are now back to due to abort. We need the insert/updates counts that happened *before* the first TRUNCATE, hence the need for separate counters. To the point of making a dedicated pgstat testing tool: let's have another TODO item? Sure. Added one. -- Alex -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] REVIEW: Track TRUNCATE via pgstat
Jim Nasby jim.na...@bluetreble.com writes:
https://commitfest.postgresql.org/action/patch_view?id=1661 (apologies for
not replying to the thread; I can't find it in my inbox)
Patch applies and passes make check. Code formatting looks good.
Jim,
The regression test partially tests this. It does not cover 2PC, nor
does it test rolling back a subtransaction that contains a
truncate. The latter actually doesn't work correctly.
Thanks for pointing out the missing 2PC test, I've added one.
The test you've added for rolling back a subxact actually works
correctly, if you consider the fact that aborted (sub)xacts still
account for insert/update/delete in pgstat. I've added this test with
the corrected expected results.
The test also adds 2.5 seconds of forced pg_sleep. I think that's both
bad and unnecessary. When I removed the sleeps I still saw times of
less than 0.1 seconds.
Well, I never liked that part, but the stats don't get updated if we
don't put the session to sleep for at least PGSTAT_STAT_INTERVAL (which
is 500ms).
Removing these extra sleep calls would theoretically not make a
difference as wait_for_trunc_test_stats() seems to have enough sleep
calls itself, but due to the pgstat_report_stat() being called from the
main loop only, there's no way short of placing the explicit pg_sleep at
top level, if we want to be able to check the effects reproducibly.
Another idea would be exposing pgstat_report_stat(true) at SQL level.
That would eleminate the need for explicit pg_sleep(=0.5), but we'll
still need the wait_for_... call to make sure the collector has picked
it up.
Also, wait_for_trunc_test_stats() should display something if it times
out; otherwise you'll have a test failure and won't have any
indication why.
Oh, good catch. Since I've copied this function from stats.sql, we
might want to update that one too in a separate commit.
I've attached a patch that adds logging on timeout and contains a test
case that demonstrates the rollback to savepoint bug.
I'm attaching the updated patch version.
Thank you for the review!
--
Alex
PS: re: your CF comment: I'm producing the patches using
git format-patch --ext-diff
where diff.external is set to '/bin/bash src/tools/git-external-diff'.
Now that I try to apply it using git, looks like git doesn't like the
copied context diff very much...
From cc51823a01a194ef6fcd90bc763fa26498837322 Mon Sep 17 00:00:00 2001
From: Alex Shulgin a...@commandprompt.com
Date: Tue, 9 Dec 2014 16:35:14 +0300
Subject: [PATCH] WIP: track TRUNCATEs in pgstat transaction stats.
The n_live_tup and n_dead_tup counters need to be set to 0 after a
TRUNCATE on the relation. We can't issue a special message to the stats
collector because the xact might be later aborted, so we track the fact
that the relation was truncated during the xact (and reset this xact's
insert/update/delete counters). When xact is committed, we use the
`truncated' flag to reset the n_live_tup and n_dead_tup counters.
---
src/backend/commands/tablecmds.c | 3 +
src/backend/postmaster/pgstat.c | 52 -
src/include/pgstat.h | 3 +
src/test/regress/expected/prepared_xacts.out | 50
src/test/regress/expected/truncate.out | 164 +++
src/test/regress/sql/prepared_xacts.sql | 27 +
src/test/regress/sql/truncate.sql| 118 +++
7 files changed, 414 insertions(+), 3 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
new file mode 100644
index 1e737a0..4f0e3d8
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***
*** 71,76
--- 71,77
#include parser/parse_type.h
#include parser/parse_utilcmd.h
#include parser/parser.h
+ #include pgstat.h
#include rewrite/rewriteDefine.h
#include rewrite/rewriteHandler.h
#include rewrite/rewriteManip.h
*** ExecuteTruncate(TruncateStmt *stmt)
*** 1224,1229
--- 1225,1232
*/
reindex_relation(heap_relid, REINDEX_REL_PROCESS_TOAST);
}
+
+ pgstat_count_heap_truncate(rel);
}
/*
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
new file mode 100644
index f71fdeb..b02e4a1
*** a/src/backend/postmaster/pgstat.c
--- b/src/backend/postmaster/pgstat.c
*** typedef struct TwoPhasePgStatRecord
*** 201,206
--- 201,207
PgStat_Counter tuples_deleted; /* tuples deleted in xact */
Oid t_id; /* table's OID */
bool t_shared; /* is it a shared catalog? */
+ bool t_truncated; /* was the relation truncated? */
} TwoPhasePgStatRecord;
/*
*** pgstat_count_heap_delete(Relation rel)
*** 1864,1869
--- 1865,1894
}
/*
+ * pgstat_count_heap_truncate - update tuple counters due to truncate
+ */
+ void
+ pgstat_count_heap_truncate(Relation rel)
+ {
+ PgStat_TableStatus *pgstat_info =
Re: [HACKERS] REVIEW: Track TRUNCATE via pgstat
Alex Shulgin wrote: Jim Nasby jim.na...@bluetreble.com writes: The test also adds 2.5 seconds of forced pg_sleep. I think that's both bad and unnecessary. When I removed the sleeps I still saw times of less than 0.1 seconds. Well, I never liked that part, but the stats don't get updated if we don't put the session to sleep for at least PGSTAT_STAT_INTERVAL (which is 500ms). Removing these extra sleep calls would theoretically not make a difference as wait_for_trunc_test_stats() seems to have enough sleep calls itself, but due to the pgstat_report_stat() being called from the main loop only, there's no way short of placing the explicit pg_sleep at top level, if we want to be able to check the effects reproducibly. Another idea would be exposing pgstat_report_stat(true) at SQL level. That would eleminate the need for explicit pg_sleep(=0.5), but we'll still need the wait_for_... call to make sure the collector has picked it up. We already have a stats test that sleeps. Why not add this stuff there, to avoid making another test slow? I agree that tests that sleep are annoying. (Try running the timeout isolation test a few times and you'll see what I mean.) -- Álvaro Herrerahttp://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
Re: [HACKERS] REVIEW: Track TRUNCATE via pgstat
Alvaro Herrera alvhe...@2ndquadrant.com writes: Another idea would be exposing pgstat_report_stat(true) at SQL level. That would eleminate the need for explicit pg_sleep(=0.5), but we'll still need the wait_for_... call to make sure the collector has picked it up. We already have a stats test that sleeps. Why not add this stuff there, to avoid making another test slow? Well, if we could come up with a set of statements to test that would produce the end result unambigously, so that we can be certain the stats we check at the end cannot be a result of neat interaction of buggy behavior... I'm not sure this is at all possible, but I know for sure it will make debugging the possible fails harder. Even with the current approach of checking the stats after every isolated case it's sometimes takes quite a little more than a glance to verify correctness due to side-effects of rollback (ins/upd/del counters are still updated), and the way stats are affecting the dead tuples counter. I'll try to see if the checks can be converged though. -- Alex -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] REVIEW: Track TRUNCATE via pgstat
Alex Shulgin wrote: Alvaro Herrera alvhe...@2ndquadrant.com writes: Another idea would be exposing pgstat_report_stat(true) at SQL level. That would eleminate the need for explicit pg_sleep(=0.5), but we'll still need the wait_for_... call to make sure the collector has picked it up. We already have a stats test that sleeps. Why not add this stuff there, to avoid making another test slow? Well, if we could come up with a set of statements to test that would produce the end result unambigously, so that we can be certain the stats we check at the end cannot be a result of neat interaction of buggy behavior... It is always possible that things work just right because two bugs cancel each other. I'm not sure this is at all possible, but I know for sure it will make debugging the possible fails harder. Surely if some other patch introduces a failure, nobody will try to debug it by looking only at the failures of this test. Even with the current approach of checking the stats after every isolated case it's sometimes takes quite a little more than a glance to verify correctness due to side-effects of rollback (ins/upd/del counters are still updated), and the way stats are affecting the dead tuples counter. Honestly I think pg_regress is not the right tool to test stat counter updates. It kind-of works today, but only because we don't stress it too much. If you want to create a new test framework for pgstats, and include some tests for truncate, be my guest. -- Álvaro Herrerahttp://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
Re: [HACKERS] REVIEW: Track TRUNCATE via pgstat
Alvaro Herrera alvhe...@2ndquadrant.com writes: Alex Shulgin wrote: Alvaro Herrera alvhe...@2ndquadrant.com writes: Another idea would be exposing pgstat_report_stat(true) at SQL level. That would eleminate the need for explicit pg_sleep(=0.5), but we'll still need the wait_for_... call to make sure the collector has picked it up. We already have a stats test that sleeps. Why not add this stuff there, to avoid making another test slow? Well, if we could come up with a set of statements to test that would produce the end result unambigously, so that we can be certain the stats we check at the end cannot be a result of neat interaction of buggy behavior... It is always possible that things work just right because two bugs cancel each other. I'm not sure this is at all possible, but I know for sure it will make debugging the possible fails harder. Surely if some other patch introduces a failure, nobody will try to debug it by looking only at the failures of this test. Even with the current approach of checking the stats after every isolated case it's sometimes takes quite a little more than a glance to verify correctness due to side-effects of rollback (ins/upd/del counters are still updated), and the way stats are affecting the dead tuples counter. Honestly I think pg_regress is not the right tool to test stat counter updates. It kind-of works today, but only because we don't stress it too much. If you want to create a new test framework for pgstats, and include some tests for truncate, be my guest. Yes, these are all good points. Actually, moving the test to stats.sql helped me realize the current patch behavior is not strictly correct: there's a corner case when you insert/update before truncate in transaction, which is later aborted. I need to take a closer look. -- Alex -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] REVIEW: Track TRUNCATE via pgstat
https://commitfest.postgresql.org/action/patch_view?id=1661 (apologies for not replying to the thread; I can't find it in my inbox) Patch applies and passes make check. Code formatting looks good. The regression test partially tests this. It does not cover 2PC, nor does it test rolling back a subtransaction that contains a truncate. The latter actually doesn't work correctly. The test also adds 2.5 seconds of forced pg_sleep. I think that's both bad and unnecessary. When I removed the sleeps I still saw times of less than 0.1 seconds. Also, wait_for_trunc_test_stats() should display something if it times out; otherwise you'll have a test failure and won't have any indication why. I've attached a patch that adds logging on timeout and contains a test case that demonstrates the rollback to savepoint bug. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com From 973dbe11b796395641dd3947658508ad68aebda5 Mon Sep 17 00:00:00 2001 From: Jim Nasby jim.na...@bluetreble.com Date: Mon, 15 Dec 2014 18:18:40 -0600 Subject: [PATCH] Show broken rollback case Warn if stats update doesn't happen. Add test that shows broken stats counts when rolling back a subtrans with a truncate. --- src/test/regress/sql/truncate.sql | 18 ++ 1 file changed, 18 insertions(+) diff --git a/src/test/regress/sql/truncate.sql b/src/test/regress/sql/truncate.sql index c912345..f5a0e7a 100644 --- a/src/test/regress/sql/truncate.sql +++ b/src/test/regress/sql/truncate.sql @@ -251,6 +251,9 @@ begin perform pg_stat_clear_snapshot(); end loop; + IF NOT updated THEN +RAISE WARNING 'stats update never happened'; + END IF; TRUNCATE prevstats; -- what a pun INSERT INTO prevstats SELECT newstats.*; @@ -311,5 +314,20 @@ COMMIT; SELECT pg_sleep(0.5); SELECT * FROM wait_for_trunc_test_stats(); +-- now to use a savepoint: this should only count 2 inserts and have 3 +-- live tuples after commit +BEGIN; +INSERT INTO trunc_stats_test DEFAULT VALUES; +INSERT INTO trunc_stats_test DEFAULT VALUES; +SAVEPOINT p1; +INSERT INTO trunc_stats_test DEFAULT VALUES; +INSERT INTO trunc_stats_test DEFAULT VALUES; +TRUNCATE trunc_stats_test; +INSERT INTO trunc_stats_test DEFAULT VALUES; +ROLLBACK TO SAVEPOINT p1; +COMMIT; + +SELECT * FROM wait_for_trunc_test_stats(); + DROP TABLE prevstats CASCADE; DROP TABLE trunc_stats_test; -- 2.1.2 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
