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