Re: svn commit: r1865522 - /subversion/trunk/subversion/libsvn_subr/sqlite3wrapper.c
kot...@apache.org wrote on Tue, 20 Aug 2019 09:18 +00:00: > When compiling SQLite, set the SQLITE_DEFAULT_MEMSTATUS=0 compile-time option. > > This is the recommended option that is not enabled by default. Setting it to > zero avoids using a mutex (and thus suffering a performance penalty) during > every sqlite3_malloc() call, where this mutex is used to properly update the > allocations stats. We don't use sqlite3_status(), so set this option to zero. > > See https://sqlite.org/compile.html#recommended_compile_time_options Would it break anything if some application that uses libsvn had its own copy of SQLite that was compiled with another SQLITE_DEFAULT_MEMSTATUS value? I don't see anything about that in the linked page. Cheers, Daniel
Re: svn commit: r1865523 - /subversion/trunk/subversion/libsvn_subr/sqlite3wrapper.c
kot...@apache.org wrote on Tue, 20 Aug 2019 09:23 +00:00: > +++ subversion/trunk/subversion/libsvn_subr/sqlite3wrapper.c Tue Aug 20 > 09:23:55 2019 > @@ -26,6 +26,7 @@ > #ifdef SVN_SQLITE_INLINE > # define SQLITE_OMIT_DEPRECATED 1 > # define SQLITE_DEFAULT_MEMSTATUS 0 > +# define SQLITE_OMIT_WAL 1 Quoting https://sqlite.org/compile.html#_options_to_omit_features: Important Note: The SQLITE_OMIT_* options may not work with the amalgamation. SQLITE_OMIT_* compile-time options usually work correctly only when SQLite is built from canonical source files. and a bit later: Also, not all SQLITE_OMIT_* options are tested. Some SQLITE_OMIT_* options might cause SQLite to malfunction and/or provide incorrect answers. I think we should: - Revert r1865523 (feel free to add it to INSTALL, notes/knobs, etc; but off by default, please) - Stop defining SQLITE_OMIT_DEPRECATED - Backport the latter change to stable branches. To the latter two bullet points: I'm aware that https://sqlite.org/compile.html#recommended_compile_time_options recommends SQLITE_OMIT_DEPRECATED, but https://sqlite.org/compile.html#_options_to_omit_features says using that macro may break correctness (!), and I'm voting to err on the side of caution. Thanks for linking the upstream documentation in the log message! Cheers, Daniel > # define SQLITE_API static > # if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 2) > #pragma GCC diagnostic ignored "-Wunreachable-code" > > >
svn commit: r1865523 - /subversion/trunk/subversion/libsvn_subr/sqlite3wrapper.c
Author: kotkov Date: Tue Aug 20 09:23:55 2019 New Revision: 1865523 URL: http://svn.apache.org/viewvc?rev=1865523=rev Log: When compiling SQLite, enable the SQLITE_OMIT_WAL compile-time option. We don't use WAL (write-ahead logging) feature of SQLite, but just keeping it enabled has a visible I/O performance penalty, because SQLite has to check if the write-ahead log file is present on disk. In a couple of my experiments, disabling this feature resulted in a ~10% faster `svn st` on a large working copy. * subversion/libsvn_subr/sqlite3wrapper.c (): Define SQLITE_OMIT_WAL. Modified: subversion/trunk/subversion/libsvn_subr/sqlite3wrapper.c Modified: subversion/trunk/subversion/libsvn_subr/sqlite3wrapper.c URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/sqlite3wrapper.c?rev=1865523=1865522=1865523=diff == --- subversion/trunk/subversion/libsvn_subr/sqlite3wrapper.c (original) +++ subversion/trunk/subversion/libsvn_subr/sqlite3wrapper.c Tue Aug 20 09:23:55 2019 @@ -26,6 +26,7 @@ #ifdef SVN_SQLITE_INLINE # define SQLITE_OMIT_DEPRECATED 1 # define SQLITE_DEFAULT_MEMSTATUS 0 +# define SQLITE_OMIT_WAL 1 # define SQLITE_API static # if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 2) #pragma GCC diagnostic ignored "-Wunreachable-code"
svn commit: r1865522 - /subversion/trunk/subversion/libsvn_subr/sqlite3wrapper.c
Author: kotkov Date: Tue Aug 20 09:18:21 2019 New Revision: 1865522 URL: http://svn.apache.org/viewvc?rev=1865522=rev Log: When compiling SQLite, set the SQLITE_DEFAULT_MEMSTATUS=0 compile-time option. This is the recommended option that is not enabled by default. Setting it to zero avoids using a mutex (and thus suffering a performance penalty) during every sqlite3_malloc() call, where this mutex is used to properly update the allocations stats. We don't use sqlite3_status(), so set this option to zero. See https://sqlite.org/compile.html#recommended_compile_time_options * subversion/libsvn_subr/sqlite3wrapper.c (): Define SQLITE_DEFAULT_MEMSTATUS=0. Modified: subversion/trunk/subversion/libsvn_subr/sqlite3wrapper.c Modified: subversion/trunk/subversion/libsvn_subr/sqlite3wrapper.c URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/sqlite3wrapper.c?rev=1865522=1865521=1865522=diff == --- subversion/trunk/subversion/libsvn_subr/sqlite3wrapper.c (original) +++ subversion/trunk/subversion/libsvn_subr/sqlite3wrapper.c Tue Aug 20 09:18:21 2019 @@ -25,6 +25,7 @@ /* Include sqlite3 inline, making all symbols private. */ #ifdef SVN_SQLITE_INLINE # define SQLITE_OMIT_DEPRECATED 1 +# define SQLITE_DEFAULT_MEMSTATUS 0 # define SQLITE_API static # if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 2) #pragma GCC diagnostic ignored "-Wunreachable-code"
svn commit: r1865520 - /subversion/trunk/subversion/libsvn_subr/io.c
Author: kotkov
Date: Tue Aug 20 09:09:54 2019
New Revision: 1865520
URL: http://svn.apache.org/viewvc?rev=1865520=rev
Log:
Win32: fix an incorrect error status being propagated to the caller in case
where we fail to stat the destination path while being in the middle of a
failed rename.
So, if we fail to stat the destination in the middle of such rename, propagate
the *original* error. Because overriding the original error with the new one
loses information about the actual cause of failure and may even confuse some
of the callers, who for instance attempt to recover in case of an EACCES,
since they may not be receiving that error at all.
(This is the behavior we had for a long time, even before r1865518, but now
seems to be an appropriate moment to fix that)
* subversion/libsvn_subr/io.c
(win32_file_rename): Use a separate `stat_err` for the case of the failed
GetFileAttributes() call. If we failed to stat the file, return the
original `err` status to the caller.
Modified:
subversion/trunk/subversion/libsvn_subr/io.c
Modified: subversion/trunk/subversion/libsvn_subr/io.c
URL:
http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/io.c?rev=1865520=1865519=1865520=diff
==
--- subversion/trunk/subversion/libsvn_subr/io.c (original)
+++ subversion/trunk/subversion/libsvn_subr/io.c Tue Aug 20 09:09:54 2019
@@ -4500,8 +4500,9 @@ win32_file_rename(const WCHAR *from_path
DWORD attrs = GetFileAttributesW(to_path_w);
if (attrs == INVALID_FILE_ATTRIBUTES)
{
- err = apr_get_os_error();
- if (!(APR_STATUS_IS_ENOENT(err) ||
SVN__APR_STATUS_IS_ENOTDIR(err)))
+ apr_status_t stat_err = apr_get_os_error();
+ if (!(APR_STATUS_IS_ENOENT(stat_err) ||
SVN__APR_STATUS_IS_ENOTDIR(stat_err)))
+/* We failed to stat the file, propagate the original error */
return err;
}
else if (attrs & FILE_ATTRIBUTE_READONLY)
@@ -4514,6 +4515,7 @@ win32_file_rename(const WCHAR *from_path
{
err = apr_get_os_error();
if (!(APR_STATUS_IS_ENOENT(err) ||
SVN__APR_STATUS_IS_ENOTDIR(err)))
+/* We failed to set file attributes, propagate this new
error */
return err;
}
}
svn commit: r1865519 - /subversion/trunk/subversion/libsvn_subr/io.c
Author: kotkov
Date: Tue Aug 20 09:00:03 2019
New Revision: 1865519
URL: http://svn.apache.org/viewvc?rev=1865519=rev
Log:
* subversion/libsvn_subr/io.c
(win32_file_rename): Rename `status` to `err`. This lays the groundwork for
fixing the incorrect error status being propagated to the caller in case
where we fail to stat the destination path while being in the middle of
a failed rename.
Modified:
subversion/trunk/subversion/libsvn_subr/io.c
Modified: subversion/trunk/subversion/libsvn_subr/io.c
URL:
http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/io.c?rev=1865519=1865518=1865519=diff
==
--- subversion/trunk/subversion/libsvn_subr/io.c (original)
+++ subversion/trunk/subversion/libsvn_subr/io.c Tue Aug 20 09:00:03 2019
@@ -4492,17 +4492,17 @@ win32_file_rename(const WCHAR *from_path
if (!MoveFileExW(from_path_w, to_path_w, flags))
{
- apr_status_t status = apr_get_os_error();
+ apr_status_t err = apr_get_os_error();
/* If the target file is read only NTFS reports EACCESS and
FAT/FAT32 reports EEXIST */
- if (APR_STATUS_IS_EACCES(status) || APR_STATUS_IS_EEXIST(status))
+ if (APR_STATUS_IS_EACCES(err) || APR_STATUS_IS_EEXIST(err))
{
DWORD attrs = GetFileAttributesW(to_path_w);
if (attrs == INVALID_FILE_ATTRIBUTES)
{
- status = apr_get_os_error();
- if (!(APR_STATUS_IS_ENOENT(status) ||
SVN__APR_STATUS_IS_ENOTDIR(status)))
-return status;
+ err = apr_get_os_error();
+ if (!(APR_STATUS_IS_ENOENT(err) ||
SVN__APR_STATUS_IS_ENOTDIR(err)))
+return err;
}
else if (attrs & FILE_ATTRIBUTE_READONLY)
{
@@ -4512,9 +4512,9 @@ win32_file_rename(const WCHAR *from_path
attrs &= ~FILE_ATTRIBUTE_READONLY;
if (!SetFileAttributesW(to_path_w, attrs))
{
- status = apr_get_os_error();
- if (!(APR_STATUS_IS_ENOENT(status) ||
SVN__APR_STATUS_IS_ENOTDIR(status)))
-return status;
+ err = apr_get_os_error();
+ if (!(APR_STATUS_IS_ENOENT(err) ||
SVN__APR_STATUS_IS_ENOTDIR(err)))
+return err;
}
}
@@ -4526,7 +4526,7 @@ win32_file_rename(const WCHAR *from_path
return apr_get_os_error();
}
else
-return status;
+return err;
}
return APR_SUCCESS;
svn commit: r1865518 - /subversion/trunk/subversion/libsvn_subr/io.c
Author: kotkov
Date: Tue Aug 20 08:48:32 2019
New Revision: 1865518
URL: http://svn.apache.org/viewvc?rev=1865518=rev
Log:
Win32: prevent svn_io_file_rename2() from spinning in the retry loop in a
racy case where the rename function clears the read-only attribute from the
destination file, but another thread or process is quick enough to set it
back before the original rename function has a chance to proceed.
Despite sounding like an uncommon case, this may happen when the API is
being used to atomically update the file contents while also altering its
read-only attribute (and where such updates can occur in parallel).
Fix this by including the call that clears the read-only attribute on the
destination into the retry loop instead of calling it only once before
looping.
* subversion/libsvn_subr/io.c
(win32_file_rename): Handle EACCES/EEXIST and attempt to clear the
destination read-only attribute if it's there.
(svn_io_file_rename2): Don't call svn_io_set_file_read_write() in the
Win32-specific portion of this function, since clearing the read-only
attribute is now a part of the win32_file_rename() function.
Modified:
subversion/trunk/subversion/libsvn_subr/io.c
Modified: subversion/trunk/subversion/libsvn_subr/io.c
URL:
http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/io.c?rev=1865518=1865517=1865518=diff
==
--- subversion/trunk/subversion/libsvn_subr/io.c (original)
+++ subversion/trunk/subversion/libsvn_subr/io.c Tue Aug 20 08:48:32 2019
@@ -4491,7 +4491,43 @@ win32_file_rename(const WCHAR *from_path
}
if (!MoveFileExW(from_path_w, to_path_w, flags))
- return apr_get_os_error();
+{
+ apr_status_t status = apr_get_os_error();
+ /* If the target file is read only NTFS reports EACCESS and
+ FAT/FAT32 reports EEXIST */
+ if (APR_STATUS_IS_EACCES(status) || APR_STATUS_IS_EEXIST(status))
+{
+ DWORD attrs = GetFileAttributesW(to_path_w);
+ if (attrs == INVALID_FILE_ATTRIBUTES)
+{
+ status = apr_get_os_error();
+ if (!(APR_STATUS_IS_ENOENT(status) ||
SVN__APR_STATUS_IS_ENOTDIR(status)))
+return status;
+}
+ else if (attrs & FILE_ATTRIBUTE_READONLY)
+{
+ /* Try to set the destination file writable because Windows will
+ not allow us to rename when to_path is read-only, but will
+ allow renaming when from_path is read only. */
+ attrs &= ~FILE_ATTRIBUTE_READONLY;
+ if (!SetFileAttributesW(to_path_w, attrs))
+{
+ status = apr_get_os_error();
+ if (!(APR_STATUS_IS_ENOENT(status) ||
SVN__APR_STATUS_IS_ENOTDIR(status)))
+return status;
+}
+}
+
+ /* NOTE: If the file is not read-only, we don't know if the file did
+ not have the read-only attribute in the first place or if this
+ attribute disappeared due to a race, so try to rename it anyway.
+ */
+ if (!MoveFileExW(from_path_w, to_path_w, flags))
+return apr_get_os_error();
+}
+ else
+return status;
+}
return APR_SUCCESS;
}
@@ -4515,18 +4551,6 @@ svn_io_file_rename2(const char *from_pat
SVN_ERR(svn_io__utf8_to_unicode_longpath(_path_w, from_path_apr, pool));
SVN_ERR(svn_io__utf8_to_unicode_longpath(_path_w, to_path_apr, pool));
status = win32_file_rename(from_path_w, to_path_w, flush_to_disk);
-
- /* If the target file is read only NTFS reports EACCESS and
- FAT/FAT32 reports EEXIST */
- if (APR_STATUS_IS_EACCES(status) || APR_STATUS_IS_EEXIST(status))
-{
- /* Set the destination file writable because Windows will not
- allow us to rename when to_path is read-only, but will
- allow renaming when from_path is read only. */
- SVN_ERR(svn_io_set_file_read_write(to_path, TRUE, pool));
-
- status = win32_file_rename(from_path_w, to_path_w, flush_to_disk);
-}
WIN32_RETRY_LOOP(status, win32_file_rename(from_path_w, to_path_w,
flush_to_disk));
#elif defined(__OS2__)
