refactor: bind transaction settings in static statement#5010
Conversation
mkleczek
force-pushed
the
push-opzmrxsnynvy
branch
4 times, most recently
from
f72e713 to
d451524
Compare
| r".+: select set_config\(setting, value, true\) " | ||
| r"from unnest\(\$1::text\[\], \$2::text\[\]\) with ordinality as _\(setting, value, ordinality\) " | ||
| r"order by ordinality" |
There was a problem hiding this comment.
I'm a bit lost here, so this involves changing the emitted SQL?
What is the advantage? So far I'm not seeing a reduction in LOC.
There was a problem hiding this comment.
What is the advantage? So far I'm not seeing a reduction in LOC.
Haven't looked at the changes, yet, but the PR body reads:
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.
That seems like a good thing?
There was a problem hiding this comment.
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.
That seems like a good thing?
Right, @mkleczek it would be good to know what is the resultant query after this on the PR description or commit message, so it's easier to understand.
wolfgangwalther
left a comment
wolfgangwalther
left a comment
There was a problem hiding this comment.
I believe once the unused functions in SqlFragment are removed, it should be visibly simpler overall. Plus, the reduction of prepared statements is a clear win.
👍 for the concept.
| import PostgREST.Query.SqlFragment (escapeIdentList, fromQi, | ||
| intercalateSnippet, | ||
| setConfigWithConstantName, | ||
| setConfigWithConstantNameJSON, | ||
| setConfigWithDynamicName) |
There was a problem hiding this comment.
I did not look in-depth, but I would expect some (all?) of these function to be deleted from the SqlFragment module now.
I would not expect them to be used elsewhere.
mkleczek
force-pushed
the
push-opzmrxsnynvy
branch
7 times, most recently
from
41b782b to
0f7937d
Compare
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.
mkleczek
force-pushed
the
push-opzmrxsnynvy
branch
from
0f7937d to
73b2f8d
Compare
3 participants
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.