By uncommenting the last two lines of the loop you are overwriting the $sth object which will necessarily affect the condition in the while test. If you don't want to overwrite $sth choose a different variable name to hold the inner statement handle.

In addition to the remarks of the others, you may want to read up on placeholders and bind values to reduce the amount of preparing of statements.

Because you're storing a new handle in $sth thus destroying $sth thus ending the loop

Here, in a nutshell, is what I think you should be doing.   Fortunately, it is an easy change:

1. You will need to use two statement-handles:   one for the SELECT statement, the second for the UPDATE.   As it is, you are destroying(!) the SELECT statement while you’re fetching from it, and that’s why the loop ends.
2. The UPDATE statement should use placeholders, not literal text, and so it should be PREPAREd once, outside of the loop, then executed repeatedly with a different set of substitutions for those placeholders each time.

For example, in relevant parts ... (extemporaneous coding, might have syntax errors ...)

# LET'S GIVE MEANINGFUL VARIABLE-NAMES TO THE TWO STATEMENT HANDLES ..
+.
$sth_select = $dbh->prepare("SELECT volumeletter,peernodes from VolDat
+a order
+ by volumeid");
$sth_select->execute();

$sth_update = $dbh->prepare("UPDATE VolData SET volstatus = ? WHERE vo
+lumeletter = ?");
# NOTICE THAT THE '?'S ARE NOT ENCLOSED IN QUOTES!
[download]

$sth_update->execute( [$vol_current_state, $vol] );
[download]

The question-marks in the prepared statement (which are not enclosed in quotes) act as “placeholders” for values which are provided each time the prepared statement is executed.

NOTE:   In the case of an SQLite database, you should also enclose the loop in a transaction.   When an UPDATE statement is executed outside of a transaction, SQLite will physically verify (re-read ...) the data after it has written, more than doubling the I/O activity and bringing things to a crawl.   When a transaction is active, SQLite performs the expected “lazy writes.”   This is not-so important for other database engines, but it is crucial for SQLite.

sundialsvc4:

You'll need to change that last line of code to:

$sth_update->execute( $vol_current_state, $vol );
[download]

...roboticus

When your only tool is a hammer, all problems look like your thumb.

Despite the advice to use transactions not being bad, like last time I call BS on the "physically verify" part. (Got any references?)

Also like last time, you're misrepresenting what "outside a transaction" means, since there is no such thing. There's only implicit and explicit transactions, and unlike what you say, SQLite performs the same actions at the end of either. Explicit transactions just help becase they delay those actions until the user choses to commit the transaction.

Hi perlfan,

sorry, but I have to disagree with your general advice not to use prepare and execute. IMHO this use case is exactly the one to use prepared statement handles. It was just abused or misunderstood by aditya1977.

I would rewrite the snippet to the following ona rush:

my $data_source = 'something';
my $username = 'user';
my $password = 'password';

$dbh = DBI->connect($data_source, $username, $password, {
    RaiseError => 1,
    AutoCommit => 1,
});

my $sql_select = "
SELECT
    volumeletter,
    peernodes
FROM
    VolData
ORDER BY
    volumeid
";

my $sql_update = "
UPDATE
    VolData
SET
    volstatus = ?
WHERE
    volumeletter = ?
";

my %NUM_2_VOLSTATUS = (
    0 => 'Not Mirrored',
    1 => 'Mirroring',
    2 => 'Resync',
    3 => 'Broken',
    4 => 'Paused',
    5 => 'Resync Pending',
);

my $sth = $dbh->prepare($sql_select);
my $sth_update = $dbh->prepare($sql_update);
$sth->execute();

# Iterate over configured volumes and update status, rrd data and grap
+hs
while ( my ($vol, $peernodes) = $sth->fetchrow_array() ) {
    # Populate a hash with current volume data
    my %voldata = getVolumeData($vol, $peernodes);
    my $vol_current_state = $NUM_2_VOLSTATUS{$voldata{'MirrorState'}} 
+|| 'Unknown';

    print "COLLECTOR: Setting volume $vol to state $vol_current_state\
+n";
    $sth_update->execute($vol_current_state, $vol);
}
$sth_update->finish;
$sth->finish;
$dbh->disconnect;
[download]

The two SQL statements are prepared and checked early. The statement for the update which is often used in the loop is prepared exactly once and used many times.

This solution has another advantage: On a database which supports server side result set cursors you really iterate over the result set row by row. You don't pull the whole result set to the DB client.

I also replaced the switch clause by a hash lookup which should be faster. More readable is it anyway, IMHO.

Regards
McA

You may, TIMTOWTDI =)