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 sets standard_conforming_strings to off without using it #8122

Closed
MEschenbacher opened this issue Jul 19, 2020 · 5 comments · Fixed by #8123
Closed

ido pgsql sets standard_conforming_strings to off without using it #8122

MEschenbacher opened this issue Jul 19, 2020 · 5 comments · Fixed by #8123
Assignees
@MEschenbacher
Labels
area/db-ido Database output bug Something isn't working
Milestone
2.13.0

Comments

@MEschenbacher
Copy link
Contributor

Describe the bug

https://github.com/Icinga/icinga2/blob/master/lib/db_ido_pgsql/idopgsqlconnection.cpp#L260 sets standard_conforming_strings TO off although it is not leveraging the functionality this setting provides (https://www.postgresql.org/docs/current/runtime-config-compatible.html). All query strings for which escaping is explicitly enabled are written as E='" + Escape(value) + "'".

Since postgres 9.1, the setting defaults to true and we could omit the call.
@MEschenbacher MEschenbacher changed the title ido pgsql uses sets standard_conforming_strings to off without using it ido pgsql sets standard_conforming_strings to off without using it Jul 19, 2020
MEschenbacher added a commit to MEschenbacher/icinga2 that referenced this issue Jul 19, 2020
@MEschenbacher
ido_pgsql: do not set standard_conforming_strings to off
8a404a8 
closes issue Icinga#8122
@MEschenbacher MEschenbacher mentioned this issue Jul 19, 2020
Merged
@Al2Klimov
Copy link
Member

Hello @MEschenbacher and thank you for reporting!

Is there a problem for you as Icinga user due to that setting?

Best,
AK

@Al2Klimov Al2Klimov added the needs feedback We'll only proceed once we hear from you again label Jul 20, 2020
@MEschenbacher
Copy link
Contributor Author

I was not sure about labelling the issue as either bug or feature. Maybe that is what you are hinting. On the one hand it has no impact at all if the code stays or leaves since the behavour, which the standard_conforming_strings provides, is not used by the code. On the other hand, I consider this not dead but unused/superfluous code.

I noticed the issue originally when trying to use cockroachdb as database backend for icinga2 but figured I can argue this without basing the discussion on a specific technology by just looking at the code.

@MEschenbacher
Copy link
Contributor Author

https://www.cockroachlabs.com/docs/stable/set-vars.html#supported-variables

@Al2Klimov
Copy link
Member

I noticed the issue originally when trying to use cockroachdb as database backend for icinga2

How did you notice it? Did cockroachdb not work/complain?

@MEschenbacher
Copy link
Contributor Author

Sorry for acting so vague: Yes, cockroachdb did complain with a hard error. The setting standard_conforming_strings is readonly and defaults to on (see the link in my last post).

@Al2Klimov Al2Klimov removed the needs feedback We'll only proceed once we hear from you again label Jul 21, 2020
MEschenbacher added a commit to MEschenbacher/icinga2 that referenced this issue Jul 26, 2020
@MEschenbacher
ido_pgsql: do not set standard_conforming_strings to off
35b6547 
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.
@Al2Klimov Al2Klimov added area/db-ido Database output bug Something isn't working labels Jul 30, 2020
MEschenbacher added a commit to MEschenbacher/icinga2 that referenced this issue Oct 29, 2020
@MEschenbacher
ido_pgsql: do not set standard_conforming_strings to off
bc1c613 
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.
MEschenbacher added a commit to MEschenbacher/icinga2 that referenced this issue Oct 29, 2020
@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.
@icinga-probot icinga-probot bot added this to the 2.13.0 milestone Oct 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@MEschenbacher MEschenbacher

Labels
area/db-ido Database output bug Something isn't working
Projects
None yet
Milestone
2.13.0
Development

Successfully merging a pull request may close this issue.

ido_pgsql: do not set standard_conforming_strings to off MEschenbacher/icinga2
2 participants
@MEschenbacher @Al2Klimov