Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Received-SPF: pass (athena.apache.org: domain of philip.martin@wandisco.com
 designates 74.125.82.172 as permitted sender)
From: Philip Martin <philip.martin@wandisco.com>
To: Evgeny Kotkov <evgeny.kotkov@visualsvn.com>
Cc: Subversion Development <dev@subversion.apache.org>
Subject: Re: [RFC/PATCH] Modifying internal FS transaction properties
References: 
 <CAP_GPNi_A=1dFzZpAzdT+MYDPOqHeCqneX2S93tnz2q_c1XqGg@mail.gmail.com>
Date: Tue, 03 Mar 2015 11:58:48 +0000
In-Reply-To: 
 <CAP_GPNi_A=1dFzZpAzdT+MYDPOqHeCqneX2S93tnz2q_c1XqGg@mail.gmail.com>
	(Evgeny Kotkov's message of "Mon, 2 Mar 2015 23:23:32 +0300")
Message-ID: <8761aijnrb.fsf@wandisco.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.4 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain

Evgeny Kotkov <evgeny.kotkov@visualsvn.com> writes:

>    I propose that we solve this part of the problem with the attached patch.
>    Please note that while preparing this patch, I also unveiled that the
>    SVN_FS_TXN_CLIENT_DATE flag might not work as expected with BDB.
>    I think we should fix this with a separate patch; see the comment in the
>    new test_internal_txn_props() test.
>
>    Log message:
> [[[
> Error out on attempts to change internal transaction properties like
> 'svn:check-locks' through the public FS API.

Looks OK.  Both BDB and FSFS set the date on the txn when the txn is
created.  This was introduced so that admins could look at the date and
get the txn creation date which is useful when deciding whether or not
to delete old txns.  The BDB bug is that this setting should not be
classed as an explicit setting by the client.

> 2) These internal properties currently are a part of what svn_fs_txn_proplist()
>    and svn_fs_txn_prop() return.  From my point of view, they are nothing more
>    than an implementation detail and I don't think that something like that
>    should ever reach the caller of a public API.  However, this is how things
>    have been working for a long time, and I am not sure about changing it now,
>    although it does make sense to me.

Using svn:date to store txn creation date doesnt't work so well now that
we allow clients to set svn:date.  It might have been better if the txn
creation date was stored in a separate txn property that was deleted on
commit.  If we were to do that then we would need to expose the internal
txn value, which would mean svn_fs_txn_prop/proplist could not hide all
internal txn props.

> +  if (strcmp(opts->fs_type, SVN_FS_TYPE_BDB) == 0)
> +    {
> +      /* ### Skip the svn:client-date check with BDB.  I think that we have a
> +             bug in the BDB backend, because when SVN_FS_TXN_CLIENT_DATE is
> +             set, a transaction created with svn_fs_begin_txn2() has the
> +             svn:client-date property value = "1", even though no explicit
> +             svn:date changes happened (it's "0" in FSFS).
> +
> +             This happens because svn_fs_base__begin_txn() sets svn:date in a
> +             separate svn_fs_base__change_txn_prop() call when the initial
> +             internal properties, including SVN_FS__PROP_TXN_CLIENT_DATE, are
> +             already set.  As a consequence, we should have different bevavior
> +             for transactions created with SVN_FS_TXN_CLIENT_DATE, and with no
> +             explicit svn:date changes happening depending on the FS backend.
> +             When such transaction is committed, FSFS is going to use the time
> +             of the commit for the 'svn:date' value.  BDB, however, is going
> +             to erroneously use the timestamp of the svn_fs_base__begin_txn()
> +             call.
> +      */
> +    }
> +  else
> +    SVN_TEST_STRING_ASSERT(val->data, "0");

I think this test should FAIL, or XFAIL, on BDB rather than be hacked to PASS.

-- 
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*