DBI->execute returns number of matched rows instead of number of affected

[justdit]

Hi there,

I was looking into the DBI doc which says (source) :

For a non-SELECT statement, execute returns the number of rows affected, if known. If no rows were affected, then execute returns "0E0", which Perl will treat as 0 but will regard as true. Note that it is not an error for no rows to be affected by a statement. If the number of rows affected is not known, then execute returns -1.

But it looks like DBD::mysql returns number of matched rows by default (source):

#ifdef MYSQL_NO_CLIENT_FOUND_ROWS
    client_flag = 0;
#else
    client_flag = CLIENT_FOUND_ROWS;
#endif

I would like to understand the reasoning behind it. I would assume drivers should be complaint with interface itself.

[pali]

Can you provide example script/code which returns incorrect value?

[justdit]

Basically, it's just any update statement that doesn't really affect the rows.

So let's update the row to value that's completely new:

strace -s1024  -etrace=recvfrom perl -lwe '
    use DBI;

    my $dbh= DBI->connect("DBI:mysql:database=my_test_db", "root", "**********",
                          { RaiseError => 1, PrintError => 1, AutoCommit => 1});


    my $sth = $dbh->prepare(q{UPDATE test_table SET bin = "new_value" where name = "name1" });
    my $rows_changed = $sth->execute();
    print STDERR "ROWS CHANGED:$rows_changed\n";
'

In this case mysql has updated the column bin value:

...
recvfrom(3, "0\0\0\1\0\2\0\"\0\0\0(Rows matched: 2  Changed: 2  Warnings: 0", 16384, 0, NULL, NULL) = 52
ROWS CHANGED:2

If you replay that command once again, you'll see that mysql returns "Changed: 0", however script says that it updated 2 rows:

...
recvfrom(3, "0\0\0\1\0\2\0\"\0\0\0(Rows matched: 2  Changed: 0  Warnings: 0", 16384, 0, NULL, NULL) = 52
ROWS CHANGED:2

[pali]

Ok, now I did some investigation.

Hm... this behavior was changed in commit f2aa689 in 2003 for DBD::mysql version 2.9001.

And after that default behavior is to return number of found rows, not changed -- which is against DBI documentation.

Still there is compile time option --nofoundrows:

  --nofoundrows          Change the behavior of \$sth->rows() so that it
  			 returns the number of rows physically modified
			 instead of the rows matched

which adds compiler flag -DDBD_NO_CLIENT_FOUND_ROWS. But as you wrote check in dbdimp.c is done for #ifdef MYSQL_NO_CLIENT_FOUND_ROWS -- so for different compiler flags... Which means --nofoundrows has no effect. This wrong name was introduced in same commit f2aa689. So at least compile time option --nofoundrows should be fixed.

Moreover there is runtime DBD::mysql option mysql_client_found_rows:
https://metacpan.org/pod/DBD::mysql#mysql_client_found_rows

Enables (TRUE value) or disables (FALSE value) the flag CLIENT_FOUND_ROWS
while connecting to the MySQL server. This has a somewhat funny effect:
Without mysql_client_found_rows, if you perform a query like

  UPDATE $table SET id = 1 WHERE id = 1;

then the MySQL engine will always return 0, because no rows have changed.
With mysql_client_found_rows however, it will return the number of rows
that have an id 1, as some people are expecting. (At least for compatibility
to other engines.)

So description contradict fact that is correct behavior (compatibility to other engines).

Should be this default value for mysql_client_found_rows changed to match DBI documentation? And how other DBI modules returns?

CCing DBD::mysql maintainers @mbeijen @CaptTofu Can you comment this issue?

[CaptTofu]

That's been a long time since that change! I would say it ought to do whatever other drivers do and the documentation should match that functionality (and the contradiction removed)

…

On 3/28/17 3:31 PM, pali wrote: Ok, now I did some investigation. Hm... this behavior was changed in commit f2aa689 <f2aa689> in 2003 for DBD::mysql version 2.9001. And after that default behavior is to return number of found rows, not changed -- which is against DBI documentation. Still there is compile time option |--nofoundrows|: |--nofoundrows Change the behavior of \$sth->rows() so that it returns the number of rows physically modified instead of the rows matched | which adds compiler flag |-DDBD_NO_CLIENT_FOUND_ROWS|. But as you wrote check in |dbdimp.c| is done for |#ifdef MYSQL_NO_CLIENT_FOUND_ROWS| -- so for different compiler flags... Which means |--nofoundrows| has no effect. This wrong name was introduced in same commit f2aa689 <f2aa689>. So at least compile time option |--nofoundrows| should be fixed. Moreover there is runtime DBD::mysql option |mysql_client_found_rows|: https://metacpan.org/pod/DBD::mysql#mysql_client_found_rows |Enables (TRUE value) or disables (FALSE value) the flag CLIENT_FOUND_ROWS while connecting to the MySQL server. This has a somewhat funny effect: Without mysql_client_found_rows, if you perform a query like UPDATE $table SET id = 1 WHERE id = 1; then the MySQL engine will always return 0, because no rows have changed. With mysql_client_found_rows however, it will return the number of rows that have an id 1, as some people are expecting. (At least for compatibility to other engines.) | So description contradict fact that is correct behavior (compatibility to other engines). Should be this default value for |mysql_client_found_rows| changed to match DBI documentation? And how other DBI modules returns? CCing DBD::mysql maintainers @mbeijen <https://github.com/mbeijen> @CaptTofu <https://github.com/CaptTofu> Can you comment this issue? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#111 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGgridCFpi3E6ZvwTISl0sPFCY3Wz0Gks5rqWAmgaJpZM4MqGfO>.

[pali]

If you agree with this fact that default behavior would be changed, then I can prepare patch which change default value of mysql_client_found_rows and affects return value of execute.

[justdit]

I've justed tested the script against dbd::pg(postresql) and dbd::sqlite, and it looks like they return the number of found rows as opposed to the DBI documentation as well. So with that comment below looks correct:

it will return the number of rows that have an id 1, as some people are expecting. (At least for compatibility to other engines.)

As noted in comment, people mostly care about the end result: whether the given rows have the given values. So for that case, I assume returning matched rows makes more sense.

Thanks for looking into this, but I now think default option should stay as it's now. Nevertheless, it's worth fixing the typo in the option itself.

[pali]

Ok, so just writing this information to documentation (execute does not follow DBI doc) and fixing --nofoundrows for Makefile.PL?

[Grinnz]

In my experience DBD::mysql does return number of affected rows (as mysql protocol returns), not number of matched rows, and we are relying on this somewhat. We have not set any special configuration in DBD::mysql.

[pali]

@Grinnz Are you sure? Can you test @justdit's example? It looks strange as reported behavior is what can be really deduced from source code.

[justdit]

@pali, yes, it makes sense to explicitly state that execute returns the number of matched rows. There's some comment under the mysql_client_found_rows option, but people usually refer to it for advances use cases. That said the best would be updating DBI docs with some clarification.

Yes, --nofoundrows also needs to be fixed to match the check for MYSQL_NO_CLIENT_FOUND_ROWS.

[mjegh]

use 5.016;
use DBI;
use strict;
use warnings;

my $h = DBI->connect('dbi:SQLite:db=fred.db');
$h->do(q/create table test (a int, b varchar(10))/);
$h->do(q/insert into test values (1, 'fred')/);

foreach (1..2) {
    my $affected = $h->do(q/update test set b = ? where b = ?/, undef, 'dave', 'fred');
    say $affected;
}

outputs:

1
0E0

as I'd expect and as DBI documents. If DBD::MySQL does not do this it is broken. As far as I am concerned DBD::SQLite does behave like the DBI docs say.

[justdit]

@mjegh, it's because on the second run you don't have matched rows anymore(no more rows with "fred")

Check the following code:

use 5.016;
use DBI;
use strict;
use warnings;

my $h = DBI->connect('dbi:SQLite:db=fred.db');
$h->do(q/create table test (a int, b varchar(10), c varchar(10))/);
$h->do(q/insert into test values (1, 'bobo', 'value1')/);
$h->do(q/insert into test values (1, 'bobo', 'value2')/);
  
foreach (1..2) {
    my $affected = $h->do(q/update test set c = ? where b = ?/, undef, 'new_value', 'bobo');
    say $affected;
}

Output:

2
2

[mjegh]

But in your example 2 rows are updated in the first update and 2 rows are updated in the second update. That is correct behaviour - do returned the affected rows. So I've obviously missed something. What is DBD::MySQL doing differently to that and if it does the same as the SQLite example what do you think is wrong with it?

[justdit]

@mjegh, by affected I mean physically updated to the new value. As you see from the code, I'm setting to the same 'new_value' in both iterations, and getting "2 rows updated" in both cases. However, the second update is idempotent, and doesn't actually "change" anything.

[mjegh]

That is just not how it works. If you don't want the rows which have not changed value, you'll have to add them to a where clause on your update. See this example which should hopefully show the update occurs even though the column hasn't actually changed value:

use 5.016;
use DBI;
use strict;
use warnings;
use Data::Dumper;

my $h = DBI->connect('dbi:SQLite:db=fred.db');
$h->do(q/create table test (a int, b varchar(10), c varchar(10), updated timestamp default 0)/);
$h->do(q/create trigger test_trigger after update on test for each row begin update test set updated = current_timestamp; end/);
$h->do(q/insert into test (a, b, c) values (1, 'bobo', 'value1')/);
$h->do(q/insert into test (a, b, c) values (1, 'bobo', 'value2')/);

foreach (1..2) {
    my $affected = $h->do(q/update test set c = ? where b = ?/, undef, 'new_value', 'bobo');
    say $affected;
}

say Dumper ($h->selectall_arrayref(q/select * from test/));

2
2
$VAR1 = [
          [
            1,
            'bobo',
            'new_value',
            '2017-03-31 10:04:01'
          ],
          [
            1,
            'bobo',
            'new_value',
            '2017-03-31 10:04:01'
          ]
        ];
```

See how the trigger fired on the update and updated the updated column.

[justdit]

@mjegh, yes, that's why I got more inclined to think that the current default is better -- due to automagic timestamps, triggers, etc.

[Grinnz]

@pali I tested and it seems you are correct, it is returning the number of matched rows. Either I misremembered or something has changed; possibly on the server side?

[mjegh]

@pali From the definition of DBI, matched rows is the correct answer. DBI borrowed some definitions from elsewhere (like ODBC) and like ODBC, the rows affected are rows matched in MySQL terms. See my example with a trigger and DBD::SQLite - it would be mighty strange if the trigger triggered on an update and updated rows but DBD::MySQL returned 0 just because a column wasn't actually changed during an update. Undoubtedly there will be a lot of code which relies on this correct behaviour.

[pali]

Ah, different definitions... :-(

[pali]

Anyway, here is pull request for fixing compile argument --nofoundrows: #113

[mbeijen]

Merged pull request, thanks!

[pali]

Fix from #113 was reverted in 4.043.