Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
> https://commitfest.postgresql.org/action/patch_view?id=1661 (apologies for not replying to the thread; I can't find
itin 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...
