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

Fix create for libpq 9.6 and older db #418

Conversation

omergertel
Copy link
Contributor

When Postgres library (libpq) 9.6 is installed on the server, pg_dump adds an SQL command to SET idle_in_transaction_session_timeout, which is new to 9.6, so it fails when running against older versions of the Postgres database.

I've added the parameter to the blacklist.

Is this acceptable?

@caironoleto
Copy link
Contributor

caironoleto commented Mar 28, 2017

These changes break in rails 5. Can you look?

@omergertel
Copy link
Contributor Author

There are other PRs with unrelated changes that fail on the same tests, so I don't think the problems are caused by my changes. If I have a chance to investigate further, I'll try and help.

@meganemura
Copy link
Collaborator

The reason which breaks Rails 5.0 tests is solved in #449.
rebase to the current development branch would make the build success.

When Postgres library (libpq) 9.6 is installed on the server, `pg_dump` adds an SQL command to SET `idle_in_transaction_session_timeout`, which is new to 9.6, so it fails when running against older versions of the Postgres database.

I've added the parameter to the blacklist.
@omergertel omergertel force-pushed the Fix-create-for-libpq-9.6-and-older-db branch from 3174b18 to 4bfc69a Compare July 19, 2017 07:40
@mikecmpbll
Copy link
Collaborator

i don't really know much about pg. @meganemura — does this seem okay to you? if so, i'll merge :).

@meganemura
Copy link
Collaborator

meganemura commented Jul 19, 2017

@mikecmpbll

I actually don't know much about pg either.

Those statements in black list are ignored from dumped schema on cloning pg schema.
I think idle_in_transaction_session_timeout is not important for cloning only schema.
So, it looks good to me.

@mikecmpbll
Copy link
Collaborator

@meganemura whups, my bad, thought you were a pg user :)

@omergertel why would you ever be dumping with 9.6 and loading the dump into an older version?

@omergertel
Copy link
Contributor Author

omergertel commented Jul 19, 2017

Not quite what happens.

This blocks the tenant creation if the client (libqp) is in version 9.6 and the database server is in version 9.5, because the client library adds the SET idle_in_transaction_session_timeout to the dump on its own.
The dump and restore are actually to the same database.

In our case, we've found out that Heroku have updated the client library to 9.6 in all of their images, and we were still running the database on 9.5. That's mostly ok because libpq is mostly backwards compatible. Except for this one setting.

Generally, it isn't farfetched to have a server running a client library different than the version of the database (as database upgrades are much more difficult and less frequent than, say, running apt-get on your machine).

This is the same reasoning behind the 9.3 workaround already in the code, I think.

@mikecmpbll
Copy link
Collaborator

the essence of the issue then is using incompatible versions of the client and server libraries :/. not sure why we'd do anything about that in apartment but seeing as, as you correctly pointed out, we already are, i'll merge..

@mikecmpbll mikecmpbll merged commit 163961b into influitive:development Jul 19, 2017
@omergertel
Copy link
Contributor Author

Thank you!

@omergertel omergertel deleted the Fix-create-for-libpq-9.6-and-older-db branch July 19, 2017 11:27
meganemura pushed a commit to meganemura/apartment that referenced this pull request Jul 20, 2017
When Postgres library (libpq) 9.6 is installed on the server, `pg_dump` adds an SQL command to SET `idle_in_transaction_session_timeout`, which is new to 9.6, so it fails when running against older versions of the Postgres database.

I've added the parameter to the blacklist.
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.

4 participants