Skip to content

Fix unprepared_statement to work it when nesting #41423

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

Merged
merged 1 commit into from
Feb 12, 2021

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Feb 12, 2021

I've been reported a problem by @osyo-manga, preloading queries in
unprepared_statement doesn't work as expected in main branch (queries
are executed as prepared statements).

It is caused by #41385, due to scope.to_sql in grouping_key depends
on unprepared_statement which has an issue when nesting.

To fix the issue, don't add/delete object_id in the prepared statements
disabled cache if that is already disabled.

I've been reported a problem by @osyo-manga, preloading queries in
`unprepared_statement` doesn't work as expected in main branch (queries
are executed as prepared statements).

It is caused by rails#41385, due to `scope.to_sql` in `grouping_key` depends
on `unprepared_statement` which has an issue when nesting.

To fix the issue, don't add/delete object_id in the prepared statements
disabled cache if that is already disabled.
@kamipo kamipo merged commit 1efa175 into rails:main Feb 12, 2021
@kamipo kamipo deleted the fix_nested_unprepared_statements branch February 12, 2021 16:25
kamipo added a commit that referenced this pull request Feb 18, 2021
Fix `unprepared_statement` to work it when nesting
@dla-c-box
Copy link

To save some investigation to other researchers, this patch can also fix new errors such as ActiveRecord::StatementInvalid PG::ProtocolViolation: ERROR: bind message supplies 0 parameters, but prepared statement "" requires 2 when migrating to a lower Rails version that doesn't contain this PR's patch, e.g. if using the method .with from the gem postgres_ext (CTE aren't available natively until Rails 7.1): it uses .to_sql to build its arel, so if you call .to_sql on a relation that uses .with, you end up with nested unprepared_statement and hit the problem this PR fixes (if you were to use the output of your .to_sql call as a component of another query, you'd end up with bind parameters where none are expected, hence the StatementInvalid).

Note: .to_sql is generated as unprepared_statement since Rails 5.1: 3befc7a

Before this fix (notice the "$1" in the generated string):

puts User.with(test: User.limit(1)).limit(1).to_sql
--> WITH "test" AS (SELECT "public"."users".* FROM "public"."users" LIMIT 1) SELECT "public"."users".* FROM "public"."users" LIMIT $1

After this fix:

puts User.with(test: User.limit(1)).limit(1).to_sql
--> WITH "test" AS (SELECT "public"."users".* FROM "public"."users" LIMIT 1) SELECT "public"."users".* FROM "public"."users" LIMIT 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants