My team would like me to develop a way to run FDB in a read-only mode, to protect against accidental writes categorically. Our goal is to be able to flip a switch and prevent writes on any specific day, without changing our code to prevent writes in general, since we sometimes still need them.
The simplest way I can think of doing this to satisfy our needs is to replace the FDBTransaction class in the java api (since we only use java) with one that throws an exception any time one of the write-specific methods from the Transaction interface is called, as long as the database has been acquired in a read-only mode. If it’s been acquired in write mode, it executes the methods normally.
But if possible we’d really like to get this upstream in some form or another so that it doesn’t have the potential to break after every release. So if there’s a preferred way to do it in the c++ code (or someone who’s already working on this) I’d be more than happy to take that approach instead. Has there been any discussion about this before?
I was thinking that this could maybe be done by introducing an ALLOW_WRITES TransactionOption, and then use the support from Support setting Transaction options at the Database level to make ALLOW_WRITES=false the default after opening the database, thus making all your transactions created read-only by default.
However, it doesn’t appear that #1323 applied to all transactions options, only some? @alloc, was that intentional? Would extending it for this be fine?
Yeah, #1323 was intentionally applied to only a subset of transaction options. In particular, the only options that I added were the ones I could make a good case for as something that made sense to apply to all transactions (like retry limit). So, for example, we’d probably never want to add the set_next_write_no_conflict_range option globally.
But I think it would be reasonable to add more. The change (at an implementation level) was to add more fields to the DatabaseContext struct and to use those values to initialize the transaction options, so adding more fields could make sense. The two that were the most questionably excluded were set_disable_use_during_commit_protection and set_read_lock_aware. The former in theory might make sense to disable globally and, in fact, is disabled globally already in Java as it’s called by Database::createTransaction. Pushing that down might also make sense, though whether it’s safe kind of depends on the language bindings.
The set_read_lock_aware option, in theory, maybe should be set globally so that if one has a locked cluster, one can set it up so that one can easily read stale data from it without having to set the option manually each time. (Though maybe this is less important in FDB 6 than FDB 5 because of multi-region configurations don’t require the option be set to read from secondary regions.) The problem is that set_read_lock_aware is implemented by (1) setting lock_aware and (2) disabling commits. And one can’t “undisable” commits, so you get a kind of strange database object back.
Which I guess brings me back to the original question. The only method that must actually throw an error to ensure the transaction is read-only is commit—all other write-related operations will just be buffered locally. (Except maybe some keys in the \xff\xff keyspace? Maybe?) And the native client actually already has a way to disable commits (it’s used by set_read_lock_aware) though it appears not to be settable by the user.
Unlike the proposed option, I don’t think it can be undone, so using it as a way of providing a “read-only be default” database might not work. But your client could implement something like “on transaction creation, if I am in a read only context, disable commits”, and the only awkward thing is that doesn’t play super well with how our retry loops work.
Thanks! It sounds like the biggest issue you see is the inability to turn read-only mode off once commits have been disabled. But it should be possible to address that by defaulting the database to a write-enabled mode, and then setting the disable writes option at the database level on a per client basis, correct? I’m out of the office next week, but I’ll take a look at how set_read_lock_aware actually disables commits when I’m back.
Right. To be clear, I was really just commenting on what would happen on the client. (None of this affects the server. AFAIK, there isn’t any way of locking the database in a read only mode at the server level.) In particular, as implemented, as there’s no way for a given transaction to start allowing writes, a client who wishes to have a mix of read-only and read-write transactions from the same process can only do so by defaulting all transactions to read-write and then disabling writes (in a somewhat roundabout way) on transactions they want to be read only. It seemed borderline to me to have an option that would (surprise!) stop the client from making any commits at all, but I guess I can see the benefit of a whole process which could only read from the DB. (I don’t have too many principled objections, but it seemed like the conservative thing to do when introducing a new API would be to leave out the things that were borderline if they could be added later.) I do think that the option should be explicit (e.g., a “set read only” option on the database object) rather than implicit (e.g., turns out, setting “read lock aware” also disables commits).
I’m not sure I’m reading the code correctly, but it seems like the TransactionOptions option readOnly does exactly the right thing when enabled, canceling the commit of any transaction that does more than read. It seems better to me to expose setting that option directly (as now I think it can only be set from calling set_lock_aware or set_read_lock_aware) by adding a new code for it in fdb_c_options.g.h. Then Database::createTransaction could do the same thing it does with set_disable_use_during_commit_protection in the java bindings and enable it for every transaction, but only conditionally on the value of some new readOnly bool stored on the Database object. If I’m understanding everything correctly, that completely sidesteps the disabling/inability to “undisable” issue.
For a little more context of our goals, this ticket is motivated by an issue we had where a CLI tool that was thought to be read-only was run on a database and ended up actually writing things. We want stronger protection against accidental misuse.
Yeah, I think a setReadOnly option added to transaction options would make sense.
I think instead of using the Java-level Database object, you’d want to add a new database option that was like setReadOnly or setTransactionsAreReadOnly. This mirrors the way we handle the setTimeout transaction option and the setTransactionTimeout database option. See PR #1323 for how that was added if you wanted a template. (But also see Issue #1435 for one bug in how that interacts with the multi-version client. That…might not matter in this case. It would somewhat depend on whether the option would need to be reset on retry, I suppose. There were reasons to disable that with the other DB-wide options, but maybe not this one.) That approach would let the other bindings also get this change.
I’m not sure this totally sidesteps the “undisable” thing. It would mean that if the user had a mix of read only and read write transactions, they wouldn’t be able to use the setTransactionsAreReadOnly option on a database; the only supported path for them would be to remember to set the setReadOnly option on their read-only transactions with the default being read-write. You could imagine a different API that was setTransactionsAreDefaultReadOnly (or something) that only sets the flag if the user doesn’t also set a setReadWrite option on a database (thereby allowing them to, by default, disable writes, but still provide a way for them to opt-into it).
That being said, it is probably fine if configuring the database as read only also made it so that the transaction was itself read only. We should probably document that and make that its formal contract if that’s what we go with, but I think it has reasonable enough semantics. If there’s a way forward (that doesn’t necessarily have to be implemented now) to enable a “transactions are read only by default but can be made read-write” feature, I think that would be for the best, but maybe not strictly necessary.
We ended up developing a solution in our code rather than FDB’s because we thought it would be simpler (it probably wasn’t because of a lot of unexpected problems arising, but that’s how hindsight goes). So the feature does not exist in FDB. There is still some discussion here of doing it in FDB proper so that we don’t need to keep updating our solution with every FDB update, but it’s not a high priority for us right now.