-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
README: add caveat (prepared queries) #36
Conversation
README.md
Outdated
`pg_query_settings` doesn't work well with **prepared queries**. More specifically: | ||
for a given parameter `P`, if the generic plan is selected, and if the executor | ||
needs to fetch the value of `P`, then it won't get the value specified in the | ||
`pgqs_config` table. For example, the executor fetches the value of `work_mem` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the example. It seems a bit complex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about this?
For example, the executor fetches the value of work_mem
in order to decide whether to perform an in-memory sort, but at this point the value would have been reset to its default (at the end of the prepare
statement). There's no problem with parameters that only affect planification, such as max_parallel_workers_per_gather
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to explain in details what's wrong with planed queries? The first sentence itself is enough.
If you want, you can document in details the technical problem in source code itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the last commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I keep thinking you don't need to add so much details in the README. It just doesn't work well with prepared queries, so prepared queries are buggy and should be considered as not supported yet. No need to explain why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I agree, I put everything as code comment and only the mininum in README
0578158
to
a46489c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good for me.
No description provided.