Thread: Calling pgstat_report_wait_end() before ereport(ERROR)
Hi,
There are something like the following code in many places in PostgreSQL code.
pgstat_report_wait_start(WAIT_EVENT_xxx);
if (write(...) != len)
{
ereport(ERROR, ...);
}
pgstat_report_wait_end();
Almost of these places don't call pgstat_report_wait_end() before
ereport(ERROR) but some places. Especially in RecreateTwoPhaseFile()
we have,
/* Write content and CRC */
errno = 0;
pgstat_report_wait_start(WAIT_EVENT_TWOPHASE_FILE_WRITE);
if (write(fd, content, len) != len)
{
int save_errno = errno;
pgstat_report_wait_end();
CloseTransientFile(fd);
/* if write didn't set errno, assume problem is no disk space */
errno = save_errno ? save_errno : ENOSPC;
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not write file \"%s\": %m", path)));
}
if (write(fd, &statefile_crc, sizeof(pg_crc32c)) != sizeof(pg_crc32c))
{
int save_errno = errno;
pgstat_report_wait_end();
CloseTransientFile(fd);
/* if write didn't set errno, assume problem is no disk space */
errno = save_errno ? save_errno : ENOSPC;
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not write file \"%s\": %m", path)));
}
pgstat_report_wait_end();
/*
* We must fsync the file because the end-of-replay checkpoint will not do
* so, there being no GXACT in shared memory yet to tell it to.
*/
pgstat_report_wait_start(WAIT_EVENT_TWOPHASE_FILE_SYNC);
if (pg_fsync(fd) != 0)
{
int save_errno = errno;
CloseTransientFile(fd);
errno = save_errno;
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not fsync file \"%s\": %m", path)));
}
pgstat_report_wait_end();
First two call pgstat_report_wait_end() but third one doesn't.
As far as I know there are three places where call
pgstat_report_wait_end before ereport(ERROR): two in twophase.c
andanother in copydir.c(at L199). 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?
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
On Fri, Apr 12, 2019 at 07:27:44PM +0900, Masahiko Sawada wrote: > As far as I know there are three places where call > pgstat_report_wait_end before ereport(ERROR): two in twophase.c > andanother in copydir.c(at L199). 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? RecreateTwoPhaseFile() gets called in the checkpointer or the startup process which do not have a transaction context so the wait event would not get cleaned up, and we should call pgstat_report_wait_end() in the third elog(ERROR), no? It looks that 249cf070 has been rather inconsistent in its way of handling things. -- Michael
Attachment
On Fri, Apr 12, 2019 at 9:07 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Fri, Apr 12, 2019 at 07:27:44PM +0900, Masahiko Sawada wrote: > > As far as I know there are three places where call > > pgstat_report_wait_end before ereport(ERROR): two in twophase.c > > andanother in copydir.c(at L199). 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? > > RecreateTwoPhaseFile() gets called in the checkpointer or the startup > process which do not have a transaction context Yes. > so the wait event would not get cleaned up But I think that's not right, I've checked the code. If the startup process failed in that function it raises a FATAL and recovery fails, and if checkpointer process does then it calls pgstat_report_wait_end() in CheckpointerMain(). > It looks that 249cf070 has been rather > inconsistent in its way of handling things. Yeah, I think that at least handling of pgstat_report_wait_end() in RecreateTwoPhseFile() is inconsistent in any case. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Masahiko Sawada <sawada.mshk@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
On Fri, Apr 12, 2019 at 10:06:41PM +0900, Masahiko Sawada wrote: > But I think that's not right, I've checked the code. If the startup > process failed in that function it raises a FATAL and recovery fails, > and if checkpointer process does then it calls > pgstat_report_wait_end() in CheckpointerMain(). Well, the point is that the code raises an ERROR, then a FATAL because it gets upgraded by recovery. The take, at least it seems to me, is that if any new caller of the function misses to clean up the event then the routine gets cleared. So it seems to me that the current coding is aimed to be more defensive than anything. I agree that there is perhaps little point in doing so. In my experience a backend switches very quickly back to ClientRead, cleaning up the previous event. Looking around, we have also some code paths in slot.c and origin.c which close a transient file, clear the event flag... And then PANIC, which makes even less sense. In short, I tend to think that the attached is an acceptable cleanup. Thoughts? -- Michael
Attachment
On Fri, Apr 12, 2019 at 11:05 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Masahiko Sawada <sawada.mshk@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. > Thank you for explanation! That's really helpful for me. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
On Tue, Apr 16, 2019 at 2:45 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Fri, Apr 12, 2019 at 10:06:41PM +0900, Masahiko Sawada wrote: > > But I think that's not right, I've checked the code. If the startup > > process failed in that function it raises a FATAL and recovery fails, > > and if checkpointer process does then it calls > > pgstat_report_wait_end() in CheckpointerMain(). > > Well, the point is that the code raises an ERROR, then a FATAL because > it gets upgraded by recovery. The take, at least it seems to me, is > that if any new caller of the function misses to clean up the event > then the routine gets cleared. So it seems to me that the current > coding is aimed to be more defensive than anything. I agree that > there is perhaps little point in doing so. In my experience a backend > switches very quickly back to ClientRead, cleaning up the previous > event. Looking around, we have also some code paths in slot.c and > origin.c which close a transient file, clear the event flag... And > then PANIC, which makes even less sense. > > In short, I tend to think that the attached is an acceptable cleanup. > Thoughts? Agreed. There are also some code which raise an ERROR after close a transient file but I think it's a good idea to not include them for safety. It looks to me that the patch you proposed cleans places as much as we can do. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Michael Paquier <michael@paquier.xyz> writes:
> In short, I tend to think that the attached is an acceptable cleanup.
> Thoughts?
WFM.
regards, tom lane
On Tue, Apr 16, 2019 at 08:03:22PM +0900, Masahiko Sawada wrote: > Agreed. There are also some code which raise an ERROR after close a > transient file but I think it's a good idea to not include them for > safety. It looks to me that the patch you proposed cleans places as > much as we can do. Thanks for the lookup, committed. -- Michael