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

Postgres Support #20

Merged
merged 1 commit into from
Dec 4, 2015
Merged

Postgres Support #20

merged 1 commit into from
Dec 4, 2015

Conversation

derencius
Copy link
Contributor

This pull request will allow the basic features of polo to work with postgres.

@derencius
Copy link
Contributor Author

But if the on_duplicate feature is used, the generated SQL is going to be invalid because postgres doesn't support INSERT IGNORE and ON DUPLICATE KEY UPDATE features out-of-box yet. What would be an appropriate solution for this? An alert or stop the process?

@nettofarah
Copy link
Contributor

Hey, @derencius.
Thank you for submitting this PR.
You're correct, there's no easy implementation of the on_duplicate options for PostgreSQL.

I agree that it would make sense to raise an exception when someone tries to use on_duplicate in their Polo.configure block.

Btw, can you rebase your branch on top of master as opposed to merging it? That way it would keep Polo's history cleaner.

After you do that, can you please squash all the commits and use this standard for the commit message?
http://chris.beams.io/posts/git-commit/

This is looking awesome! Can't wait to 🚢 this!

@bbonamin
Copy link

bbonamin commented Dec 1, 2015

@derencius @nettofarah AFAIK postgres will start supporting something similar to INSERT IGNORE or ON DUPLICATE UPDATE starting on 9.5 https://wiki.postgresql.org/wiki/UPSERT

@bbonamin
Copy link

bbonamin commented Dec 1, 2015

By the way, does this feature work with associations? When I tried this branch, it didn't work for me. It only generated the "base" insert statement @derencius

@nettofarah
Copy link
Contributor

I sounds like we're gonna have to tackle #23 before we fiddle with this PR some more.

What do y'all think?

@derencius
Copy link
Contributor Author

@bbonamin how did you try it? did you specify the branch in your Gemfile?

gem 'polo', github: 'derencius/polo', branch: 'postgres'

Because postgres can use prepared statments, Polo was failing to capture generated SQL.
Disabling prepared statements while capturing the queries fixed this issue.

Rails earlier than version 4.0.1 doesn't have support to prepared statements,
so the call to unprepared_statement should be prevented.

fixes support for rails < 4.0.1, that doesn't have unprepared_statments
@derencius
Copy link
Contributor Author

@nettofarah not sure about the way to prevent the usage of on_duplicate_strategy. any suggestion?

@nettofarah
Copy link
Contributor

@derencius I think we could leave this as is for now, assuming everything works.
I've been thinking about creating multiple translators, one for each database adapter.

We could implement a postgres one that just raises an error when trying to use one of these features in run time.

Another option would be to check when calling Polo.configure, though I think that could become a little cumbersome.

In any case, I think your PR looks great, and we should be able to merge it at any time. We can open a new issue for the on_duplicate strategy improvements.

@bbonamin
Copy link

bbonamin commented Dec 4, 2015

@derencius I just tried again and it follows associations as you would expect! My bad 😄

@nettofarah
Copy link
Contributor

Alright alright!
I think we're good to go then!

congrats @derencius on your awesome PR.
And thank you everyone for helping out.

nettofarah added a commit that referenced this pull request Dec 4, 2015
@nettofarah nettofarah merged commit b69b915 into IFTTT:master Dec 4, 2015
@derencius
Copy link
Contributor Author

💯 🎉

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.

3 participants