Skip to content
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

Add support for locking clauses in SQL #7979

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Add support for locking clauses in SQL #7979

wants to merge 8 commits into from

Conversation

elprans
Copy link
Member

@elprans elprans commented Nov 8, 2024

This adds parsing/codegen bits. The resolver needs to learn to push the locking_clause down to constituent elements of the inheritance union.

@aljazerzen
Copy link
Contributor

Wait, this is not a sound approach.

If we compile this:

SELECT name FROM "User" WHERE id = $1::uuid FOR UPDATE

... to this ...

WITH
user_inh_view AS (
  SELECT ... FOR UPDATE
)
SELECT name FROM user_inh_view WHERE id = $1::uuid

... PostgreSQL will lock all rows of the base table, not just the one selected by the where clause.

@msullivan
Copy link
Member

Hm. Should we just document then that locking clauses only work sensibly for non-inherited types without access policies?
Which is probably fine anyway, for queue-like flows?

@aljazerzen
Copy link
Contributor

That would work.

But we'd have to modify code for compiling inheritance CTEs, because currently we do it unconditionally - even if object has no access policies, we call into the pg_compiler and generate inheritance views, which then turn out to be very verbose no-ops. Also when access policies are disabled, we generate inheritance CTEs unconditionally, even if it is just a select from single table.

@aljazerzen
Copy link
Contributor

The decision here (that me and Elvis made yesterday) is to first support FOR UPDATE/INSERT/... only object types / link tables that don't have sub-types and don't have access policies.

I'll start work on this.

@aljazerzen
Copy link
Contributor

Ok, I've implemented our restrictions. This can be reviewed.

This PR should also improve resolver & query performance, because it skips creating inheritance CTEs or access policy CTEs when they are not needed.

I still have to add tests that check if locks are actually obtained as they should be.

@aljazerzen aljazerzen marked this pull request as ready for review November 22, 2024 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants