From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp37.i.mail.ru (smtp37.i.mail.ru [94.100.177.97]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id A197145C304 for ; Fri, 11 Dec 2020 18:20:50 +0300 (MSK) References: <2554D7DF-A952-4238-8096-C4618FD2C938@tarantool.org> <236e0556-b6a1-0b67-58f9-617dd1fca21b@tarantool.org> <20201211125130.GC12730@tarantool.org> From: Leonid Vasiliev Message-ID: <5513066d-5d5b-2012-29e9-99a220a7b0a7@tarantool.org> Date: Fri, 11 Dec 2020 18:19:51 +0300 MIME-Version: 1.0 In-Reply-To: <20201211125130.GC12730@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH 2/2] sql: update temporary file name format List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik Cc: tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy , m.semkin@corp.mail.ru Hi! On 11.12.2020 15:51, Nikita Pettik wrote: > On 11 Dec 02:08, Leonid Vasiliev wrote: >> Hi! Thank you for review. >> >> On 10.12.2020 19:39, Sergey Ostanevich wrote: >>> 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. >> >> As I wrote in the commit message, I think using `O_TMPFILE` or >> `tmpfile()` is the best solution to work with temporary files. >> But we already have the code in which the filename is passed from one >> function to another, and some logic over all this. So, replacing the use of >> a named file with a real temporary file will lead to refactoring, >> changing some logic and increasing the number of differences with >> SQLite. This can lead to degradation and complication of the review. > > Anyway, there's already huge difference between our codebases, so > don't worry about incompatibility between SQLite and Tarantool. > If you don't want fix this problem now, could you please open issue > and describe problem there (copy-paste alternatives you suggested > in the previous letter)? We have a discussion with Sergos, and I will create several tasks as a result of the discussion (or find existing ones). AFAIK, this patch will completely fix the problem. Going to a true temp files is more about how to do it from the start. If you see potential issues with this solution please share. > >> Instead, I propose a simple one line change. Could you please describe >> the reproduction of the probable problem step by step? >> >>> Sergos >>> >>> >>>> On 8 Dec 2020, at 22:59, Leonid Vasiliev 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 >>>> >>>> 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 >>>> >>>