Re: [Tarantool-patches] [PATCH 2/2] sql: update temporary file name format

[Sergey Ostanevich <sergos@tarantool.org>]

Thanks for the patch!

I tend to the 1st alternative, although the code using the name generated
is hairy. I believe the same resolution as for the first part: if we’re
in a rush - LGTM, better solution is desirable otherwise.

Sergos
 


> On 8 Dec 2020, at 22:59, Leonid Vasiliev <lvasiliev@tarantool.org> wrote:
> 
> The bug was consisted in fail when working with temporary files
> created by VDBE to sort large result of a `SELECT` statement with
> `ORDER BY`, `GROUP BY` clauses.
> 
> Whats happen (step by step):
> - We have two instances on one node (sharded cluster).
> - A query is created that executes on both.
> - The first instance creates the name of the temporary file and
>  checks a file with such name on existence.
> - The second instance creates the name of the temporary file
>  (the same as in  first instance) and checks a file with such name
>  on existence.
> - The first instance creates a file with the `SQL_OPEN_DELETEONCLOSE`
>  flag.
> - The second instance opens(try to open) the same file.
> - The first instance closes (and removes) the temporary file.
> - The second instance tries to work with the file and fails.
> 
> Why did it happen:
> The temporary file name format has a random part, but the random
> generator uses a fixed seed.
> 
> When it was decided to use a fixed seed:
> 32cb1ad298b2b55d8536a85bdfb3827c8c8739e1
> 
> How the patch fixes the problem:
> The patch injects the PID in the temporary file name format.
> The generated name is unique for a single process (due to a random part)
> and unique between processes (due to the PID part).
> 
> Alternatives:
> 1) Use `O_TMPFILE` or `tmpfile()` (IMHO the best way to work with
>  temporary files). In both cases, we need to update a significant
>  part of the code, and some degradation can be added. It's hard to
>  review.
> 2) Return a random seed for the generator. As far as I understand,
>  we want to have good reproducible system behavior, in which case
>  it's good to use a fixed seed.
> 3) Add reopening file with the flags `O_CREAT | O_EXCL` until we
>  win the fight. Now we set such flags when opening a temporary file,
>  but after that we try to open the file in `READONLY` mode and
>  if ok - return the descriptor. This is strange logic for me and I
>  don't want to add any aditional logic here. Also, such solution will
 
>  add additional attempts to open the file.
> 
> So, it look like such minimal changes will work fine and are simple
> to review.
> 
> Co-authored-by: Mergen Imeev<imeevma@gmail.com>
> 
> Fixes #5537
> ---
> src/box/sql/os_unix.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/sql/os_unix.c b/src/box/sql/os_unix.c
> index 557d709..ce415cb 100644
> --- a/src/box/sql/os_unix.c
> +++ b/src/box/sql/os_unix.c
> @@ -1483,8 +1483,8 @@ unixGetTempname(int nBuf, char *zBuf)
> 		assert(nBuf > 2);
> 		zBuf[nBuf - 2] = 0;
> 		sql_snprintf(nBuf, zBuf,
> -				 "%s/" SQL_TEMP_FILE_PREFIX "%llx%c", zDir,
> -				 r, 0);
> +				 "%s/" SQL_TEMP_FILE_PREFIX "%ld_%llx%c", zDir,
> +				 (long)randomnessPid, r, 0);
> 		if (zBuf[nBuf - 2] != 0 || (iLimit++) > 10)
> 			return -1;
> 	} while (access(zBuf, 0) == 0);
> -- 
> 2.7.4
>