query_analysis: qualify pragmas for reads/writes

[psarna]

This commit changes the way PRAGMA statements are qualified: instead of treating all of them as writes, read-only pragmas are qualified as reads, so that they are available for connections that authenticated with a read-only token.

[psarna]

@MarinPostma is it in line with your previous analysis, or should it be more strict? In particular, I wonder if pragmas that optionally take parameters and are given the parameters, should be treated as writes or refused. I think all the forbidden pragmas should just be put in the // changes the state of the connection, and can't be allowed rn branch, so please check if it makes sense.

My main rationale is to allow read-only token holders to call table_xinfo(something), it will be a valid use case for browser apps that communicate directly with sqld/Turso

[MarinPostma]

this is not correct. All of those are read-only if they don't have params. If they have params, then they write, and we can't support that currently, and without careful consideration

[psarna]

Right, but we wanted to support at least some write pragmas, like foreign_keys, for the sake of atlas integration? @haaawk was it the case for Atlas?

Otherwise, I'm fine with refusing everything else and just returning None here

[psarna]

*encoding would be special-cased too, since it's write only when the db hasn't been created yet, which is never the case in sqld

[MarinPostma]

let's add a specific case for foreign key, yes, and writable_schema as well

[MarinPostma]

but notice that: if you set the foreign_key, then this is a write and is executed on the primary, now, if you read foreign_key, it's categorized as read, and read from the replica, which has a different state.

So all pragmas need to be executed on the primary. Let's add a PRAGMA category, and send that to the primary

[psarna]

So all pragmas need to be executed on the primary.

I think this is too strong a statement. Some pragmas behave like that - probably all the ones that can either set or read parameters. But there's also a class of pragmas that are only readonly and it's fine to send them to replicas - e.g. ones that return schema information. I believe these should be allowed to be executed on replicas too

[MarinPostma]

I think it's a lot of complications for not many benefits. If you're willing to put the time and effort to make it work correctly, then go for it, otherwise, I think all pragmas on primary is an acceptable tradeoff

[psarna]

A special category for pragmas that are reads-but-should-happen-on-primary sounds good in general though. I think it's not really different from reading and writing without a transaction, just a snapshot isolation thing, but no harm in making pragmas resilient to that

[psarna]

@MarinPostma ultimately the current way is safer, so let's go with that until, if ever somebody with a read-only token complains. Meanwhile I'll just rework this PR to have the same semantics as yours, but with the new structure, because it will make it easier for me to try and figure out a correct solution for RO pragmas as a low priority weekend project, wdyt?

[psarna]

oor maybe I'll just not resist myself and add a short list of pragmas I'm sure are reads, and let you review.

[psarna]

@MarinPostma v2, more conservative, I only picked a small subset of pragmas I vouch for being read-only. One important thing is that I brought back module_list, which was explicitly allowed in #356 and wasn't listed after #360, which is technically a very minor regression. (And I think #356 was based on users' feedback)

[MarinPostma]

bors merge

[bors]

Build succeeded:

• Build Docker
• Run Checks
• Run Clippy
• Run Rustfmt
• Run Tests