-
Notifications
You must be signed in to change notification settings - Fork 26
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
with parallel queries, all workers report the quals #9
Comments
Yes, that's indeed badly broken. I think this issue is linked to the one you raised few days ago on https://www.postgresql.org/message-id/3f62f24e-51b3-175c-9222-95f25fd2a9d6@2ndquadrant.com? I didn't have time to thoroughly read that thread yet, but I think the last patch you proposed for auto_explain would also be suitable for pg_qualstats (especially since it already has to be in shared_preload_libraries), unless this is somehow fixed in the parallel DSM machinery? |
Yes, it's essentially the same issue. I'm not sure whether to wait for the DSM. That might be the "correct" solution but hard to say when it's going to be available. However this extension already uses shared memory (and has to be loaded using shared_preload_libraries), so while adding the flags to the shared state is not quite pretty, it's probably a viable temporary fix. |
I agree. I took a closer look at your upstream patch, and I wonder about this: +static Size
+explain_shmem_size(void)
+{
+ return offsetof(AutoExplainState, sampled) + MaxConnections * sizeof(bool); The total number of connections is MaxConnections + autovacuum_max_workers + 1 + max_worker_processes I think autovacuum (launcher and workers) shouldn't go through ExecutorStart (not really sure though), but you can have custom bgworkers that run queries, so we should instead do +static Size
+explain_shmem_size(void)
+{
+ return offsetof(AutoExplainState, sampled) + (MaxConnections + autovacuum_max_workers + 1 + max_worker_processes) * sizeof(bool);
}
[...]
+ if (!found)
+ {
+ /* First time through ... */
+ explainState->lock = &(GetNamedLWLockTranche("auto_explain"))->lock;
+ memset(explainState->sampled, 1, sizeof(bool) * (MaxConnections + autovacuum_max_workers + 1 + max_worker_processes));
+ } Also, it looks like you defined UPDATE: taking a closer look on MyBackendId calculation, it's necessary to also reserve space for autovacuum worker and launcher, and perhaps additional +1 to keep code simpler, since it starts at 1, not 0. |
Yeah, the patch was more of a PoC than anything else, so you're right the fixes you propose are needed. If you want, feel free to submit a fixed version of the auto_explain patch, although I'm rather skeptical about this approach being accepted in that case (because it require using the shared_preload_libraries, which is not quite desirable). |
I agree that this approach will probably not be accepted for auto_explain, however any other extension writer facing the same issue will probably notice the thread, so having a patch with these fixes could be worthwhile. Anyway, I just pushed dalibo@80f7e06 based on your patch. Thanks! |
With parallel queries on PostgreSQL 9.6, pgqs_ExecutorEnd() is executed by both the leader and parallel workers, accounting for the quals from both leader and the workers. With sampling, this behaves unpredictably because random() is evaluated in each process independently, and thus only a subset of processes will do the reporting.
The text was updated successfully, but these errors were encountered: