Masahiko Sawada <sawada.m...@gmail.com> writes:
> There are something like the following code in many places in PostgreSQL code.
> ...
> Since we eventually call
> pgstat_report_wait_end() in AbortTransaction(). I think that we don't
> need to call pgstat_report_wait_end() if we're going to raise an error
> just after that. Is that right?
Yes ... and those CloseTransientFile calls are unnecessary as well.
To a first approximation, *any* cleanup-type call occurring just before
an ereport(ERROR) is probably unnecessary, or if it is necessary then
the code is broken in other ways. One should not assume that there is
no other way for an error to be thrown while the resource is held, and
therefore it's generally better design to have enough infrastructure
so that the error cleanup mechanisms can handle whatever cleanup is
needed. We certainly have such infrastructure for OpenTransientFile/
CloseTransientFile, and according to what you say above (I didn't
check it) pgstat wait reporting is handled similarly. So these
call sites could all be simplified substantially.
There are exceptions to this rule of thumb. In some places, for
instance, it's worth releasing a lock before ereport simply to shorten
the length of time that the lock might stay held. And there are places
where a very low-level resource (such as a spinlock) is only held in
straight-line code so there's not really need for error cleanup
infrastructure for it. Perhaps there's an argument to be made that
pgstat wait reporting could be put in this second category, but
I doubt it.
regards, tom lane
