Re: shared tempfile was not removed on statement_timeout - Mailing list pgsql-hackers

At Tue, 26 Jan 2021 11:00:56 +0200, Heikki Linnakangas <hlinnaka@iki.fi> wrote in 
> On 26/01/2021 06:46, Kyotaro Horiguchi wrote:
> > Looking the comment of SharedFileSetOnDetach:
> > | * everything in them.  We can't raise an error on failures, because this
> > | * runs
> > | * in error cleanup paths.
> > I feel that a function that shouldn't error-out also shouldn't be
> > cancellable. If that's the case, we omit the CHECK_FOR_INTERRUPTS() in
> > walkdir() when elevel is smaller than ERROR.
> > =====
> > diff --git a/src/backend/storage/file/fd.c
> > b/src/backend/storage/file/fd.c
> > index b58502837a..593c23553e 100644
> > --- a/src/backend/storage/file/fd.c
> > +++ b/src/backend/storage/file/fd.c
> > @@ -3374,7 +3374,9 @@ walkdir(const char *path,
> >       {
> >           char        subpath[MAXPGPATH * 2];
> >   -        CHECK_FOR_INTERRUPTS();
> > +        /* omit interrupts while we shouldn't error-out */
> > +        if (elevel >= ERROR)
> > +            CHECK_FOR_INTERRUPTS();
> >             if (strcmp(de->d_name, ".") == 0 ||
> >               strcmp(de->d_name, "..") == 0)
> > =====
> 
> Don't we potentially have the same problem with all on_dsm_detach
> callbacks? Looking at the other on_dsm_detach callbacks, I don't see
> any CHECK_FOR_INTERRUPT() calls in them, but it seems fragile to
> assume that.
> 
> I'd suggest adding HOLD/RESUME_INTERRUPTS() to dsm_detach(). At least
> around the removal of the callback from the list and calling the
> callback. Maybe even over the whole function.

Yes, I first came up with HOLD/RESUME_QUERY_INTERRUPTS() to the same
location.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center