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.

Sign up for GitHub

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

Jump to bottom

ido_pgsql: do not set standard_conforming_strings to off #8123

Merged
merged 1 commit into from Oct 29, 2020
Merged

ido_pgsql: do not set standard_conforming_strings to off #8123

merged 1 commit into from Oct 29, 2020

Conversation

MEschenbacher
Copy link
Contributor

@MEschenbacher MEschenbacher commented Jul 19, 2020 •
edited by Al2Klimov

fixes #8122

Al2Klimov
Al2Klimov requested changes Jul 21, 2020
View reviewed changes
Copy link
Member

@Al2Klimov Al2Klimov left a comment

Choose a reason for hiding this comment

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

Hello @MEschenbacher and thank you for contributing!

Have you looked through the whole Postgres IDO code and all strings are E'...'?

Please test this change with Postgres 9.1 and 9.0.

@MEschenbacher
Copy link
Contributor Author

Will do.

@MEschenbacher
Copy link
Contributor Author

MEschenbacher commented Jul 26, 2020 •
edited

Progress so far:

  • looked through the whole postgres ido code: all strings which are executed with Query(...) within an E'...' literal are escaped using Escape(...)
  • Tested with postgres version 11.7
  • Tested with postgres version 9.0
  • Tested with postgres version 9.1
  • Provide a better commit message

@MEschenbacher
Copy link
Contributor Author

Testing with postgres 9.1 is finished.

@Al2Klimov Al2Klimov added this to the 2.13.0 milestone Jul 30, 2020
@Al2Klimov Al2Klimov added this to To review in v2.13.0 merge window via automation Jul 30, 2020
v2.13.0 merge window automation moved this from To review to Done Jul 30, 2020
@Al2Klimov Al2Klimov moved this from Done to To review in v2.13.0 merge window Jul 30, 2020
@Al2Klimov Al2Klimov added area/db-ido Database output bug Something isn't working labels Jul 30, 2020
v2.13.0 merge window automation moved this from To review to Changes requested Oct 29, 2020
Al2Klimov
Al2Klimov requested changes Oct 29, 2020
View reviewed changes
Copy link
Member

@Al2Klimov Al2Klimov left a comment

Choose a reason for hiding this comment

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

Please rebase.

@MEschenbacher
Copy link
Contributor Author

Done

@MEschenbacher
ido_pgsql: do not set standard_conforming_strings to off
d808956 
Before postgres 9.1, this setting defaulted to off and icinga2 code was
making heavy use of this feature. Since postgres 9.1, this settings
defaults to on. During the adoption of postgres >= 9.1, the icinga2
postgres ido code maintained compatibility by setting it to off
explicitly.

In the mean time, the postgres ido code has been converted to using the
`E'...'` escape literal syntax exclusively.

The last remaining step is now to no longer force the setting to off
because no query is using the feature any longer.

Closes github issue Icinga#8122.
julianbrost
julianbrost approved these changes Oct 29, 2020
View reviewed changes
Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

LGTM, couldn't find any non E'...' strings either.

@Al2Klimov Al2Klimov merged commit 1b9f161 into Icinga:master Oct 29, 2020
v2.13.0 merge window automation moved this from Changes requested to Merged Oct 29, 2020
This was referenced Aug 4, 2021
Closed
Merged
@quinox quinox mentioned this pull request Aug 10, 2021
Closed
@MEschenbacher MEschenbacher deleted the confirmingstrings branch October 21, 2023 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Al2Klimov Al2Klimov Al2Klimov requested changes

@julianbrost julianbrost julianbrost approved these changes

Assignees

@MEschenbacher MEschenbacher

Labels
area/db-ido Database output bug Something isn't working
Projects
No open projects
v2.13.0 merge window
  
Merged
Milestone
2.13.0
Development

Successfully merging this pull request may close these issues.

ido pgsql sets standard_conforming_strings to off without using it
3 participants
@MEschenbacher @julianbrost @Al2Klimov