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
