Re: Buildbots: require Python-SQLite v3.8.2 [was: FSFS recovery should prune rep-cache even if its use is disabled]
On 27.08.2018 14:00, Daniel Shahaf wrote: > Branko Čibej wrote on Mon, 27 Aug 2018 13:44 +0200: >> On 24.08.2018 13:55, Julian Foad wrote: >>> Brane, this one's yours: >>> >>> https://ci.apache.org/buildslaves/svn-x64-macosx-dgvrs >>> >>> The new tests for #4077 are failing on at least this buildbot-slave. >>> >>> "Can't read rep-cache schema 2 using old Python-SQLite version (3, >>> 7, 13) < (3,8,2)" >>> >>> (danielsh says: the magic number 3.8.2 is the minimum version of SQLite >>> that trunk can be built with.) >>> >>> Can you upgrade it please? >> What should I upgrade? > The 'sqlite3' Python module. Compare: > > svn --version -v | grep SQLite > python -c 'import sqlite3; print(sqlite3.sqlite_version)' Ah, ok, that's separate, of course. Implies upgrading Python itself ... we're using the OS-default version, which is 2.7.5, and a manually compiled Python3. I'll see what I can do. -- Brane
Re: Buildbots: require Python-SQLite v3.8.2 [was: FSFS recovery should prune rep-cache even if its use is disabled]
Branko Čibej wrote on Mon, 27 Aug 2018 13:44 +0200: > On 24.08.2018 13:55, Julian Foad wrote: > > Brane, this one's yours: > > > > https://ci.apache.org/buildslaves/svn-x64-macosx-dgvrs > > > > The new tests for #4077 are failing on at least this buildbot-slave. > > > > "Can't read rep-cache schema 2 using old Python-SQLite version (3, > > 7, 13) < (3,8,2)" > > > > (danielsh says: the magic number 3.8.2 is the minimum version of SQLite > > that trunk can be built with.) > > > > Can you upgrade it please? > > What should I upgrade? The 'sqlite3' Python module. Compare: svn --version -v | grep SQLite python -c 'import sqlite3; print(sqlite3.sqlite_version)' > The build on that buildbot use get-deps.sh to download an amalgamated > version of SQLite. Cheers, Daniel
Re: Buildbots: require Python-SQLite v3.8.2 [was: FSFS recovery should prune rep-cache even if its use is disabled]
On 24.08.2018 13:55, Julian Foad wrote: > Brane, this one's yours: > > https://ci.apache.org/buildslaves/svn-x64-macosx-dgvrs > > The new tests for #4077 are failing on at least this buildbot-slave. > > "Can't read rep-cache schema 2 using old Python-SQLite version (3, > 7, 13) < (3,8,2)" > > (danielsh says: the magic number 3.8.2 is the minimum version of SQLite > that trunk can be built with.) > > Can you upgrade it please? What should I upgrade? The build on that buildbot use get-deps.sh to download an amalgamated version of SQLite. -- Brane
Buildbots: require Python-SQLite v3.8.2 [was: FSFS recovery should prune rep-cache even if its use is disabled]
Brane, this one's yours: https://ci.apache.org/buildslaves/svn-x64-macosx-dgvrs The new tests for #4077 are failing on at least this buildbot-slave. "Can't read rep-cache schema 2 using old Python-SQLite version (3, 7, 13) < (3,8,2)" (danielsh says: the magic number 3.8.2 is the minimum version of SQLite that trunk can be built with.) Can you upgrade it please? -- - Julian
Re: FSFS recovery should prune rep-cache even if its use is disabled
Julian Foad wrote on Fri, 24 Aug 2018 12:17 +0100: > Julian Foad: > > Committed in http://svn.apache.org/r1838813 > > And nominated for backport to 1.10.x and 1.9.x. > > The tests are failing on some buildbots: > > "Can't read rep-cache schema 2 using old Python-SQLite version (3, 7, > 13) < (3,8,2)" > Clarification: the magic number 3.8.2 is the minimum version of SQLite that trunk can be built with. > If we're using SQLite and want to have tests written in Python, would it > not be reasonable to require a version of Python-SQLite that can read > it? > > Can someone upgrade the buildbots? Brane, this one's yours: > https://ci.apache.org/buildslaves/svn-x64-macosx-dgvrs
Re: FSFS recovery should prune rep-cache even if its use is disabled
Julian Foad: > Committed in http://svn.apache.org/r1838813 And nominated for backport to 1.10.x and 1.9.x. The tests are failing on some buildbots: "Can't read rep-cache schema 2 using old Python-SQLite version (3, 7, 13) < (3,8,2)" If we're using SQLite and want to have tests written in Python, would it not be reasonable to require a version of Python-SQLite that can read it? Can someone upgrade the buildbots? Brane, this one's yours: https://ci.apache.org/buildslaves/svn-x64-macosx-dgvrs -- - Julian
Re: FSFS recovery should prune rep-cache even if its use is disabled
Daniel Shahaf wrote: > Julian Foad wrote on Thu, Aug 23, 2018 at 10:21:17 +0100: > > +++ subversion/libsvn_fs_fs/recovery.c (working copy) > > @@ -468,15 +468,15 @@ recover_body(void *baton, apr_pool_t *po > >/* Prune younger-than-(newfound-youngest) revisions from the rep > > - cache if sharing is enabled taking care not to create the cache > > - if it does not exist. */ > > - if (ffd->rep_sharing_allowed) > > + cache, no matter whether sharing is currently enabled, taking care > > + not to create the cache if it does not exist. */ > > + if (ffd->format >= SVN_FS_FS__MIN_REP_SHARING_FORMAT) > > Looks good to me: that should fix both #4077 and #4214. Committed in http://svn.apache.org/r1838813 I added tests there for both cases (enabled, disabled). > I would only suggest expanding the comment [...] Looks good to me, so I included your text. Thanks. - Julian
Re: FSFS recovery should prune rep-cache even if its use is disabled
Julian Foad wrote on Thu, Aug 23, 2018 at 10:21:17 +0100:
> +++ subversion/libsvn_fs_fs/recovery.c(working copy)
> @@ -468,15 +468,15 @@ recover_body(void *baton, apr_pool_t *po
>/* Prune younger-than-(newfound-youngest) revisions from the rep
> - cache if sharing is enabled taking care not to create the cache
> - if it does not exist. */
> - if (ffd->rep_sharing_allowed)
> + cache, no matter whether sharing is currently enabled, taking care
> + not to create the cache if it does not exist. */
> + if (ffd->format >= SVN_FS_FS__MIN_REP_SHARING_FORMAT)
Looks good to me: that should fix both #4077 and #4214.
I would only suggest expanding the comment to explain why it's important that
the condition is what it is; for example:
/* Prune younger-than-(newfound-youngest) revisions from the rep
cache, taking care not to create the cache if it does not exist.
We do this whenever rep-cache.db exists, whether it's currently enabled
or not, to prevent a data loss that could result from having revisions
created after this 'recover' operation referring to rep-cache.db rows
that were created before the recover and that point to revisions younger-
than-(newfound-youngest).
*/
(Possibly with some rewording to make it easier to read in three years when
we've all forgotten the context.)
Cheers,
Daniel
> {
>svn_boolean_t rep_cache_exists;
>
>SVN_ERR(svn_fs_fs__exists_rep_cache(_cache_exists, fs, pool));
>if (rep_cache_exists)
> SVN_ERR(svn_fs_fs__del_rep_reference(fs, max_rev, pool));
Re: FSFS recovery should prune rep-cache even if its use is disabled
Proposed patch attached: prune-rep-cache-1.patch
--
- Julian
Let "svnadmin recover" prune the FSFS rep-cache even if it's not being used.
Part of issue #4077 "FSFS recover should prune unborn revisions from
rep-cache.db".
This was included as r1213716 in the original fix for issue #4077, first
released in Subversion 1.7.3, but was reverted in r1367674 (issue #4214)
and so omitted from Subversion 1.8 and later series of releases.
* subversion/libsvn_fs_fs/recovery.c
(recover_body): Prune the rep-cache no matter whether it's in use.
--This line, and those below, will be ignored--
Index: subversion/libsvn_fs_fs/recovery.c
===
--- subversion/libsvn_fs_fs/recovery.c (revision 1837286)
+++ subversion/libsvn_fs_fs/recovery.c (working copy)
@@ -468,15 +468,15 @@ recover_body(void *baton, apr_pool_t *po
_("Revision %ld has a non-file where its "
"revprops file should be"),
max_rev);
}
/* Prune younger-than-(newfound-youngest) revisions from the rep
- cache if sharing is enabled taking care not to create the cache
- if it does not exist. */
- if (ffd->rep_sharing_allowed)
+ cache, no matter whether sharing is currently enabled, taking care
+ not to create the cache if it does not exist. */
+ if (ffd->format >= SVN_FS_FS__MIN_REP_SHARING_FORMAT)
{
svn_boolean_t rep_cache_exists;
SVN_ERR(svn_fs_fs__exists_rep_cache(_cache_exists, fs, pool));
if (rep_cache_exists)
SVN_ERR(svn_fs_fs__del_rep_reference(fs, max_rev, pool));
Re: FSFS recovery should prune rep-cache even if its use is disabled
CC'ing Philip...
Daniel Shahaf wrote:
> Julian Foad wrote on Wed, 22 Aug 2018 22:20 +0100:
> > Julian Foad wrote:
> > > It looks like r1213716 ("also prune the rep-cache when it's present but
> > > reportedly not being used") was reverted by r1367674, apparently
> > > unintentionally.
> >
> > Well, with some degree of intention, [... but] no mention in
> > https://issues.apache.org/jira/browse/SVN-4214 .
I just noticed that https://issues.apache.org/jira/browse/SVN-4214 says
"Recover should not operate on rep-cache.db if rep-caching is disabled" in its
description. At first I misread that line as "Recover should not create
rep-cache.db ..." like the issue's summary.
Philip, can you recall anything that might help re-evaluate this now?
> I assume, going by that comment, that Philip's reason for changing the
> condition was that svn_fs_fs__del_rep_reference() would create an empty
> rep-cache.db if it was called when (ffd->format >=
> SVN_FS_FS__MIN_REP_SHARING_FORMAT
> && ffd->rep_sharing_allowed == FALSE).
But in the new inner block, __del_rep_reference() isn't called if rep-cache.db
doesn't exist, so it can't create it.
> > > If "recovery" while re-sharing is disabled (by the fsfs.conf setting)
> > > leaves future revision entries in the rep-cache, then later re-enabling
> > > the rep-cache could cause serious corruption if those entries are then
> > > used.
> > >
> > > Therefore I think we should repeat r1213716 as a bug fix.
> > >
> > > WDYT?
>
> +1, no question about it. Or rather, I think the question is whether to
> backport it only to 1.10 or also to 1.9...
It can potentially lead to data loss, so we should backport to 1.9 as well.
--
- Julian
Re: FSFS recovery should prune rep-cache even if its use is disabled
Julian Foad wrote on Wed, 22 Aug 2018 22:20 +0100:
> Julian Foad wrote:
> > It looks like r1213716 ("also prune the rep-cache when it's present but
> > reportedly not being used") was reverted by r1367674, apparently
> > unintentionally.
>
> Well, with some degree of intention, judging by the code comment having
> been adjusted accordingly, but with no reason given and no mention in
> https://issues.apache.org/jira/browse/SVN-4214 .
>
I assume, going by that comment, that Philip's reason for changing the
condition was that svn_fs_fs__del_rep_reference() would create an empty
rep-cache.db if it was called when (ffd->format >=
SVN_FS_FS__MIN_REP_SHARING_FORMAT
&& ffd->rep_sharing_allowed == FALSE).
> > If "recovery" while re-sharing is disabled (by the fsfs.conf setting)
> > leaves future revision entries in the rep-cache, then later re-enabling
> > the rep-cache could cause serious corruption if those entries are then
> > used.
> >
> > Therefore I think we should repeat r1213716 as a bug fix.
> >
> > WDYT?
+1, no question about it. Or rather, I think the question is whether to
backport it only to 1.10 or also to 1.9...
Cheers,
Daniel
Re: FSFS recovery should prune rep-cache even if its use is disabled
Julian Foad wrote:
> It looks like r1213716 ("also prune the rep-cache when it's present but
> reportedly not being used") was reverted by r1367674, apparently
> unintentionally.
Well, with some degree of intention, judging by the code comment having been
adjusted accordingly, but with no reason given and no mention in
https://issues.apache.org/jira/browse/SVN-4214 .
- Julian
> https://svn.apache.org/r1213716 (on 2011-12-13)
> > Follow-up to r1213681: also prune the rep-cache when it's present
> > but reportedly not being used.
> [...]
>/* Prune younger-than-(newfound-youngest) revisions from the rep cache. */
> - if (ffd->rep_sharing_allowed)
> + if (ffd->format >= SVN_FS_FS__MIN_REP_SHARING_FORMAT)
> SVN_ERR(svn_fs_fs__del_rep_reference(fs, max_rev, pool));
>
> https://svn.apache.org/r1367674 (on 2012-07-31)
> > Fix issue 4214, "svnadmin recover" should not create rep-cache.db.
> > * subversion/libsvn_fs_fs/fs_fs.c (recover_body): Don't create rep-cache.
> [...]
> - /* Prune younger-than-(newfound-youngest) revisions from the rep cache. */
> - if (ffd->format >= SVN_FS_FS__MIN_REP_SHARING_FORMAT)
> -SVN_ERR(svn_fs_fs__del_rep_reference(fs, max_rev, pool));
> + /* Prune younger-than-(newfound-youngest) revisions from the rep
> + cache if sharing is enabled taking care not to create the cache
> + if it does not exist. */
> + if (ffd->rep_sharing_allowed)
> +{
> + svn_boolean_t rep_cache_exists;
> +
> + SVN_ERR(svn_fs_fs__exists_rep_cache(_cache_exists, fs, pool));
> + if (rep_cache_exists)
> +SVN_ERR(svn_fs_fs__del_rep_reference(fs, max_rev, pool));
> +}
>
> The function concerned, recover_body(), has since been moved to
> subversion/libsvn_fs_fs/recovery.c.
>
> If "recovery" while re-sharing is disabled (by the fsfs.conf setting)
> leaves future revision entries in the rep-cache, then later re-enabling
> the rep-cache could cause serious corruption if those entries are then
> used.
>
> Therefore I think we should repeat r1213716 as a bug fix.
>
> WDYT?
FSFS recovery should prune rep-cache even if its use is disabled
It looks like r1213716 ("also prune the rep-cache when it's present but
reportedly not being used") was reverted by r1367674, apparently
unintentionally.
https://svn.apache.org/r1213716 (on 2011-12-13)
> Follow-up to r1213681: also prune the rep-cache when it's present
> but reportedly not being used.
[...]
/* Prune younger-than-(newfound-youngest) revisions from the rep cache. */
- if (ffd->rep_sharing_allowed)
+ if (ffd->format >= SVN_FS_FS__MIN_REP_SHARING_FORMAT)
SVN_ERR(svn_fs_fs__del_rep_reference(fs, max_rev, pool));
https://svn.apache.org/r1367674 (on 2012-07-31)
> Fix issue 4214, "svnadmin recover" should not create rep-cache.db.
> * subversion/libsvn_fs_fs/fs_fs.c (recover_body): Don't create rep-cache.
[...]
- /* Prune younger-than-(newfound-youngest) revisions from the rep cache. */
- if (ffd->format >= SVN_FS_FS__MIN_REP_SHARING_FORMAT)
-SVN_ERR(svn_fs_fs__del_rep_reference(fs, max_rev, pool));
+ /* Prune younger-than-(newfound-youngest) revisions from the rep
+ cache if sharing is enabled taking care not to create the cache
+ if it does not exist. */
+ if (ffd->rep_sharing_allowed)
+{
+ svn_boolean_t rep_cache_exists;
+
+ SVN_ERR(svn_fs_fs__exists_rep_cache(_cache_exists, fs, pool));
+ if (rep_cache_exists)
+SVN_ERR(svn_fs_fs__del_rep_reference(fs, max_rev, pool));
+}
The function concerned, recover_body(), has since been moved to
subversion/libsvn_fs_fs/recovery.c.
If "recovery" while re-sharing is disabled (by the fsfs.conf setting) leaves
future revision entries in the rep-cache, then later re-enabling the rep-cache
could cause serious corruption if those entries are then used.
Therefore I think we should repeat r1213716 as a bug fix.
WDYT?
--
- Julian
