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

update to db unprepared statements #66

Merged
merged 1 commit into from
Dec 13, 2016
Merged

Conversation

bcardiff
Copy link
Collaborator

From crystal-lang/crystal-db#25 (currently in master but not released) a connection will create prepared and non prepared statements. This is required for uniform transactions api.

It seems crystal-pg has only implemented non prepared statements so the quick solution is to reuse them when ever a prepared statement is expected.

Prepared statements have special treatment since they need the be released as opposed to non prepared ones.

I would vouch for a proper prepared statement in postgres, but I am not sure you will agree.

crystal-pg does not support prepared statements (?)
Copy link
Owner

@will will left a comment

Choose a reason for hiding this comment

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

thanks

@will will merged commit 6d19fe3 into will:master Dec 13, 2016
@samueleaton
Copy link

@bcardiff @will This is breaking stuff.

Error in libs/db/db/connection.cr:36: abstract `def DB::Connection#build_statement(query)` must be implemented by PG::Connection

    abstract def build_statement(query) : Statement
                 ^~~~~~~~~~~~~~~

@bcardiff
Copy link
Collaborator Author

@samueleaton yes but only if you depend on crystal-pg master branch for your app/lib.

The alternative is that in master of this shard we keep crystal-db master as dependency and upon publishing a.new version of -pg that is fixed to some versioning number. Or a full git flow scheme is followed.

In the following days a new release of db will come and master will be stable again.

@samueleaton
Copy link

Should it be in a different working branch until it works? Seems premature to put it in master.

But yeah I removed my reference to master. 👍

@will
Copy link
Owner

will commented Dec 14, 2016

Seems premature to put it in master.

I misunderstood and thought not having this merged was blocking finishing the other patch

@bcardiff bcardiff mentioned this pull request Dec 14, 2016
@bcardiff
Copy link
Collaborator Author

Apologies I should been more asserting when I said

(currently in master but not released)

At the end of the day it depends if we see master as stable or unstable.
Also, the story of developing multiple shards at the same time is not nice enough still.

I've just sent another PR that will fix master since crystal-db 0.3.0 is released now.

@bcardiff bcardiff deleted the feature/unprepared branch December 15, 2016 18:04
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.

None yet

3 participants