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

Correction of multiline UPDATE statements in the Postgres driver #2629

Closed
wants to merge 7 commits into from
Closed

Correction of multiline UPDATE statements in the Postgres driver #2629

wants to merge 7 commits into from

Conversation

therufa
Copy link

@therufa therufa commented Sep 9, 2013

Added support for returning values in multi-line UPDATE query statements to the PostgreSQL database driver

@therufa
Copy link
Author

therufa commented Sep 9, 2013

The feature is provided by the PostgreSQL engine, and it would be great to be able to use this feature without any hack in CI.

Source:
http://www.postgresql.org/docs/9.2/static/sql-update.html#AEN79130

@narfbg
Copy link
Contributor

narfbg commented Sep 9, 2013

Nice one, although it needs some additional work:

  • Please check our styleguide, use tabs for indentation and remove the empty line at EOF.
  • Add a changelog entry.
  • I haven't checked, but if INSERT and UPDATE support the RETURNING clause, doesn't REPLACE do as well?
  • Furthermore, is it SET that you're also adding to or UPDATE ... SET ?

@therufa
Copy link
Author

therufa commented Sep 9, 2013

I do hope, that I could accomplish everything properly.

By the way; there is no REPLACE statement in the PostgreSQL.
If I do understand your question right, yes I wanted the SET at that location :)

@therufa
Copy link
Author

therufa commented Sep 10, 2013

i hope everything is right now

@narfbg
Copy link
Contributor

narfbg commented Sep 10, 2013

So, you're telling me that you can do something like this?

SET time zone 'Europe/Sofia' RETURNING '2';

This seems quite odd to me. :)
Otherwise ... the PR is not complete, but I'll do the afterwork so that it doesn't take days in explanations between the two of us. You'll see what I mean afterwards.

@therufa
Copy link
Author

therufa commented Sep 10, 2013

Well, the set part might make no sense with your example, but if you work with multi-line queries it can be quite annoying that the SET keyword turns the query to a non-reading type.

The example of @fema90 shows exactly the problem. Look at the second line; that SET there would break the whole query, and the result would be that you have no result, but that the query went well. Thats not what I want in such a case.

I hope this explains my step. :)

@therufa
Copy link
Author

therufa commented Sep 10, 2013

So I think the current solution fits everything, except what you've mentioned (to be honest I don't really understand what you've meant, with the PR, I hope that is not too much).

narfbg added a commit that referenced this pull request Sep 10, 2013
An improved version of PR #2629.
Also removes REPLACE from the regular expression, as it is not supported by PostgreSQL.
@narfbg
Copy link
Contributor

narfbg commented Sep 10, 2013

@fema90's example shows an UPDATE query, the regular expression that I asked you about would validate a query that bebings with a SET. The one now, after your last commit is even messier. :)

See the above commit for the cleaner way. Sorry for making you add a changelog entry btw, I just realized it was the same bug that was fixed for INSERT, so no need to have 2 lines in there.

@narfbg narfbg closed this Sep 10, 2013
@therufa
Copy link
Author

therufa commented Sep 11, 2013

Well, this does not solve the base problem (at least not "mine"), but maybe I just don't understand something, and maybe its off topic:
Should I pass only single-line queries to the db->query() method?

In the Postgres community (as it is in the Oracle and in the MsSQL) its common practice to write long queries, and for readability these are split into multiple lines.

So wouldn't it make sense to support this practice, or is it considered as wrong? I'm just asking right now.

narfbg added a commit that referenced this pull request Sep 11, 2013
@narfbg
Copy link
Contributor

narfbg commented Sep 11, 2013

The good old PCRE ignoring everything after the newline character, even without the 'm' modifier ... should be OK now. :)

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

2 participants