refactor: get rid of RoleSettings cache in Config#5020
Conversation
Build the transaction GUC setup as a static SQL statement that binds setting names and values as two text arrays. The change is to avoid growth in the number of prepared statements for each combination of preferred timezone, role settings, application settings, and function settings. refactor: encode transaction settings as binary text arrays Encode the transaction setting name/value arrays as binary text[] parameters while keeping the executed SQL static. This lets PostgreSQL apply its normal text receive/client_encoding conversion rules for each array element, avoiding app-side UTF-8 conversion and text array literal escaping for the bound setting arrays. refactor: use postgresql-binary array encoder Replace the local text array binary encoder with postgresql-binary array_foldable while keeping transaction setting values as raw text element payloads. This avoids maintaining the array header and element loop in PreQuery directly.
c0ea9bb to
dafd472
Compare
mkleczek
changed the title
perf: memoize config dependent PreQuery state
dafd472 to
d3db279
Compare
|
Did not look at the diff, yet, but if we do this, then we can't make full use of pipelining mode later, right? That's because we need to read the results of that first step before preparing the second one. |
No, this change is API compatible with what we have (ie. pre-query is still I just moved role settings read as part of pre-query. |
| , configDbRootSpec = Nothing | ||
| , configDbSchemas = fromList ["test"] | ||
| , configDbConfig = False | ||
| , configDbHasParameterPrivilege = False |
There was a problem hiding this comment.
I think AppConfig is the wrong place to store the cached result of a version check.
| if configDbConfig then | ||
| lmap (const roleSettingsParams) $ SQL.Statement (txSettingsSqlWithRoleSettings configDbHasParameterPrivilege) txRoleSettingsParams HD.noResult configDbPreparedStatements | ||
| else | ||
| lmap (const settings) $ SQL.Statement txSettingsSqlWithoutRoleSettings txSettingsParams HD.noResult configDbPreparedStatements |
There was a problem hiding this comment.
Please note that postgrest-loadtest sets PGRST_DB_CONFIG=false, so the loadtests won't ever exercise the code path with the new, longer SQL.
Edit: The reason is that the loadtest is not designed / supposed to test performance of SQL queries. It is supposed to test the performance of Haskell code, not the whole thing. The right thing to do is to benchmark this with postgrest-benchmark.
There was a problem hiding this comment.
Please note that
postgrest-loadtestsetsPGRST_DB_CONFIG=false, so the loadtests won't ever exercise the code path with the new, longer SQL.
That would explain why there is no difference :D, thanks!
Edit: The reason is that the loadtest is not designed / supposed to test performance of SQL queries. It is supposed to test the performance of Haskell code, not the whole thing.
I am not sure if that's the right decision as Haskell code might change the generated SQL queries... OTOH that's quite rare.
There was a problem hiding this comment.
I am not sure if that's the right decision as Haskell code might change the generated SQL queries... OTOH that's quite rare.
That's not a decision made purely because I want it that way... I mean we discussed this at length, it all comes down to creating as little variance as possible from our side, because we have so much variance and so little resources in GHA. Maybe it can be done better with #5015, not sure.
There was a problem hiding this comment.
The reason is that the loadtest is not designed / supposed to test performance of SQL queries. It is supposed to test the performance of Haskell code, not the whole thing
@wolfgangwalther So perhaps we should modify the loadtests to not run the SQL at all? One idea could be to have them return application/sql (#3580) so they still go through the whole Haskell pipeline.
to not run the SQL at all?
To be clear, SQL would be ran only at startup to set up the schema cache but not during requests.
There was a problem hiding this comment.
So perhaps we should modify the loadtests to not run the SQL at all? One idea could be to have them return
application/sql(#3580) so they still go through the whole Haskell pipeline.
Yeah, I like that 🚀. That fits both my mental model of what the loadtests should do, plus it should certainly help to reduce noise in CI.
3 participants
No description provided.