Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Fix segfault when running command through execute_reader

In this case we just return an empty reader, since that seems to be the
most sensible. Not all drivers support this distinction anyway, so
raising an exception here is hard. Also since the query does succeed,
the exception could be confusing as in that it actually executes while
that was not expected.

Related to #56
  • Loading branch information...
commit b7ba9a842cc785230956fa9c4c235b4b030007db 1 parent 8a55ade
@dbussink dbussink authored
View
7 data_objects/lib/data_objects/spec/shared/command_spec.rb
@@ -101,6 +101,13 @@
expect { @arg_reader.execute_reader(nil, nil) }.not_to raise_error(ArgumentError)
end
+ it 'returns an empty reader if the query does not return a result' do
+ runs_command = @connection.create_command("UPDATE widgets SET name = '' WHERE name = ''")
+ res = runs_command.execute_reader
+ res.fields.should == []
+ res.next!.should == false
+ end
+
end
describe 'with a valid reader and ? inside column alias' do
View
7 do_mysql/ext/do_mysql/do_mysql.c
@@ -504,10 +504,6 @@ VALUE do_mysql_cCommand_execute_reader(int argc, VALUE *argv, VALUE self) {
MYSQL *db = DATA_PTR(mysql_connection);
MYSQL_RES *response = do_mysql_cCommand_execute(self, connection, db, query);
- if (!response) {
- rb_raise(eDO_ConnectionError, "No result set received for a query that should yield one.");
- }
-
unsigned int field_count = mysql_field_count(db);
VALUE reader = rb_funcall(cDO_MysqlReader, DO_ID_NEW, 0);
@@ -587,6 +583,9 @@ VALUE do_mysql_cReader_next(VALUE self) {
}
MYSQL_RES *reader = DATA_PTR(reader_container);
+ if(!reader) {
+ return Qfalse;
+ }
MYSQL_ROW result = mysql_fetch_row(reader);
// The Meat
View
6 do_postgres/ext/do_postgres/do_postgres.c
@@ -545,7 +545,8 @@ VALUE do_postgres_cCommand_execute_reader(int argc, VALUE *argv, VALUE self) {
PGconn *db = DATA_PTR(postgres_connection);
PGresult *response = do_postgres_cCommand_execute(self, connection, db, query);
- if (PQresultStatus(response) != PGRES_TUPLES_OK) {
+ int status = PQresultStatus(response);
+ if(status != PGRES_TUPLES_OK && status != PGRES_COMMAND_OK) {
do_postgres_raise_error(self, response, query);
}
@@ -565,8 +566,7 @@ VALUE do_postgres_cCommand_execute_reader(int argc, VALUE *argv, VALUE self) {
if (field_types == Qnil || 0 == RARRAY_LEN(field_types)) {
field_types = rb_ary_new();
infer_types = 1;
- }
- else if (RARRAY_LEN(field_types) != field_count) {
+ } else if (RARRAY_LEN(field_types) != field_count) {
// Whoops... wrong number of types passed to set_types. Close the reader and raise
// and error
rb_funcall(reader, rb_intern("close"), 0);

7 comments on commit b7ba9a8

@godfat

Can we have this fix released on rubygems.org soon?

@dbussink
Owner

Is it actually causing a problem? Since this only happens because it uses DO in an invalid way, so under normal usage it should not happen. Not to say it shouldn't crash of course.

@godfat

As I showed in https://gist.github.com/godfat/5626115#file-segfault-rb
I think altering table is a pretty common use case?

The program I am writing at the moment is actually a migration script,
which migrates all our old data into new schema. For now I am using
psql directly as a workaround. It works and probably works better,
but since I already used datamapper and setup the database connection
properly, it would be good if I could reuse it instead of using psql,
and handing connection setup once again.

@dbussink
Owner

Altering a table shouldn't be done with the execute_reader API. Altering a table should be done like this:

conn.create_command(<<-EOF).execute_non_query
  CREATE TABLE users (
    id                INTEGER GENERATED BY DEFAULT AS IDENTITY(START WITH 1),
    name              VARCHAR(200) default 'Billy' NULL,
    fired_at          TIMESTAMP
 )
EOF

This is actually what I meant with invalid usage of the API. For statements that are not queries, but DDL statements, you should use the create_command / execute_non_query API.

@godfat

Changing from DataMapper::Adapters::PostgresAdapter#select to DataMapper::Adapters::PostgresAdapter#execute works well, thanks!

Though I do still think any segfault should be fixed (and released) as soon as possible, but at least I can work with current release and use the proper API.

@dbussink
Owner

Yeah, will release it when I can, the release process is a bit more involved that other gems usually have, since it also has to build cross compiled gems for Windows for example.

@godfat

I see, thank you for your help and effort!

Please sign in to comment.
Something went wrong with that request. Please try again.