Re: shared tempfile was not removed on statement_timeout

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.

- Heikki