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

SELECT incorrectly returning no data #24

Closed
bridger opened this issue Jul 9, 2017 · 19 comments
Closed

SELECT incorrectly returning no data #24

bridger opened this issue Jul 9, 2017 · 19 comments

Comments

@bridger
Copy link

bridger commented Jul 9, 2017

I noticed that every once in a while my authentication code would find no data, even though it should have. It was reproing sporadically.

This is the query which is was running:
SELECT Devices.id, Devices.name, Devices.created, Devices.type, Users.id, Users.name, Users.created FROM Devices JOIN Users ON Devices.authedUser = Users.id WHERE (Devices.secretHash = '7aa5e492231b9c7fc32e13f1') AND (Devices.salt = 'ZR6E');

The result was sometimes incorrectly successNoData even though the row in the database existed for a while and running the exact same query immediately afterwards in psql would return the correct data.

I am now using the branch issue_23 and it isn't reproing for me now that I've switched. I was hopeful that branch had a race condition fix in it and it is promising so far.

@t3ndai
Copy link

t3ndai commented Aug 19, 2017

Seems I'm running into the same issue too, I was using SwiftKuery Postgresql in a Server app, but now the select queries return no results

@dsperling
Copy link
Contributor

dsperling commented Feb 1, 2018

I am seeing the same issue with 1.0.3. I am having trouble trying to test the issue_23 branch as it was made pre Swift 4.

From what I can tell at the moment, the issue appears to be related to the ConnectionPool. If I make my poolOptions ConnectionPoolOptions(initialCapacity: 5), I get a successNoData returned on the 6th select statement. Increasing the initialCapacity to 10, I see the 11th SELECT statement return successNoData. After that, SQL statements work sporadically.

@dsperling
Copy link
Contributor

@bridger thanks for point out the issue_23 branch. I have confirmed that I see the problem fixed as well. I created a fork and merged issue_23 changes into an issue_24 branch. If anyone wants to test, you can use the following line in your Swift 4 Packages.swift file:

.package(url: "https://github.com/dsperling/Swift-Kuery-PostgreSQL.git", .branch("issue_24")),

@irar2 your changes for issue #23 also appear to fix issue #24.

I will do some more testing and submit a PR, unless @irar2 you are planning on doing the same in #23.

@ianpartridge
Copy link
Contributor

Thanks for the info guys. @djones6 are you familiar with this issue?

@djones6
Copy link
Contributor

djones6 commented Feb 6, 2018

@ianpartridge I am not, but FWIW, the fix that @irar2 developed looks reasonable to me.
We should also document the choice of behaviour described in #23.

@dsperling thanks for the rebasing - please go ahead and submit a PR if you're satisfied the fix addresses your issue.

@dsperling
Copy link
Contributor

I have figured out the issue I was having. This was not a bug in Swift-Kuery-PostgreSQL. The culprit was a count function that grabbed a value without letting the resultSet.rows loop finish. The count function would be used for counting SELECT statements and was called repeatedly to create values for a chart.

getCount(sql: "SELECT count(some_column) FROM users where state = 1") { (result: Int) in
    ...
}

Here is the buggy function, which for some reason, the fix for #23 seemed to workaround:

func getCount(sql: String, callback: @escaping (Int) -> Void) {
    guard let connection = pool.getConnection() else {
        callback(0) // error - return zero
        return
    }
    connection.execute(sql) { queryResult in
        if let resultSet = queryResult.asResultSet {
            for row in resultSet.rows {
                for value in row {
                    if let intValue = value as? Int64 {
                        callback(Int(intValue))  // BUG!  Short circuit the for loop
                        return
                    }
                }
            }
        }
        callback(0)    // error - return zero
    }
}

With the fix below, I no longer see SELECT functions return successNoData

func getCount(sql: String, callback: @escaping (Int) -> Void) {
    guard let connection = pool.getConnection() else {
        callback(0) // error - return zero
        return
    }
    connection.execute(sql) { queryResult in
        var result: Int = 0
        if let resultSet = queryResult.asResultSet {
            for row in resultSet.rows {
                for value in row {
                    if let intValue = value as? Int64 {
                        result = Int(intValue)
                        break
                    }
                }
            }
        }
        callback(result)
    }
}

Questions:

  • Is there a better strategy to handle data with one row?
  • Could an assert be added to the code to alert developers to this issue?

No code changes are required from my perspective. I am not sure if this information might help the original author of this issue.

@ianpartridge
Copy link
Contributor

@EnriqueL8 could you take a look at this please?

@EnriqueL8
Copy link
Contributor

Hey @dsperling , I think this could be a robust strategy to handle data with one row:

func getCount(sql: String, callback: @escaping (Int) -> Void) {
    var result: Int = 0
    guard let connection = pool.getConnection() else {
        callback(result) // error - return zero
        return
    }
    connection.execute(sql) { queryResult in
        guard queryResult.success else {
          callback(result)
          return
        }

        // Rows is an array of dictionaries -->  [[String: Any?]]

        guard let rows = result.asRows, rows.count = 1 else {
          // Error
          callback(result)
          return
        }

        guard let dictionary = rows[0], dictionary.count = 1 {
          // Error
          callback(result)
          return
        }

        // if you know the key
        guard let intValue = dictionary[KEY] as? Int64 else {
          // Error
          callback(result)
          return
        }
        result = Int(intValue)

        // if you don't know the key
        for (key, value) in dictionary {
          guard let intValue = value as? Int64 else {
            // Error
            callback(result)
            return
          }
          result = Int(intValue)
        }

        callback(result)
    }
}

You could merge the guards together if you find it painful.

@dsperling
Copy link
Contributor

@EnriqueL8 thanks for the hints above using asRows. I have created a replacement function below. In looking at the asRows implementation, I saw the asValue variable and thought it might be what I should be using. However I don't see asValue work in the SELECT count() ... scenario. Should asValue work?

private func getCount(sql: String, callback: @escaping (Int) -> Void) {
    guard let connection = pool.getConnection() else {
        callback(0)
        return
    }
    connection.execute(sql) { queryResult in
        var result = 0
        guard queryResult.success,
            let rows = queryResult.asRows, rows.count == 1, rows[0].count == 1
            else {
                callback(result)    // Error
                return
        }
        for (_, value) in rows[0] {
            guard let intValue = value as? Int64 else {
                callback(result)    // Error
                return
            }
            result = Int(intValue)
        }
        callback(result)
    }
}

@EnriqueL8
Copy link
Contributor

Hey @dsperling , I don't think asValue should work because it is used when a query returns one value. In a SELECT statement, it returns a column(key=value pairs) which cannot be translate do just one value instead it is a dictionary with one entry.

@dsperling
Copy link
Contributor

@EnriqueL8 ok that makes sense. What is an example of a SQL query that would return a single value?

@EnriqueL8
Copy link
Contributor

I think for example when running a DELETE statement , you would get back .success with a value which would be the number of rows deleted.

@dsperling
Copy link
Contributor

@bridger and @t3ndai - does the discussion above help solve the problem you are seeing, or is the original bug report a different problem from what I experienced?

@t3ndai
Copy link

t3ndai commented Mar 8, 2018

i shutdown the server that was running the project since, it was a side project. Can always start it back up and let you guys know.

@bridger
Copy link
Author

bridger commented Mar 11, 2018

I've been running a fork of this repo with the issue_23 branch and it fixed the issue for me.

@dsperling
Copy link
Contributor

@bridger I also confirmed that issue_23 worked around the issue. However, fixing a bug in my code allowed me to use the master branch. The detail is above, but basically I was escaping a for loop early:

func getCount(sql: String, callback: @escaping (Int) -> Void) {
    ...
    connection.execute(sql) { queryResult in
        if let resultSet = queryResult.asResultSet {
            for row in resultSet.rows {
                for value in row {
                    if let intValue = value as? Int64 {
                        // BUG!  Short circuit the for loop
                        return
                    }
                }
            }
        }
    }
}

We were just wondering if your original bug report had a similar root cause.

@EnriqueL8
Copy link
Contributor

Thanks @bridger, @t3ndai, @dsperling ! I'll look into merging issue_23 so there is no need for forks! Let me know if you encounter other issues.

@EnriqueL8
Copy link
Contributor

EnriqueL8 commented Mar 13, 2018

Hey @dsperling , I have seen that you have merged issue_23 into your fork! Could you create a Pull Request so we can add it to Swift-Kuery-PostgreSQL?

djones6 pushed a commit that referenced this issue Mar 14, 2018
- terminates the first query fetcher when a second query is made using the same connection from within a result callback, to allow the inner query fetcher to function properly
- resolves #23 and #24
@djones6
Copy link
Contributor

djones6 commented Mar 14, 2018

The fix has been merged in #45 and tagged 1.1.2.

@djones6 djones6 closed this as completed Mar 14, 2018
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

No branches or pull requests

6 participants