https://authzed.com logo
Join Discord
Powered by
Sooooo v1.18.0 has made this issue reappear: https...
# spicedb
w
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
j
yeah, weird
I swore we had a test for this
huh
that got removed somehow...
@Jake ^
@williamdclt will see where it went in an hour or so and readd a test to ensure it doesn't go aways again
@williamdclt 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
w
Ah yeah of course, didn't even think of specifying that
it was
Copy code
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
j
thanks PG! /s
interesting that it is a relationship lookup this time
w
I haven't seen a fix commit for this issue but I could have missed it: has this been fixed?
j
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
w
> 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
j
@williamdclt are you turning prepared statements off entirely yourself right now and, if so, how?
w
I am, with the 
plan_cache_mode=force_custom_plan
postgres param
Well actually no, I'm not turning off prepared statements per se, I'm turning off generic plans for prepared statements
j
oh
that seems good
so @williamdclt 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...
w
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
j
the downside of turning off prepared statements is that pgx falls into "raw" mode
and I'd prefer not to do that
I mean... P99 of 16ms seems fine to me 🙂
w
what's the problem of raw mode?
j
concerns about proper escaping of variables during query construction
w
Oh I see
Surely pgx has a way to pass parameters separately without using prepared statements?
j
that's the question
but if the plans are the problem, using prepared statements still seems safe
w
The whole problem is that PG computes a buggy generic plan for the prepared statement
j
yeah, but turning that off still does get the security benefits of a prepared statement
w
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)
j
yeah, that's what I want to determine
whether pgx uses it
just need to check it
w
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
j
or use PreferSimpleProtocol
Copy code
// 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.
that's my concern about using it :/
w
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
j
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?
w
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
but it's not overly clear
j
hrmph
yeah...
I wonder if there is a way to tell from the queries being made
4 Views
PreviousNext
Powered by