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

Use numbered indices in list arguments for async postgresql queries #3579

Conversation

sachera
Copy link
Contributor

@sachera sachera commented Oct 7, 2022

Queries with "IN" use the createArguments function at runtime to generate an argument list consisting of the correct number of questionmarks.

In #3559 the BindParameterMixin was introduced to replace questionsmarks with numbered indices for queries generated with the postgresql dialect when the R2dbcDriver is used. This change did not include the dynamic generation of argument lists at runtime.

This PR introduces a new helper function createNumberedArguments and changes the code generation to switch between this and the already existing createArguments function depending on the dialect and if async queries are to be used.

Also, if numbered indices are used, each query in a transactions uses a seperate ${name}Indexes variable due to probably different offsets. It may be possible to optimize them away in some cases if the offsets are stored and only one variable per list argument and offset is created. I did not attempt to write this optimization since I expect the code to become significantly more complicated for an insignificant performance gain.

|selectForValueAndMultipleIds:
|SELECT *
|FROM data
|WHERE value = ? AND id IN ?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add another ? after the array parameter just to make sure the index still works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. It actually doesn't work as intended. It changes the ? behind the list array $2 but tries to bind at index id.size+1. I think the best way to fix this is by moving the non array arguments to the front and change the way the offset calculation and the code for the binding works accordingly.

@hfhbd
Copy link
Collaborator

hfhbd commented Oct 7, 2022

Thanks for adding support for arrays, but I think Alec is right, we should move it to the driver. ##3569 is similar. And I dislike the hardcoded $1 in the runtime api. The runtime should not care about driver-specific placeholders.

@sachera
Copy link
Contributor Author

sachera commented Oct 7, 2022

Thanks for adding support for arrays, but I think Alec is right, we should move it to the driver. ##3569 is similar. And I dislike the hardcoded $1 in the runtime api. The runtime should not care about driver-specific placeholders.

In this case I will convert this PR to a draft an await the further discussion. I would be happy to help implementing them though.

@sachera sachera marked this pull request as draft October 7, 2022 17:34
@AlecKazakova
Copy link
Collaborator

closing this one in favour of the other PR for discussion/improvements

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.

None yet

3 participants