On 2018-11-21 23:45:01 -0300, Alvaro Herrera wrote:
> On 2018-Nov-22, Michael Paquier wrote:
>
> > On Thu, Nov 22, 2018 at 10:56:39AM +0900, Masahiko Sawada wrote:
> > > On Thu, Nov 22, 2018 at 10:43 AM Thomas Munro <thomas.munro@enterprisedb.com> wrote:
> > >> Presumably you could add your own call to __gcov_flush() in
> > >> quickdie(), so that we get GCOV data but no other atexit()-like stuff.
> > >> I see that some people advocate doing that in signal handlers, but I
> > >> don't know if it's really safe. If that is somehow magically OK,
> > >> you'd probably also need the chdir() hack from proc_exit() to get
> > >> per-pid files.
> > >
> > > That's probably a good idea, I'm also not sure if it's really safe
> > > though. An alternative approach could be that we can do $node->restart
> > > after recovered from $node->teardown_node() to write gcda file surely,
> > > although it would make the tests hard to read.
> >
> > Thanks for looking at the details around that. I'd prefer much if we
> > have a solution like what's outline here because we should really try to
> > have coverage even for code paths which involve an immediate shutdown
> > (mainly for recovery). Manipulating the tests to get a better coverage
> > feels more like a band-aid solution, and does not help folks with custom
> > TAP tests in their plugins.
>
> On the contrary, I think we shouldn't mess with the exit sequence.
> Today we have three shutdown modes -- smart, fast, immediate. If we add
> stuff to the exit sequence of the immediate mode, we have four shutdown
> modes: those three, plus an actual server crash which would be different
> from immediate. I'd rather not do that, because we'll then grow a
> totally untested code path.
>
> Anyway I now think this problem can be fixed by careful changing of
> teardown_node() into stop('fast') in some places. The places for which
> it actually matters that a shutdown is immediate are not really
> interested with the code that executes in the server that shuts down --
> they are interested in the code run by the server that *doesn't* shut
> down (the replica), or the server after it restarts (and which we can
> shut down cleanly afterwards). No need to mess with the backend exit
> code path ISTM.
I don't find this particularly convincing. Coverage ought to indicate
code coverage, not some random subset based on what kind of shutdown
mode testing uses.
Yes, it's not particularly safe to do things during an immediate
shutdown, but doing so only when computing coverage, e.g. when some
environment variable is set, seems pretty reasonable. The likelihood of
regular failures due to that seem exceedingly slim.
Greetings,
Andres Freund