From: | Alex Shulgin <ash(at)commandprompt(dot)com> |
---|---|
To: | Jim Nasby <Jim(dot)Nasby(at)BlueTreble(dot)com> |
Cc: | Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: REVIEW: Track TRUNCATE via pgstat |
Date: | 2014-12-16 13:48:32 |
Message-ID: | 87bnn3yabz.fsf@commandprompt.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Jim Nasby <Jim(dot)Nasby(at)BlueTreble(dot)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...
Attachment | Content-Type | Size |
---|---|---|
truncate-and-pgstat-v0.3.patch | text/x-diff | 19.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Alvaro Herrera | 2014-12-16 14:01:57 | Re: REVIEW: Track TRUNCATE via pgstat |
Previous Message | David Fetter | 2014-12-16 13:44:49 | Re: Commitfest problems |