I wrote a very ugly piece of code, can someone help me correct it?

[glwa]

I am not very experienced with perl, but I know enough that this looks pretty bad

$dbh->begin_work;
my $href = $dbh->selectall_hashref("SELECT * FROM data WHERE ToScan IS
+ NULL AND title IS NOT NULL limit 1 FOR UPDATE SKIP LOCKED", "someid"
+);
foreach my $tmp (keys %$href) {
   $ID=$href->{$tmp}->{'id'}; 
   $title=$href->{$tmp}->{'title'};
   $channel=$tmp; 
}
my $rsth=$dbh->prepare("UPDATE data SET lock=1 WHERE id='$ID'"); $rsth
+->execute() || die $rsth->errstr;
$dbh->commit;

is there any cleaner way of doing this? I need to "...FOR UPDATE" then get the ID of selected row to set a lock on it. My code works but is very ugly. Thank you

[dsheroh]

Two things immediately stand out to me:

1) You have "limit 1" in your first query. Since it will only ever return a single row, there's no reason to use selectall_hashref. Use selectrow_array (if you're OK with identifying fields by position) or selectrow_hashref (if you want to access fields by name) instead and then you won't need to bother with looping over the (single) returned row.

If you opted for selectall/looping as a way to deal with the possibility that the query might return no rows, note that your existing code is broken for that case, since $ID will be undef when running the second query, causing it to lock all rows with NULL id fields. (You probably haven't noticed this bug because your table doesn't allow NULL ids, but it's still there.) Fetching a single row, you can check whether $ID (or another "NOT NULL" field) is undef to determine whether there was a matching row or not, and skip the second query if there's nothing to lock.

2) Although $ID did just come from the database, so it's probably safe, you really should use a placeholder in the second query instead of interpolating it into the query string. Mostly just for the sake of keeping up the habit of using placeholders whenever possible, but I also can't rule out the possibility that someone might be able to exploit a weird corner case to turn it into an SQL injection vulnerability.

A quick (and completely untested) rewrite:

$dbh->begin_work;
my $channel = $dbh->selectrow_hashref("SELECT * FROM data WHERE ToScan
+ IS NULL AND title IS NOT NULL limit 1 FOR UPDATE SKIP LOCKED");
my $ID = $channel->{'id'}; 
my $title = $channel->{'title'};

# $dbh->do returns the number of rows affected on success or undef
# on failure, so you need to use defined-or (//) here instead of a
# regular or (||) unless you want "zero rows affected" to be
# treated as a fatal error.
if (defined $ID) {
  $dbh->do("UPDATE data SET lock=1 WHERE id=?", undef, $ID) // die $rs
+th->errstr;
}
$dbh->commit;

[marto]

Alternatively, if $title isn't being used elsewhere, since a single row is being returned something like:

Update data set lock=1 where id=( select id from data where id is not null and .... <rest of the where clause here> .... )

[glwa]

using selectrow_hashref is exactly what I was looking for. It was killing me having to loop unnecessarily through that single row. Also I think I cannot use selectrow_array, but I will go play with it now to make sure. In regards to having query return no rows, you are completely right. Although this table has 100 million records now sometime in the future I may have parsed all data and there might be no further data to parse Thank you for all these helpful hints, I really appreciate it