Tarantool development patches archive
 help / color / mirror / Atom feed
From: Leonid Vasiliev <lvasiliev@tarantool.org>
To: v.shpilevoy@tarantool.org, imeevma@tarantool.org,
	korablev@tarantool.org, sergos@tarantool.org,
	m.semkin@corp.mail.ru
Cc: tarantool-patches@dev.tarantool.org
Subject: [Tarantool-patches] [PATCH v2 3/3] sql: update temporary file name format
Date: Fri, 11 Dec 2020 17:49:12 +0300	[thread overview]
Message-ID: <59ffd9dd6c9e895b85f89bfb23d6f8fcbf2c556f.1607696813.git.lvasiliev@tarantool.org> (raw)
In-Reply-To: <cover.1607696813.git.lvasiliev@tarantool.org>
In-Reply-To: <cover.1607696813.git.lvasiliev@tarantool.org>

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

  parent reply	other threads:[~2020-12-11 14:50 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-11 14:49 [Tarantool-patches] [PATCH v2 0/3] Fix working with temporary files in VDBE Leonid Vasiliev
2020-12-11 14:49 ` [Tarantool-patches] [PATCH v2 1/3] sql: add missing diag_set on failure when working with files inside SQL module Leonid Vasiliev
2020-12-13 18:30   ` Vladislav Shpilevoy
2020-12-14 15:49     ` Leonid Vasiliev
2020-12-15 20:29       ` Sergey Ostanevich
2020-12-15 22:12   ` Vladislav Shpilevoy
2020-12-16 23:17     ` Leonid Vasiliev
2020-12-11 14:49 ` [Tarantool-patches] [PATCH v2 2/3] sql: set an error to diag in sql_execute() on failure Leonid Vasiliev
2020-12-11 15:03   ` Nikita Pettik
2020-12-11 15:40     ` Leonid Vasiliev
2020-12-13 18:30   ` Vladislav Shpilevoy
2020-12-14 15:52     ` Leonid Vasiliev
2020-12-15 21:05       ` Sergey Ostanevich
2020-12-11 14:49 ` Leonid Vasiliev [this message]
2020-12-13 18:30   ` [Tarantool-patches] [PATCH v2 3/3] sql: update temporary file name format Vladislav Shpilevoy
2020-12-14 15:54     ` Leonid Vasiliev
2020-12-15 21:07       ` Sergey Ostanevich
2020-12-16 14:47       ` Nikita Pettik

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=59ffd9dd6c9e895b85f89bfb23d6f8fcbf2c556f.1607696813.git.lvasiliev@tarantool.org \
    --to=lvasiliev@tarantool.org \
    --cc=imeevma@tarantool.org \
    --cc=korablev@tarantool.org \
    --cc=m.semkin@corp.mail.ru \
    --cc=sergos@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v2 3/3] sql: update temporary file name format' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox