SpiceDB is an open source, Google Zanzibar-inspired, database system for creating and managing security-critical application permissions.

SpiceDB

hey guys. is there some kind of playground history or something like that ? I just accidently loaded an example and deleted my open playground

Sooooo
v1.18.0 has made this issue reappear: https://github.com/authzed/spicedb/issues/486 😅 
I've seen my response time randomly shot through the roof (P99 20ms -> many seconds, maybe minutes). Took me a while but apparently I learn from the past, setting `plan_cache_mode` to `force_custom_plan` fixed it.
Can we do the same fix as last time in SpiceDB? Disabling plan caching when calling out to pgx IIRC

Note that disabling caching (ie avoiding prepared statements) isnt' free: my P99 has increased significantly (~20ms -> 30ms). It's still acceptable latency to me, and obv much better than many seconds

Also, I think you have some automated testing on query plans: not sure they take this behaviour of prepared statements into account

<@760445414113280000> will see where it went in an hour or so and readd a test to ensure it doesn't go aways again

<@760445414113280000> did you happen to see which query was being slow again?

last time we only had to do it for a single query

so I'd rather do that then for all of them

Ah yeah of course, didn't even think of specifying that

it was 
```sql
SELECT namespace, object_id, relation, userset_namespace, userset_object_id, userset_relation, caveat_name, caveat_context FROM relation_tuple WHERE pg_visible_in_snapshot(created_xid, $1) = $2 AND pg_visible_in_snapshot(deleted_xid, $3) = $4 AND namespace = $5 AND relation = $6 AND object_id IN ($7) AND ((userset_namespace = $8 AND userset_object_id IN ($9) AND userset_relation = $10)) LIMIT 9223372036854775807
```

interesting that it is a relationship lookup this time

I haven't seen a fix commit for this issue but I could have missed it: has this been fixed?

we haven't fixed this yet, no; trying to decide how best to address it because it doesn't appear to be a single query but various ones

so we'd likely need to turn off the prepared statements entirely

but that means adding an override to disable that if need be

> so we'd likely need to turn off the prepared statements entirely
That sounds like a much better default to me TBH, I think `pgx` is overreaching with this prepared statement behaviour by default

<@760445414113280000> are you turning prepared statements off entirely yourself right now and, if so, how?

I am, with the `plan_cache_mode=force_custom_plan` postgres param

Well actually no, I'm not turning off prepared statement, I'm turning off generic plans for prepared statements

so <@760445414113280000> thoughts on hypothetically turning this param on by default?

I have concerns about negative perf impact for people not running into the problem with the custom plans...

I don't know if that's something that you can enable on spicedb side, really? You might have to make it a prerequisite, which is going to be a huge gotcha

And it would mean that we let pgx use prepared statement as an optimisation, but we compensate the optimisation negatively by disabling custom plans, it's just adding more complexity for nothing 🙂

This prepared statement behaviour is an optimisation, I say that not using it is a "penalty"

FWIW I'm getting a P99 of 16ms with the custom plans disabled and 400 req/s, which is very acceptable perf-wise

the downside of turning off prepared statements is that pgx falls into "raw" mode

I mean... P99 of 16ms seems fine to me 🙂

concerns about proper escaping of variables during query construction

Surely pgx has a way to pass parameters separately without using prepared statements?

but if the plans are the problem, using prepared statements still seems safe

The whole problem is that PG computes a buggy generic plan for the prepared statement

yeah, but turning that off still does get the security benefits of a prepared statement

True, but there's no reason in theory that we _need_ prepared statements to get these security benefits 😄 it's possible to pass parameters to postgres separately from the query without using prepared statements

Using prepared statement _for_ this security benefit seems like using the wrong tool for the job

> it's possible to pass parameters to postgres separately from the query without using prepared statements
(IDK if pgx exposes this capability though)

Seems like it's possible to disable statement caching: https://pkg.go.dev/github.com/jackc/pgx/v4#ParseConfig

not 100% sure that's really what we're after

```
    // PreferSimpleProtocol disables implicit prepared statement usage. By default
    // pgx automatically uses the unnamed prepared statement for Query and
    // QueryRow. It also uses a prepared statement when Exec has arguments. This
    // can improve performance due to being able to use the binary format. It also
    // does not rely on client side parameter sanitization. However, it does incur
    // two round-trips per query and may be incompatible proxies such as
    // PGBouncer. Setting PreferSimpleProtocol causes the simple protocol to be
    // used by default. The same functionality can be controlled on a per query
    // basis by setting QueryExOptions.SimpleProtocol.
```

> It also
>     // does not rely on client side parameter sanitization.

Yeah it does seem risky. I don't think we should need to opt out of the extended protocol, it would also seem like a workaround

`BuildStatementCache` to `nil` might be what we want?  https://pkg.go.dev/github.com/jackc/pgx/v4#ConnConfig

but isn't the issue on the PG side? removing the cache client side would still make PG use the prepared statement?

or do you suspect that PGX would have to make a *new* prepared statement everytime and somehow PG would not hit the corner case on it as a result?

What I'm understanding (but I'm really not sure) is that what pgx calls "caching" is just "using prepared statements"

it does say `Set to nil to disable automatic prepared statements` in the doc of `BuildStatementCache`

this bit of docs also suggests that "statement caching" and "automatic prepared statements" are the same thing: https://pkg.go.dev/github.com/jackc/pgx/v4#hdr-Prepared_Statements

I wonder if there is a way to tell from the queries being made

unfortunately, no. Playground should have warned you before you replaced it?

do I have your permission to use a simplified version of your test schema in our test suite?

yeah it warned me. But i was to fast and just clicked it anyway :/. No worries not much is lost

well, I'll see if we can add a history of at least a few revisions then 🙂

feel free to do whatever you want with the playground - I've this stuff in my local repository, so no worry 😄

note to this behaviour: CHECK seems to work fine, even if the resource is not listed by lookup-resources

so a check on the missing "asset:hidden-1" yields "true" (correct).

so, the validation does cover this behaviour

<@767440806628950016> https://github.com/authzed/spicedb/pull/1226

<@767440806628950016> there are also ways to simplify your schema (which would have avoided this bug too), but I'm quite glad you found it

sure I'm interested in simplifying the schema!
how to?

you're walking to the `parent` container multiple times

you should define a single, combined permission on the `container`, and then arrow to that

rather than arrowing to the individual ones

`parent->can_read + hidden->can_read + parent->can_read_hidden + hidden->can_read_hidden`

you could do `parent->can_read_and_hidden` and define that on `container`

(I think you need to provide a new link for those following along at home 🙂 )