https://authzed.com logo
Title
w

williamdclt

03/24/2023, 4:11 PM
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

Joey

03/24/2023, 4:25 PM
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

williamdclt

03/24/2023, 10:03 PM
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
j

Joey

03/24/2023, 10:04 PM
thanks PG! /s
interesting that it is a relationship lookup this time
w

williamdclt

04/11/2023, 9:47 AM
I haven't seen a fix commit for this issue but I could have missed it: has this been fixed?
j

Joey

04/11/2023, 2:55 PM
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

williamdclt

04/11/2023, 3:38 PM
> 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

Joey

04/11/2023, 3:44 PM
@williamdclt are you turning prepared statements off entirely yourself right now and, if so, how?
w

williamdclt

04/11/2023, 3:46 PM
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
j

Joey

04/11/2023, 3:46 PM
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

williamdclt

04/14/2023, 2:17 PM
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

Joey

04/14/2023, 2:21 PM
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

williamdclt

04/14/2023, 2:34 PM
what's the problem of raw mode?
j

Joey

04/14/2023, 2:38 PM
concerns about proper escaping of variables during query construction
w

williamdclt

04/14/2023, 2:38 PM
Oh I see
Surely pgx has a way to pass parameters separately without using prepared statements?
j

Joey

04/14/2023, 2:40 PM
that's the question
but if the plans are the problem, using prepared statements still seems safe
w

williamdclt

04/14/2023, 2:42 PM
The whole problem is that PG computes a buggy generic plan for the prepared statement
j

Joey

04/14/2023, 2:43 PM
yeah, but turning that off still does get the security benefits of a prepared statement
w

williamdclt

04/14/2023, 2:45 PM
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

Joey

04/14/2023, 2:46 PM
yeah, that's what I want to determine
whether pgx uses it
just need to check it
w

williamdclt

04/14/2023, 2:49 PM
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

Joey

04/14/2023, 2:51 PM
or use PreferSimpleProtocol
// 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

williamdclt

04/14/2023, 2:53 PM
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

Joey

04/14/2023, 2:55 PM
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

williamdclt

04/14/2023, 3:50 PM
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

Joey

04/14/2023, 4:22 PM
hrmph
yeah...
I wonder if there is a way to tell from the queries being made