Since I don't often write raw SQL, I can only assume the author named their CTE `deleted_tasks` to elucidate that the query might delete multiple items. Otherwise, it makes little sense, for they intended to "pop" a single row, and yet their aptly named `deleted_tasks` ended up removing more than one!
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?
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'.
I think arguably there is no bug here, but the blog doesn't do a good job of explaining the issue and their fix may work because they remove the CTE and switch to `=`, but they prove they don't understand why the difference when they suggest "using id IN (...) might still be necessary"; if you do that, then the problem will return.
There are two factors here.
The subquery
SELECT id FROM task_queue WHERE queue_group_id = 15 FOR UPDATE SKIP LOCKED LIMIT 1
can return different ids each time you run it. If it was ordered, then it would always return the same id and if postgres optimized in a way that it runs more than once it would just get the same result each time anyway.
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;
This is interesting - I don’t expect undefined behavior like this. Should the query delete more than one row or not?
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.
It's not undefined behavior, it's the fact that the uncorrelated subquery within the CTE doesn't specify an ordering, therefore it cannot be implicitly materialized / evaluated once. Postgres documentation is clear here [1]:
> 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.
I see what you saying, but it is very subtle, wouldn’t you agree?
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.
Our curiosities are very aligned here. I wrote this post on a bus, and was rushed, so I agree it’s intentionally lacking some more critical details. The use of ”=“ solved the issue at the time (understandably why), but I haven’t had the time to do more thorough deep dive.
Hopefully soon!
Thank you for sharing your thoughts and comments. I went on a bit of a similar roller coaster :D
It doesn't matter. The output of a query cannot depend on the plan. All plans should generate semantically equivalent output necessarily. Notice I say semantically equivalent because obviously
Select random() can return different numbers each time. But it should still be semantically, one single random number.
In this case, the number of rows changing given the same input dataset, is a bug.
The output of a query absolutely will depend on the plan. In ANSI SQL you're right it cannot (up to ordering), which is why ANSI SQL doesn't have anything like random() or nondeterministic user functions. But nearly all databases support something that is outside of that standard because it's genuinely useful, and then you get into poorly-defined situations that you need to deal with ad-hoc as an implementer.
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.
It is allowed to be a planner choice though! Very surprising, but if you understand the docs, it will follow that it is not actually a bug. It could change in the future and has changed in the past apparently - but that change was to give the planner more opportunity to optimize queries by not materializing parts of the query and inlining that part into the parent.
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.
I wrote this to a deleted comment, but even if the CTE was materialized, the subquery of the CTE would still not be...
For instance, with the stand alone query:
DELETE … WHERE id IN (
SELECT id … LIMIT 1 FOR UPDATE SKIP LOCKED
)
the planner is free to turn that IN ( subquery ) into a nested‐loop semi‐join, re-executing the subquery as many times as it deems optimal. Therefore it can delete more than 1 row.
More to the point: The SQL standard doesn't talk about semijoins at all, it conceptually evaluates the WHERE clause for each and every row. So the question is whether the subquery is guaranteed to give the same result each and every time. That would presumably depend on the transaction isolation level in use, except that SKIP LOCKED (which is nonstandard, from what I know) presumably calls off all bets anyway.
Hmm, it seems like the subquery is getting re-run on every single row of the task queue table, which seems like a performance issue in addition to correctness.
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.
"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."
We're likely seeing a simplified example. All queries should be able to be embedded in a CTE without integrity loss.
> it seems like the subquery is getting re-run on every single row of the task queue table
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.
Could be a generational thing. There was a time not every query planner would properly eliminate columns based on the final projection and any intermediary copies required more I/O if you included everything. Even now I still find myself following "best practices" for things that haven't been relevant in a decade.
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 don't understand where that item_id value comes from, since that's not a column that's mentioned anywhere else in the query.
I guess it must be an unmentioned column on that task_queue table?
In DELETE ... RETURNING, RETURNING works like SELECT in a read query, so, yes, item_id is a column in task_queue and the result set is the item_id value of the row deleted.
in general end to end tests/whole system tests which mock/stub as little as possible and are focused on testing whole use-case flows is a good starting point (sadly in many teams also a constant uphill battle from convincing PM over allocate time to bootstrap it to making CI harder due to basically having to bootstrap a whole testing system including db etc. to educating devs to write such tests properly, instead of writing unit tests at a end to end test level (which rarely ends well).
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.
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.
To add to sibling comment. This sounds like a parameter sniffing. Read it up. It should help you understand the problem. It is a known issue with SQL Server. Or rather a tradeoff. You don't want to compile a new execution plan for every query. This would eat up CPU by itself.
It’s one of RDBMS peculiarities that you just need to learn about. Any half decent MSSQL course or book will tell you about the plan cache and the occasional gotcha exactly like yours. Time wasted debugging this issue by the unaware is probably in kilo man years by now.
…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.)
My point was more that I feel it's a discrepancy that the DB tries to be very clever, but seemingly doesn't self-evaluate its decisions enough to determine when it wasn't.
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?
IN is fine. The biggest problem comes with NOT IN, which has NULL semantics that makes life difficult for the planner. It is consistent but rarely what the user wants. NOT EXISTS is typically better there.
DELETE in SKIP LOCKED scenario is recipe for failure. God/Postgres gave you transactions for atomicity... only UPDATE w/ SKIP LOCKED, holy brother in Christ, and only DELETE completed normally.
SWE's are learning about Type-2 tables the hard way round; of course, this is pure entertainment to data engineering people! Wait until "Audit table considered harmful" blog posts start popping up...
Since I don't often write raw SQL, I can only assume the author named their CTE `deleted_tasks` to elucidate that the query might delete multiple items. Otherwise, it makes little sense, for they intended to "pop" a single row, and yet their aptly named `deleted_tasks` ended up removing more than one!
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?
This really has nothing to do with CTEs.
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'.
I don’t know about “gotcha”. This sounds like a full blown planner bug to me.
I think arguably there is no bug here, but the blog doesn't do a good job of explaining the issue and their fix may work because they remove the CTE and switch to `=`, but they prove they don't understand why the difference when they suggest "using id IN (...) might still be necessary"; if you do that, then the problem will return.
There are two factors here.
The subquery
can return different ids each time you run it. If it was ordered, then it would always return the same id and if postgres optimized in a way that it runs more than once it would just get the same result each time anyway.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.
This is interesting - I don’t expect undefined behavior like this. Should the query delete more than one row or not?
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.
It's not undefined behavior, it's the fact that the uncorrelated subquery within the CTE doesn't specify an ordering, therefore it cannot be implicitly materialized / evaluated once. Postgres documentation is clear here [1]:
> 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
I see what you saying, but it is very subtle, wouldn’t you agree?
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.
Our curiosities are very aligned here. I wrote this post on a bus, and was rushed, so I agree it’s intentionally lacking some more critical details. The use of ”=“ solved the issue at the time (understandably why), but I haven’t had the time to do more thorough deep dive.
Hopefully soon!
Thank you for sharing your thoughts and comments. I went on a bit of a similar roller coaster :D
It doesn't matter. The output of a query cannot depend on the plan. All plans should generate semantically equivalent output necessarily. Notice I say semantically equivalent because obviously Select random() can return different numbers each time. But it should still be semantically, one single random number.
In this case, the number of rows changing given the same input dataset, is a bug.
The output of a query absolutely will depend on the plan. In ANSI SQL you're right it cannot (up to ordering), which is why ANSI SQL doesn't have anything like random() or nondeterministic user functions. But nearly all databases support something that is outside of that standard because it's genuinely useful, and then you get into poorly-defined situations that you need to deal with ad-hoc as an implementer.
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.
It is allowed to be a planner choice though! Very surprising, but if you understand the docs, it will follow that it is not actually a bug. It could change in the future and has changed in the past apparently - but that change was to give the planner more opportunity to optimize queries by not materializing parts of the query and inlining that part into the parent.
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
I wrote this to a deleted comment, but even if the CTE was materialized, the subquery of the CTE would still not be...
For instance, with the stand alone query:
the planner is free to turn that IN ( subquery ) into a nested‐loop semi‐join, re-executing the subquery as many times as it deems optimal. Therefore it can delete more than 1 row.More to the point: The SQL standard doesn't talk about semijoins at all, it conceptually evaluates the WHERE clause for each and every row. So the question is whether the subquery is guaranteed to give the same result each and every time. That would presumably depend on the transaction isolation level in use, except that SKIP LOCKED (which is nonstandard, from what I know) presumably calls off all bets anyway.
Hmm, it seems like the subquery is getting re-run on every single row of the task queue table, which seems like a performance issue in addition to correctness.
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.
"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."
We're likely seeing a simplified example. All queries should be able to be embedded in a CTE without integrity loss.
I think CTE is red herring here, the problem is in the sub-query regardless if it's in CTE or not
Yes, it is a red herring for sure; using a limit without a sort in a sub-query is always an issue.
> it seems like the subquery is getting re-run on every single row of the task queue table
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.
Why was the author not just doing returning *
No need for the CTE to select everything…
Could be a generational thing. There was a time not every query planner would properly eliminate columns based on the final projection and any intermediary copies required more I/O if you included everything. Even now I still find myself following "best practices" for things that haven't been relevant in a decade.
I guess it must be an unmentioned column on that task_queue table?
In DELETE ... RETURNING, RETURNING works like SELECT in a read query, so, yes, item_id is a column in task_queue and the result set is the item_id value of the row deleted.
Interesting to think about how to guard against these cases where query optimisation leads to unexpected results.
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.
in general end to end tests/whole system tests which mock/stub as little as possible and are focused on testing whole use-case flows is a good starting point (sadly in many teams also a constant uphill battle from convincing PM over allocate time to bootstrap it to making CI harder due to basically having to bootstrap a whole testing system including db etc. to educating devs to write such tests properly, instead of writing unit tests at a end to end test level (which rarely ends well).
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.
On a somewhat related note...
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.
To add to sibling comment. This sounds like a parameter sniffing. Read it up. It should help you understand the problem. It is a known issue with SQL Server. Or rather a tradeoff. You don't want to compile a new execution plan for every query. This would eat up CPU by itself.
It’s one of RDBMS peculiarities that you just need to learn about. Any half decent MSSQL course or book will tell you about the plan cache and the occasional gotcha exactly like yours. Time wasted debugging this issue by the unaware is probably in kilo man years by now.
…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.)
My point was more that I feel it's a discrepancy that the DB tries to be very clever, but seemingly doesn't self-evaluate its decisions enough to determine when it wasn't.
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?
IN is not recommended. Use = ANY instead.
aren't they the same in Postgres?
Yes, according to the manual, IN is equivalent to = ANY() https://www.postgresql.org/docs/current/functions-subquery.h...
I had to check because for some reason, I always thought =ANY was somehow better than IN.
IN is fine. The biggest problem comes with NOT IN, which has NULL semantics that makes life difficult for the planner. It is consistent but rarely what the user wants. NOT EXISTS is typically better there.
It is, but in a pretty minor way: any can be used with no items, IN can not and will error.
ANY can be used with arrays, particularly query parameters: `id = ANY($1::int[])`, but not `id IN $1::int[]`.
DELETE in SKIP LOCKED scenario is recipe for failure. God/Postgres gave you transactions for atomicity... only UPDATE w/ SKIP LOCKED, holy brother in Christ, and only DELETE completed normally.
God gave us Postgres to teach humanity that everything is an INSERT.
SWE's are learning about Type-2 tables the hard way round; of course, this is pure entertainment to data engineering people! Wait until "Audit table considered harmful" blog posts start popping up...