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

Possible nexset() misbehaviour #185

Closed
mi-volodin opened this issue Nov 16, 2017 · 8 comments · Fixed by #242
Closed

Possible nexset() misbehaviour #185

mi-volodin opened this issue Nov 16, 2017 · 8 comments · Fixed by #242
Labels
enhancement protocol requires Vertica protocol knowledge

Comments

@mi-volodin
Copy link

mi-volodin commented Nov 16, 2017

Current nextset() def:

def nextset(self):
        # skip any data for this set if exists
        self.flush_to_command_complete()

        if self._message is None:
            return False
        elif isinstance(self._message, messages.CommandComplete):
            # there might be another set, read next message to find out
            self._message = self.connection.read_message()
            if isinstance(self._message, messages.RowDescription):
                # next row will be either a DataRow or CommandComplete
                self._message = self.connection.read_message()
                return True
            elif isinstance(self._message, messages.ReadyForQuery):
                return False
            elif isinstance(self._message, messages.ErrorResponse):
                raise errors.QueryError.from_error_response(self._message, self.operation)
            else:
                raise errors.Error(
                    'Unexpected nextset() state after CommandComplete: {0}'.format(self._message))
        elif isinstance(self._message, messages.ReadyForQuery):
            # no more sets left to be read
            return False
        else:
raise errors.Error('Unexpected nextset() state: {0}'.format(self._message))

In particular, this works fine if and only if our consequent select statements return any row description.
If we consider any case with not select statement it fails with error 'Unexpected nextset() state after CommandComplete: .

I might be wrong, but that is not what is expected in case I have multiple statements of mixed type in one call. And same for the case with only DDL statements.

Just for example consider
query = "select 1; select 2; drop table if exists aaa;"

The messages sequence is as follows:

DataRow
CommandComplete
RowDescription
DataRow
CommandComplete
NoticeResponse
CommandComplete
ReadyForQuery

After nextset second call the read_message returns NoticeResponce which is not described in processing cycle -- and the error is thrown. I think adding it there would be logically sound.

Also, I think nextset() in general should work for no SELECT statements at all. However, in general DDL commands provide empty set, which is still a set.

@nchammas
Copy link
Contributor

I agree that it would be more consistent if we could use nextset() regardless of the type of query that is run.

For example, if we do something like this:

cursor.execute(
"""
    CREATE TABLE IF NOT EXISTS ...;
    INSERT INTO ...;
    GRANT ...;
""")

I'd want to be able to check that everything ran fine. It seems like nextset() should be usable like this:

while cursor.nextset():
    pass

However, if I understood @mi-volodin correctly, this will only work if all the statements are SELECT statements.

@sitingren
Copy link
Member

Good catch! Actually, NoticeResponce and ParameterStatus are asynchronous messages, which need to be handled separately.

@sitingren sitingren added enhancement protocol requires Vertica protocol knowledge labels Oct 1, 2018
@schimmy
Copy link

schimmy commented Oct 31, 2018

I would say this is a bug, not an enhancement - how can I verify the accuracy of my calls through the library? Right now a script I run has an error which I can't catch, since the query is not a select.

@tomwall
Copy link

tomwall commented Oct 31, 2018

Yes, it is definitely problematic that you can't get errors out for DDL or other commands that don't use RowDescriptions. We could address some of the the immediate need by whitelisting CommandComplete after another CommandComplete in the snippet above, but that's not the whole story.

Asynchronous messages like notice & parameter status are valid in this sequence too. Those messages are not fatal and generally shouldn't result in an exception being thrown. To relay that information, we need a new API, hence the enhancement request. Other clients have polling (e.g. JDBC's ResultSet.getWarnings()) or event driven (ADO.NET's InfoMessage) API's to check for warnings out of band. We need something like that to have a complete solution.

It is important to address these problems together because they happen together often in the context of DDL. Even if you whitelist CommandComplete above, a successful "drop table if exists" generates a notice if the table didn't exist, and right now that would also cause an error to be thrown when it shouldn't.

@hannahshachar
Copy link

I have the opposite problem, I have a statement that drops the table if it exists and then creates the table again with a long query. As soon as it Drops the table, it moves on in the python to the next part, not waiting for the second query to complete. Is it the same solution?

@mi-volodin
Copy link
Author

I think it might be connected to it, but it depends on how you process the result of your query.

Actually, in your case I would recommend to split the drop and create statements. From the database perspective any DDL operation closes transaction (or it is a transaction itself).

So your two consequent commands are actually independent transactions.

Hence, if it is a problem on the very first step - you probably shouldn't run the next one. But in multicommand statement it won't be considered automatically. Which brings us to a program design, where you control the result of the previous statement synchronously, and thus have to run it separately.

@hannahshachar
Copy link

We have tons of code (hundreds of scripts) that work fine this way with pyodbc, so we are looking for a solution that is not rewriting all of the code just to make it work in vertica_python.

@mi-volodin
Copy link
Author

Well, then it is best to investigate the real reason for your behavior. Or describe the case more specifically so others can do that.

From my point of view - it sounds more serious.
In my cases all statements were executed before I got the control back.

But all this happened years ago - there might be some significant changes to API or the way the statements are executed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement protocol requires Vertica protocol knowledge
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants