Skip to content

Conversation

@DrewKimball
Copy link
Collaborator

PL/pgSQL routines use barrier expressions to prevent optimizations that would remove side effecting expressions from the query plan. Previously, we would add a barrier below the outer join used to ensure at least one row returned by a SQL statement with an INTO clause. However, this wasn't enough, since after some inlining and projection push-down into the join, the join could be eliminated. This could result in a side-effecting SQL statement being dropped if its target variable(s) weren't referenced.

This commit fixes the bug by moving the barrier above the outer-join. This prevents optimizations (like join elimination) that rely on proving that the columns from the null-extended side of the join are not needed.

Fixes #147269

Release note (bug fix): Fixed a bug existing since SQL statements with INTO clause were introduced for PL/pgSQL routines in v23.2. The bug could cause a SQL statement with side effects (e.g. INSERT) to be dropped if none of the target variables from the INTO clause were referenced.

PL/pgSQL routines use barrier expressions to prevent optimizations
that would remove side effecting expressions from the query plan.
Previously, we would add a barrier below the outer join used to ensure
at least one row returned by a SQL statement with an INTO clause.
However, this wasn't enough, since after some inlining and projection
push-down into the join, the join could be eliminated. This could
result in a side-effecting SQL statement being dropped if its target
variable(s) weren't referenced.

This commit fixes the bug by moving the barrier above the outer-join.
This prevents optimizations (like join elimination) that rely on
proving that the columns from the null-extended side of the join are
not needed.

Fixes cockroachdb#147269

Release note (bug fix): Fixed a bug existing since SQL statements with
INTO clause were introduced for PL/pgSQL routines in v23.2. The bug could
cause a SQL statement with side effects (e.g. INSERT) to be dropped if
none of the target variables from the INTO clause were referenced.
@DrewKimball DrewKimball requested a review from a team November 5, 2025 20:36
@DrewKimball DrewKimball requested a review from a team as a code owner November 5, 2025 20:36
@DrewKimball DrewKimball requested review from mw5h and removed request for a team November 5, 2025 20:36
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find! :lgtm: Worth backporting perhaps all the way to 24.1?

@yuzefovich reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mw5h)

Copy link
Collaborator

@mgartner mgartner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Great find!

@mgartner reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @mw5h)

Copy link
Collaborator

@michae2 michae2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Piling on to say nice job!

@michae2 reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: :shipit: complete! 3 of 0 LGTMs obtained (waiting on @mw5h)

Copy link
Contributor

@mw5h mw5h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mw5h reviewed all commit messages.
Reviewable status: :shipit: complete! 3 of 0 LGTMs obtained (waiting on @DrewKimball)

@DrewKimball
Copy link
Collaborator Author

TFTRs!

bors r+

craig bot pushed a commit that referenced this pull request Nov 25, 2025
156966: plpgsql: move barrier after outer-join for SQL stmt with INTO clause r=DrewKimball a=DrewKimball

PL/pgSQL routines use barrier expressions to prevent optimizations that would remove side effecting expressions from the query plan. Previously, we would add a barrier below the outer join used to ensure at least one row returned by a SQL statement with an INTO clause. However, this wasn't enough, since after some inlining and projection push-down into the join, the join could be eliminated. This could result in a side-effecting SQL statement being dropped if its target variable(s) weren't referenced.

This commit fixes the bug by moving the barrier above the outer-join. This prevents optimizations (like join elimination) that rely on proving that the columns from the null-extended side of the join are not needed.

Fixes #147269

Release note (bug fix): Fixed a bug existing since SQL statements with INTO clause were introduced for PL/pgSQL routines in v23.2. The bug could cause a SQL statement with side effects (e.g. INSERT) to be dropped if none of the target variables from the INTO clause were referenced.

Co-authored-by: Drew Kimball <drewk@cockroachlabs.com>
@yuzefovich yuzefovich added the backport-all Flags PRs that need to be backported to all supported release branches label Nov 25, 2025
@craig
Copy link
Contributor

craig bot commented Nov 25, 2025

Build failed:

@DrewKimball
Copy link
Collaborator Author

Looks like a flake.

bors retry

@craig
Copy link
Contributor

craig bot commented Nov 25, 2025

@blathers-crl
Copy link

blathers-crl bot commented Nov 25, 2025

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


💡 Consider backporting to the fork repo instead of the main repo. See instructions for more details.

error creating merge commit from b706af5 to blathers/backport-release-24.3-156966: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch release-24.3 failed. See errors above.


💡 Consider backporting to the fork repo instead of the main repo. See instructions for more details.

💡 Consider backporting to the fork repo instead of the main repo. See instructions for more details.

💡 Consider backporting to the fork repo instead of the main repo. See instructions for more details.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@DrewKimball DrewKimball deleted the move-barrier branch November 25, 2025 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-all Flags PRs that need to be backported to all supported release branches backport-failed target-release-26.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"insert on conflict" with unused "returning into" variable in plpgsql causes row not to be inserted

6 participants