I mean this could lead to serious bugs. What can be a way to detect these using linters or in CI before they hit the production.
We've moved to MSSQL due to several reasons including customer demand.
We're experiencing the MSSQL query planner occasionally generating terrible plans for core queries which then gets cached, leading to support calls.
Workaround for now has been to have our query geneator append a fixed-value column which and have it change the value every 10 minutes, as a cache defeat.
Still, surprised the engine doesn't figure this out itself, like try regenerating plans frequently if they contain non-trivial table scans say.
Or just expire cache entries every 15 minutes or so, so a bad plan doesn't stick around for too long.
…but MSSQL is still a fantastic database, if you can afford it. Postgres and mysql come with their own set of gotchas, some of which need an actually decent book to be explained. (Note all the RDBMS manuals are decent books and everyone without exception should read at least the TOC of the db they’re using, which IME is still a rarity.)
Why should I have to guard against poor optimization decisions when the whole point is to just let the DB figure out the best way?
I haven't used MSSQL for some time now, but I'm sure there are tools which do exactly what you say you want, i.e. continuously monitor the DBMS and suggest actions or even act itself. This is usually done by a team of DBAs, though.
So my point was that if they want to keep parameter sniffing as default then short cache expiration can at the very least mitigate the performance cliffs that can occur. Better to have a slow query for 10-15 minutes than for hours. Recompiling once in a while shouldn't have a significant performance impact.
In general though I think in many cases Carmack's approach when he made Quake's renderer is a good one.
That is, it's often better to use an algorithm which is on average slightly slower but has much lower variance in execution time, than one which is on average slightly faster but has much greater variance. Parameter sniffing seems to fall into the latter category.
Optimal you then combine this with automatic collection and analysis of log traced to detect performance regression (which is hard but just because of the involved tooling but now it needs a CI env with reliable perf. characteristics e.g. not GitHub hosted GitHub action runners).
Through in this case the problem is a trap in the behavior of `IN (...)` sub queries which probably is not specific to WITH clause and might be as deeply rooted as the SQL standard but a you have to pay for being able to read it I'm not sure.
The blog post doesn’t really give answers - it isn’t obvious to me that the second query can’t be executed in the exact same way. The even cop to this fact:
> This structure appears less ambiguous to the planner and typically encourages it to evaluate the subquery first.
So then - their bug still exists maybe?
I have thoughts - probably we do expect it never should delete multiple rows, but after considering the query plan, I can see it being ambiguous. But I would have expected Postgres to use only a single interpretation.
> If sorting is not chosen, the rows will be returned in an unspecified order.
The original query from TFA could've instead just had the uncorrelated subquery moved into a materialized CTE.
[1] https://www.postgresql.org/docs/current/queries-order.html
Under a different plan, the inner query would only be evaluated once, it is hard to mentally parse how it will first find the rows with the group id and then pass into the sub query.
And still I am not sure how using a CTE or not in the manner in the post is supposed to avoid this issue, so now I’m a bit skeptical it does. I see how a sort would.
I hope if the sub query was its own CTE, the limit would be applied correctly, but am no longer sure… before this post I wouldn’t have questioned it.
Edit to say - now I see you need to explicitly use AS MATERIALIZED if you bump the subquery to a CTE. Someone should really write a better blog post on this… it raises an interesting case but fails to explain it… they probably have not even solved it for themselves.
I will look into a more thought out post perhaps. Thanks for sharing all the comments and feedback. Really enjoying the discussions.
It is still a good rule of those to pair your LIMITs with ORDER, however, yeah.
In this case, the number of rows changing given the same input dataset, is a bug.
Regarding selects: [0]: “A key property of WITH queries is that they are normally evaluated only once per execution of the primary query… However, a WITH query can be marked NOT MATERIALIZED to remove this guarantee. … By default, a side-effect-free WITH query is folded into the primary query if it is used exactly once in the primary query’s FROM clause.
Regarding CTEs: [1]: “A useful property of WITH queries is that they are normally evaluated only once per execution of the parent query… However, the other side of this coin is that the optimizer is not able to push restrictions from the parent query down into a multiply-referenced WITH query"
Now, in either case - if you don't want the planner to inline the query - you might have to be explicit about it (I think since postgres 10?), or otherwise - yes, the output of the query will depend on the plan and this is allowed based on the docs.
[0]: https://www.postgresql.org/docs/current/sql-select.html
[1]: https://www.postgresql.org/docs/current/queries-with.html
For instance, with the stand alone query:
DELETE … WHERE id IN (
SELECT id … LIMIT 1 FOR UPDATE SKIP LOCKED
)
E.g., for a very simple case, in SELECT * FROM a,b WHERE random() < 0.5, would you push random() down through the join or not? To one table? Both tables? Evaluate it once at the start of a query because it depends on neither a nor b? What if it's random() < a.x? Different databases have different and often poorly-defined semantics here.
I had to check because for some reason, I always thought =ANY was somehow better than IN.
DELETE FROM task_queue
WHERE id = ( -- Use '=' for a single expected ID
SELECT id FROM task_queue
WHERE queue_group_id = 15
LIMIT 1
FOR UPDATE SKIP LOCKED
)
RETURNING item_id;
I guess it must be an unmentioned column on that task_queue table?
There are two factors here.
The subquery
SELECT id FROM task_queue WHERE queue_group_id = 15 FOR UPDATE SKIP LOCKED LIMIT 1
Otherwise, you need to force postgres to evaluate your subquery exactly once by materializing it. There are different ways this might be accomplished - the blog post does this incidentally by using `=`. But it is not the only way to tell postgres that.
For instance, like this. But it is fragile - without AS MATERIALIZED, it could be run more than once.
WITH candidate AS MATERIALIZED (
SELECT id FROM task_queue WHERE queue_group_id = 15 FOR UPDATE SKIP LOCKED LIMIT 1
)
DELETE FROM task_queue t USING candidate c WHERE t.id = c.id
RETURNING t.item_id;WITH deleted_tasks AS MATERIALIZED (
SELECT id
FROM task_queue
WHERE queue_group_id = 5
AND status = 'pending'
LIMIT 1
FOR UPDATE SKIP LOCKEDDELETE FROM task_queue WHERE id IN (SELECT id FROM deleted_tasks) RETURNING item_id, id;
Personally, if I were going to put any part of that query in a CTE, it would have been (just) the select, as I generally prefer CTE's to subqueries.
I'm really not sure what motivated the author to create the CTE in the first place, as the final select seems to be essentially the identity function.
Y'all might find the term "correlated subquery" helpful here.
When I want them, I always get them. Sometimes I get them when I don't want them. Sometimes it's even my fault, not the result of the query planner looking at me sideways.
We're likely seeing a simplified example. All queries should be able to be embedded in a CTE without integrity loss.
The query reads to me like a conceptual mish-mash. Without understanding what the innermost `SELECT` was meant to accomplish, I'd naturally interpret the `WHERE id IN (...)` as operating on a set. But the most sacrilegious aspect is the inclusion of `FOR UPDATE SKIP LOCKED`. It assumes a very specific execution order that the query syntax doesn't actually enforce.
Am I right to think that not avoiding lock contention, i.e. omitting `SKIP LOCKED` would have actually produced the intended result?
I will give OP the benefit of the doubt and say that automated testing did not catch this because the optimisations depend on table statistics, and not because it was not appropriately covered.
The SQL standard does not specify how many times a subquery must or can be run. In this case, Postgres is free to run the uncorrelated subquery many times. Unfortunately, the subquery result is not idempotent.
The real solution is not to rely on the optimizer for the correctness of your query. The query should be logically correct, irrespective of the query plan, right? The correct solution would be to ensure that the subquery runs only once, using a materialized cte or temp table. Do not base your "solution" on the discovery that for this particular version of Postgres, the planner runs this type of subquery once for the '=' operator and multiple times for 'IN'.
However, I think it's telling that almost every single comment here has a different understanding of how PostgreSQL works, and nobody can link to any documentation that conclusively addresses this case. I think that indicates that this is a failing if not of implementation, then of documentation.
For example: another comment says that `=` forces PostgreSQL to evaluate the subquery exactly once. I couldn't find any justification for that statement. Is it guaranteed? I don't know.
Another comment says "if only the subquery was ordered then the result would be consistent", but AFAIK, this is not true in the presence of "SKIP LOCKED".
I think the problem stems from the fact that the SQL standard is written from an academic point of view: the ordering of execution of subqueries is not defined because it is irrelevant within the relational algebra, so optimizations which change the execution order are just that: optimizations, with no observable side effects.
Real database systems do not actually have the nice algebraic properties of the relational algebra... The same subquery may give different results over multiple executions, etc. Given this, it's important that the database system give sufficient and sufficiently clear guarantees such that they can still be used correctly, and so that the optimizations implemented by the system can be justified.
The reason = mean it runs once is because the outer query will only run once, and in this case that query, when using =, can only delete based on a single id. But if that outer query was subquery in a context where it could be run more than once, you are back to where you started. Hence me saying their fix was sort of incidental.
First, let's make sure we're talking about the same two examples.
A:
DELETE FROM task_queue
WHERE id = (
SELECT id FROM task_queue
WHERE queue_group_id = 15
LIMIT 1
FOR UPDATE SKIP LOCKED
)
DELETE FROM task_queue
WHERE id IN (
SELECT id FROM task_queue
WHERE queue_group_id = 15
LIMIT 1
FOR UPDATE SKIP LOCKED
)
Here's my logic:
- In both cases, the outer query is run once.
- In both cases the outer WHERE clause is evaluated for each row in `task_queue`.
- In both cases, it is up to the optimizer whether the result of the subquery is materialized or not.
- In both cases, if the subquery is not materialized, multiple rows in the outer query may match the condition.
In practice, it may be that the optimizer always materializes uncorrelated subqueries on the RHS of an `=` expression. My contention is whether that is a formal guarantee.
This cannot match more than one X at a time. So that forces the inner query to be run once, as we can only have one id, and running it twice may produce 2.
I am not sure though to be honest.
…
We are too deep for me to reply now, but to your next comment I didn't mean only one row, but only one id. It is easy for a small difference in word choice to get things wrong.
I think if it did return two rows; ie limit 2- the query with = will fail. Hmmm maybe that will happen even with limit 1 under certain plans. I wouldn’t trust it.
For example:
DELETE FROM example WHERE id = (SELECT RANDOM(0, 100) FROM other_table LIMIT 1)
wordofx•1mo ago
No need for the CTE to select everything…
tomnipotent•1mo ago