sql: transaction_read_only can be reset in AOST txns

[knz]

The following works and should not:

BEGIN AS OF SYSTEM TIME 'sometime in the past';
SET transaction_read_only = false;
CREATE TABLE t(x INT);
INSERT INTO t(x) VALUES (1);
SELECT * FROM t;

This succeeds and lets the transaction do stuff with the table !!?!?!

It also leaves over junk data in ranges. I haven't checked but I suspect it also leaves junk data at historical timestamps which will confuse the GC (and possibly let node crashes when table readers get their table data removed from "under them" asynchronously by a confused GC.

I am expecting an error on the SET: "cannot set a txn read/write when operating at a past timestamp"

Found while investigating #44188 (comment)

Jira issue: CRDB-5245

[knz]

@andreimatei can you have a look at the above. This is a serious issue.

[knz]

I think the durable fix is to push the readOnly field down into client.Txn and simply reject any KV request that writes if the client.Txn is read-only.

[knz]

cc @cockroachdb/sql-execution

[andreimatei]

I've looked superficially and it seems to me that at least the INSERT should be rejected by this code:

cockroach/pkg/sql/opt/exec/execbuilder/relational.go

Line 160 in 86b82f3

if opt.IsMutationOp(e) && b.evalCtx.TxnReadOnly {

We seem to be plumbing evalCtx.TxnReadOnly fine as far as I can see. Except obviously something is broken.

I'm reticent to add a read-only mode to client.Txn too quickly. I'm not sure that it'd exactly what SQL needs and I think it might cause us problems later on. For example, should a SQL read-only txn be allowed to increment sequences or acquire table leases? The answers to these questions should be independent on which transaction ends up being used to perform those operations. Conversely, should a read-only client.Txn be allowed to assist with garbage collection of old values that it scans over, as we've talked about doing?
If this read-only thing can remain exclusively a SQL concern, I'd much rather leave it there. An example of a time we've added something similar to KV only to quickly regret it was the transaction's "request count".

I'll pass this over to @jordanlewis to look into.

[knz]

We seem to be plumbing evalCtx.TxnReadOnly fine as far as I can see. Except obviously something is broken.

What is broken is that the SET transaction_read_only is happy to set the value from true to false when it's non-sensical to do so.

For reference

• PostgreSQL requires the session var to be togglable back and forth. So at least the SET code is morally doing what it's supposed to do
• the real problem is that AOST transactions are read-only in a way that's non-negotiable. We definitely can't increment SQL sequences inside of them, nor acquire table leases under them (but note that lease acquisition is typically done under a separate txn exactly for this reason already)

[knz]

My recommendation above to have a "read only" flag inside client.Txn was limited to AOST queries (i.e. not to SQL clients asking transaction_read_only=false after it started read/write). That is, after a call is made to (client.Txn).SetFixedTimestamp, any KV mutation operation should be forbidden. That seems like it falls squarely on KV's shoulders.

@tbg mind chiming in for advice?

[andreimatei]

That is, after a call is made to (client.Txn).SetFixedTimestamp, any KV mutation operation should be forbidden.

Why? What's wrong with writing after calling SetFixedTimestamp? Schema change txns used to do exactly that (I think more recently they stopped using SetFixedTimestamp()).

[knz]

Why? What's wrong with writing after calling SetFixedTimestamp?

It's totally wrong enabling KV writes by SQL transactions in an AOST txn.

[andreimatei]

The way I see it, if it is wrong, then it should be enforced just like a set transaction_read_only=true is enforced - and I think that should be enforced by SQL. As far as KV is concerned, I don't think we should tie functions that offer some control over the timestamp (SetFixedTimestamp() \ CommitTimestampFixed()) with read-only semantics.

But I don't necessarily think that it's wrong for AOST transactions to write (although perhaps you should opt into it). If what you want is to write based on data from the past, I think there are use cases for that. The writes will fail, of course, if they're trying to overwrite more recent writes. But if you're writing to a unique row (say report_20200123), it should work. Of course, it'd be cool to also offer a way to read in the past and write in the present (i.e. specify AOST on a subquery).

[andreimatei]

• I should have said that the writes will also fail if what you're trying to write has been read more recently, or if a GC has run.

[tbg]

Even if we had an API for read-only txns in place, we would want awareness in SQL to avoid writing in such transactions (clearly broken right now). My personal preference would be to focus on fixing that. After that point, any kind of check we add to in *client.Txn is just an extra layer of safety, and I'd rather place that assertion in SQL to not jumble *client.Txn up more. That said, if we did revisit the client.Txn API, I could see us wanting to add true read-only transactions, and we'd certainly want to instantiate one for AOST. Andrei is right that there's nothing wrong technically with writing after fixing a timestamp, but I do think we'd want to stay away from it in practice - I don't see that this would have a place in a clean API.

[knz]

I think I now want both levels of checks. I'll look into it.

[andreimatei]

Andrei is right that there's nothing wrong technically with writing after fixing a timestamp, but I do think we'd want to stay away from it in practice - I don't see that this would have a place in a clean API.

The clean API would be one that separates the concerns - fixing of the timestamp vs protection against programming errors by refusing some operations. It's one thing to not particularly try to support features around "writing in the past", but it's another thing to disallow them by fiat.

My worry with starting to teach KV about read-only txns is that there's a logical level and there's a physical level and both could in theory be desired, and I'm afraid of trying to decide which is which. Should a read-only txn push other txns? Should a read-only transaction clean up intents from other transactions? Should it cause range leases to be acquired? Should it causes liveness records to have their epochs bumped?

[knz]

It's one thing to not particularly try to support features around "writing in the past", but it's another thing to disallow them by fiat.

I'll grant you that.

Should a read-only txn push other txns? Should a read-only transaction clean up intents from other transactions? Should it cause range leases to be acquired? Should it causes liveness records to have their epochs bumped?

I think all these are valid questions but somewhat orthogonal. Once we have introduced the notion of read-only txns, we can decide (later) whether/how we can optimize them further.

[andreimatei]

A side note on the transaction_read_only session variable:
The transaction_read_only variable is weird. It doesn't seem to be documented much in PG, as far as I can find. But I've played around, and it seems to affect only the current transaction; it gets reset afterwards. If you run it outside of a txn, it basically has no effect. Postgres doesn't let you set a value for this on the connection string.
In Postgres, this variable seems to have a role for read-only replicas.

For us a value coming from the connection string, through Defaults["transaction_read_only"] does not dictate the read-only setting of new transactions on that connection. It does, however, control RESET transaction_read_only, which is probably a bad thing, but also probably not worth the specific code to prevent it (I guess we'd want a mechanism to say that transaction_read_only is configurable, but not accepting values through the connection string.

[github-actions]

We have marked this issue as stale because it has been inactive for
18 months. If this issue is still relevant, removing the stale label
or adding a comment will keep it active. Otherwise, we'll close it in
10 days to keep the issue queue tidy. Thank you for your contribution
to CockroachDB!