Title
w
williamdclt
03/24/2023, 4:11 PMSooooo
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_modeforce_custom_planNote 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 PMyeah, 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 PMAh 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 9223372036854775807j
Joey
03/24/2023, 10:04 PMthanks PG! /s
interesting that it is a relationship lookup this time
w
williamdclt
04/11/2023, 9:47 AMI 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 PMwe 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
pgxj
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 PMI am, with the
plan_cache_mode=force_custom_planWell 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 PMoh
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 PMI 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 PMthe 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 PMwhat's the problem of raw mode?
j
Joey
04/14/2023, 2:38 PMconcerns about proper escaping of variables during query construction
w
williamdclt
04/14/2023, 2:38 PMOh I see
Surely pgx has a way to pass parameters separately without using prepared statements?
j
Joey
04/14/2023, 2:40 PMthat's the question
but if the plans are the problem, using prepared statements still seems safe
w
williamdclt
04/14/2023, 2:42 PMThe whole problem is that PG computes a buggy generic plan for the prepared statement
j
Joey
04/14/2023, 2:43 PMyeah, but turning that off still does get the security benefits of a prepared statement
w
williamdclt
04/14/2023, 2:45 PMTrue, 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 PMyeah, that's what I want to determine
whether pgx uses it
just need to check it
w
williamdclt
04/14/2023, 2:49 PMSeems 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 PMor 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 PMYeah 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
BuildStatementCachenilj
Joey
04/14/2023, 2:55 PMbut 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 PMWhat 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 statementsBuildStatementCachethis 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 PMhrmph
yeah...
I wonder if there is a way to tell from the queries being made