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

PostgreSQL: prepare for pg-1.1 #33188

Merged
merged 2 commits into from Sep 16, 2018

Conversation

Projects
None yet
7 participants
@larskanis
Contributor

larskanis commented Jun 22, 2018

pg-1.1 is planned to deprecate some behavior. I (tried to) discussed this here. It currently warns at every method invocation, but I plan to change it to warm only once and to add an environment variable to disable the warning entirely. I'm also open to disable the most prominent warning about async_exec vs. async_exec_params until the next rails-5.x releases are out.

Fixes #33185

PostgreSQL: Prepare for pg-1.1.0
Version 1.1.0 deprecates exec and async_exec with a params array due to
distinct semantics between calls with and without params array.
Instead exec_params or async_exec_params shall be used.

Moreover in pg-1.1.0 exec_* and prepare methods are aliases for async_exec_*
and async_prepare. async_* methods don't need to be called explicit -
they are the default now, when calling exec_* and prepare.

For pg versions before 1.1, keep using async_exec.
@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot Jun 22, 2018

r? @schneems

(@rails-bot has picked a reviewer for you, use r? to override)

rails-bot commented Jun 22, 2018

r? @schneems

(@rails-bot has picked a reviewer for you, use r? to override)

Show outdated Hide outdated activerecord/lib/active_record/connection_adapters/postgresql/oid/array.rb Outdated
@@ -600,7 +608,7 @@ def exec_no_cache(sql, name, binds)
type_casted_binds = type_casted_binds(binds)
log(sql, name, binds, type_casted_binds) do
ActiveSupport::Dependencies.interlock.permit_concurrent_loads do
@connection.async_exec(sql, type_casted_binds)
@connection.exec_params(sql, type_casted_binds)

This comment has been minimized.

@SamSaffron

SamSaffron Jun 22, 2018

Contributor

Would exec_params be safe as a 5.2 backport? (would we need to backport to 5.1/5.0/4.2 as well?)

@SamSaffron

SamSaffron Jun 22, 2018

Contributor

Would exec_params be safe as a 5.2 backport? (would we need to backport to 5.1/5.0/4.2 as well?)

This comment has been minimized.

@SamSaffron

SamSaffron Jun 22, 2018

Contributor

also I find the semantics a bit confusing cause I would expect exec_params to work like exec and async_exec_params to work like async_exec ... but it appears that exec_params is always async.

@SamSaffron

SamSaffron Jun 22, 2018

Contributor

also I find the semantics a bit confusing cause I would expect exec_params to work like exec and async_exec_params to work like async_exec ... but it appears that exec_params is always async.

This comment has been minimized.

@larskanis

larskanis Jun 22, 2018

Contributor

Would exec_params be safe as a 5.2 backport?

Yes, exec_params has been there since pg-0.15.0. And starting with pg-1.1.0 it will be a alias for async_exec_params.

In pg < 1.1 exec_params did what sync_exec_params will do in pg-1.1. sync_exec_params differed from async_exec_params, in that it doesn't stop at signals. It delayes signal processing until the SQL statement has been finished.

Using exec_params instead of async_exec is therefore kind of regression regarding signals. But there are still other pg methods in use which delay signal processing equally and it is only valid when running with pg < 1.1, so that I don't think it's actually a problem. I asked to get the other methods fixed in PR #32820, but then we came to the conclusion, that it's better to alias all query methods to the corresponding async_foo method in pg-1.1 instead.

@larskanis

larskanis Jun 22, 2018

Contributor

Would exec_params be safe as a 5.2 backport?

Yes, exec_params has been there since pg-0.15.0. And starting with pg-1.1.0 it will be a alias for async_exec_params.

In pg < 1.1 exec_params did what sync_exec_params will do in pg-1.1. sync_exec_params differed from async_exec_params, in that it doesn't stop at signals. It delayes signal processing until the SQL statement has been finished.

Using exec_params instead of async_exec is therefore kind of regression regarding signals. But there are still other pg methods in use which delay signal processing equally and it is only valid when running with pg < 1.1, so that I don't think it's actually a problem. I asked to get the other methods fixed in PR #32820, but then we came to the conclusion, that it's better to alias all query methods to the corresponding async_foo method in pg-1.1 instead.

@larskanis

This comment has been minimized.

Show comment
Hide comment
@larskanis

larskanis Jun 22, 2018

Contributor

This should be backported to all rails-5.x. rails-4.x is not (yet) compatible to pg-1.x so backporting makes no sense there.

Contributor

larskanis commented Jun 22, 2018

This should be backported to all rails-5.x. rails-4.x is not (yet) compatible to pg-1.x so backporting makes no sense there.

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Jun 22, 2018

Member

I (tried to) discussed this here

Ah sorry, that's not me.

Member

matthewd commented Jun 22, 2018

I (tried to) discussed this here

Ah sorry, that's not me.

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Jun 22, 2018

Member

Yeah I think my suggestion (and I'm aware this is coming from a maintainer of a very deprecation-happy project) would be to keep the old names working at least for one more release series, as they don't seem to be "in the way" / occupying names you want to reuse with different semantics.

I think we're currently semi-unofficially still mostly maintaining 5.0, so a full 5.x backport isn't impossible... but it does seem a bit pointless: much as 4.2 doesn't support 1.x, 5.x doesn't support pg 2.x -- so it's a warning about a change that code will never encounter. 😕

Member

matthewd commented Jun 22, 2018

Yeah I think my suggestion (and I'm aware this is coming from a maintainer of a very deprecation-happy project) would be to keep the old names working at least for one more release series, as they don't seem to be "in the way" / occupying names you want to reuse with different semantics.

I think we're currently semi-unofficially still mostly maintaining 5.0, so a full 5.x backport isn't impossible... but it does seem a bit pointless: much as 4.2 doesn't support 1.x, 5.x doesn't support pg 2.x -- so it's a warning about a change that code will never encounter. 😕

Return empty array when casting malformed array strings
Parsing of malformed array strings without raising an error is deprecated in
pg-1.1. It's therefore necessary to catch parser errors starting with pg-2.0.

See also pg commit:
  https://bitbucket.org/ged/ruby-pg/commits/1b081326b346368e70c9c03ee7080e28d6b3a3dc
@larskanis

This comment has been minimized.

Show comment
Hide comment
@larskanis

larskanis Jul 7, 2018

Contributor

I adjusted the patch based on the comments, so that it's IMHO ready to be merged.

Contributor

larskanis commented Jul 7, 2018

I adjusted the patch based on the comments, so that it's IMHO ready to be merged.

@SamSaffron

This comment has been minimized.

Show comment
Hide comment
@SamSaffron

SamSaffron Jul 8, 2018

Contributor

Lars, thinking about this, perhaps we simply strip the warning for now from 1.1 (and re-introduce with 2.0) and then "eat" the behavior change with malformed arrays on upgrade of PG

The trouble of getting this into Rails and backported is that we will be that we are 100% blocked on releasing PG 1.1 till a new gem is cut for 5.X, this usually only happens when there is a security flaw found. It could be months.

I think for Rails 6.0 we should change it so malformed arrays are malformed arrays and raise properly, nuking that weird test.

@matthewd @sgrif / @tenderlove thoughts?

Contributor

SamSaffron commented Jul 8, 2018

Lars, thinking about this, perhaps we simply strip the warning for now from 1.1 (and re-introduce with 2.0) and then "eat" the behavior change with malformed arrays on upgrade of PG

The trouble of getting this into Rails and backported is that we will be that we are 100% blocked on releasing PG 1.1 till a new gem is cut for 5.X, this usually only happens when there is a security flaw found. It could be months.

I think for Rails 6.0 we should change it so malformed arrays are malformed arrays and raise properly, nuking that weird test.

@matthewd @sgrif / @tenderlove thoughts?

@y-yagi

This comment has been minimized.

Show comment
Hide comment
@y-yagi

y-yagi Aug 25, 2018

Member

It seems that pg 1.1.0 has been released. And when use it, warning about async_exec is showed.

warning: forwarding async_exec to async_exec_params and send_query to send_query_params is deprecated 

What is the status of this PR now? Thanks.

Member

y-yagi commented Aug 25, 2018

It seems that pg 1.1.0 has been released. And when use it, warning about async_exec is showed.

warning: forwarding async_exec to async_exec_params and send_query to send_query_params is deprecated 

What is the status of this PR now? Thanks.

ruby-bench-server pushed a commit to tgxworld/rails that referenced this pull request Aug 25, 2018

Avoid `pg` 1.1.0 for now
Because there are tests that fail due to the influence of the
`async_exec` deprecate message.
https://travis-ci.org/rails/rails/jobs/420345370

Related to rails#33188.
@larskanis

This comment has been minimized.

Show comment
Hide comment
@larskanis

larskanis Aug 25, 2018

Contributor

This PR is still valid. The warnings only show up if ruby warnings are on (ruby -w) and they can be disabled explicit by setting the environment variable PG_SKIP_DEPRECATION_WARNING=1.

Contributor

larskanis commented Aug 25, 2018

This PR is still valid. The warnings only show up if ruby warnings are on (ruby -w) and they can be disabled explicit by setting the environment variable PG_SKIP_DEPRECATION_WARNING=1.

value = @pg_decoder.decode(value)
value = begin
@pg_decoder.decode(value)
rescue TypeError

This comment has been minimized.

@larskanis

larskanis Aug 25, 2018

Contributor

I made it a configuration option whether a TypeError is raised or whether a partial array is returned. The latter one is the default, so that the second commit of this PR is not strictly necessary for pg-1.1.0.

@larskanis

larskanis Aug 25, 2018

Contributor

I made it a configuration option whether a TypeError is raised or whether a partial array is returned. The latter one is the default, so that the second commit of this PR is not strictly necessary for pg-1.1.0.

@y-yagi

This comment has been minimized.

Show comment
Hide comment
@y-yagi

y-yagi Aug 26, 2018

Member

Thank you for the explanation! I understood.

Member

y-yagi commented Aug 26, 2018

Thank you for the explanation! I understood.

@y-yagi

y-yagi approved these changes Aug 26, 2018

@plentz

plentz approved these changes Aug 31, 2018

@y-yagi y-yagi merged commit c021157 into rails:master Sep 16, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
codeclimate All good!
Details
@y-yagi

This comment has been minimized.

Show comment
Hide comment
@y-yagi

y-yagi Sep 16, 2018

Member

@larskanis Thanks!

Member

y-yagi commented Sep 16, 2018

@larskanis Thanks!

y-yagi added a commit that referenced this pull request Sep 17, 2018

Merge pull request #33188 from larskanis/pg-1.1
PostgreSQL: prepare for pg-1.1

y-yagi added a commit that referenced this pull request Sep 17, 2018

Merge pull request #33188 from larskanis/pg-1.1
PostgreSQL: prepare for pg-1.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment