onegative has asked for the wisdom of the Perl Monks concerning the following question:

G'day most honorable Monks,

I am asking for assistance in my approach to coding a writer into a MySQL database. As you can no doubt see from my coding I am sure you all would consider me a novice Perl coder and I would appreciate any critiques about my approach.

The purpose of this code will be to sit on four or more servers where work is performed and therefore watch an active log from each server. I intend on running a File::Tail script to watch these logs and then process the entries into a single MySQL database, four tables. My concern is with the use of this script on four or more servers and the possible contention/locks on tables as different servers attempt to write their data. I am wondering are there ways to change my code to help reduce this situation or whether this is a valid concern in the first place. All these servers are a minimum of 4 processor/4G memory with very low loads.

The stored data will then be used as a central advance log search tool via a PHP interface.

Again, I welcome constructive criticism as it helps me learn and grow my skill with Perl.

Thank you in Advance,
Danny

p.s. I forgot to mention the code does function as it has been tested on my development system. But short-cuts are always welcomed.

#!/usr/local/bin/perl -w use strict; use DBI; use File::Tail; # Create File::Tail object my $file; $file=File::Tail->new("/opt/app/superspy/tmp/commLogger.log"); my $line; while (defined($line=$file->read)) { chomp($line); # print "$line\n"; # Create DB connection my $db_handle = DBI->connect("dbi:mysql:database=P0TA6E01;host +=myhost02.fcc.hide.com:3306;user=xxxxxxxx;password=xxxxxxxx") || die +"Couldn't connect to database: $DBI::errstr\n"; if($line =~ /^AlertCmd~.*~.*~-reload.*/){next;} if($line =~ /^AlertCmd~(.*)~(.*)~(.*)~(.*)~(.*)~(.*)/){ my $tstamp = $1; my $cmnd_id = $2; my $cmnd = $3; my $client_host = $4; my $client_user = $5; my $svr_host = $6; $svr_host =~ s/\..*//g; my $sql = "INSERT INTO ALERTS (TSTAMP,CMND_ID,CMND,CLI +ENTHOST,CLIENTUSER,SVR_HOST) VALUES (\'$tstamp\',\'$cmnd_id\',\'$cmnd +\',\'$client_host\',\'$client_user\',\'$svr_host\')\;\n"; my $statement = $db_handle->prepare($sql) or die "Couldn't prepare query '$sql': $DBI::errst +r\n"; $statement->execute() or die "Couldn't execute query '$sql': $DBI::errst +r\n"; } if($line =~ /^SendFiltered~(.*)~(.*)~(.*)~(.*)~(.*)~(.*)/){ my $tstamp = $1; my $cmnd_id = $2; my $alert_id = $3; my $send_id = $4; my $state = $5; my $svr_host = $6; $svr_host =~ s/\..*//g; my $sql = "INSERT INTO FILTERED (TSTAMP,CMND_ID,ALERT_ +ID,SEND_ID,STATE,SVR_HOST) VALUES (\'$tstamp\',\'$cmnd_id\',\'$alert_ +id\',\'$send_id\',\'$state\',\'$svr_host\')\;\n"; my $statement = $db_handle->prepare($sql) or die "Couldn't prepare query '$sql': $DBI::errst +r\n"; $statement->execute() or die "Couldn't execute query '$sql': $DBI::errst +r\n"; } if($line =~ /^SendOffDuty~(.*)~(.*)~(.*)~(.*)~(.*)~(.*)/){ my $tstamp = $1; my $cmnd_id = $2; my $alert_id = $3; my $send_id = $4; my $state = $5; my $svr_host = $6; $svr_host =~ s/\..*//g; my $sql = "INSERT INTO FILTERED (TSTAMP,CMND_ID,ALERT_ +ID,SEND_ID,STATE,SVR_HOST) VALUES (\'$tstamp\',\'$cmnd_id\',\'$alert_ +id\',\'$send_id\',\'$state\',\'$svr_host\')\;\n"; my $statement = $db_handle->prepare($sql) or die "Couldn't prepare query '$sql': $DBI::errst +r\n"; $statement->execute() or die "Couldn't execute query '$sql': $DBI::errst +r\n"; } if($line =~ /^SendStarted~(.*)~(.*)~(.*)~(.*)~(.*)~(.*)~(.*)~( +.*)~(.*)~(.*)~(.*)/){ my $tstamp = $1; my $cmnd_id = $2; my $alert_id = $3; my $send_id = $4; my $user = $5; my $destination = $6; my $pin = $7; my $to = $8; my $message = $9; my $state = $10; my $svr_host = $11; $svr_host =~ s/\..*//g; my $address; if($pin eq ""){$address = $to;}else{$address = $pin;} my $sql = "INSERT INTO SENDS (START_TIME,CMND_ID,ALERT +_ID,SEND_ID,USER,DESTINATION,ADDRESS,MESSAGE,STATE,SVR_HOST,ACTIVE) V +ALUES (\'$tstamp\',\'$cmnd_id\',\'$alert_id\',\'$send_id\',\'$user\', +\'$destination\',\'$address\',\'$message\',\'$state\',\'$svr_host\',\ +'TRUE\')\;\n"; my $statement = $db_handle->prepare($sql) or die "Couldn't prepare query '$sql': $DBI::errst +r\n"; $statement->execute() or die "Couldn't execute query '$sql': $DBI::errst +r\n"; } if($line =~ /^SendCompleted~(.*)~(.*)~(.*)~(.*)~(.*)~(.*)/){ my $tstamp = $1; my $cmnd_id = $2; my $alert_id = $3; my $send_id = $4; my $state = $5; my $svr_host = $6; $svr_host =~ s/\..*//g; my $sql = "UPDATE SENDS SET END_TIME=\'$tstamp\',STATE +=\'$state\',ACTIVE=\'FALSE\' WHERE CMND_ID=\'$cmnd_id\' AND ALERT_ID= +\'$alert_id\' AND SEND_ID=\'$send_id\' AND SVR_HOST=\'$svr_host\'\;\n +"; my $statement = $db_handle->prepare($sql) or die "Couldn't prepare query '$sql': $DBI::errst +r\n"; $statement->execute() or die "Couldn't execute query '$sql': $DBI::errst +r\n"; } if($line =~ /^SendCleared~(.*)~(.*)~(.*)~(.*)~(.*)~(.*)/){ my $tstamp = $1; my $cmnd_id = $2; my $alert_id = $3; my $send_id = $4; my $state = $5; my $svr_host = $6; $svr_host =~ s/\..*//g; my $sql = "UPDATE SENDS SET END_TIME=\'$tstamp\',STATE +=\'$state\',ACTIVE=\'FALSE\' WHERE CMND_ID=\'$cmnd_id\' AND ALERT_ID= +\'$alert_id\' AND SEND_ID=\'$send_id\' AND SVR_HOST=\'$svr_host\'\;\n +"; my $statement = $db_handle->prepare($sql) or die "Couldn't prepare query '$sql': $DBI::errst +r\n"; $statement->execute() or die "Couldn't execute query '$sql': $DBI::errst +r\n"; } if($line =~ /^AlertError~(.*)~(.*)~(.*)~(.*)~(.*)~(.*)~(.*)~(. +*)/){ my $tstamp = $1; my $cmnd_id = $2; my $alert_id = $3; my $send_id = $4; my $destination = $5; my $message = $6; my $state = $7; my $svr_host = $8; $svr_host =~ s/\..*//g; my $sql = "INSERT INTO ERRORS (TSTAMP,CMND_ID,ALERT_ID +,SEND_ID,DESTINATION,MESSAGE,STATE,SVR_HOST) VALUES (\'$tstamp\',\'$c +mnd_id\',\'$alert_id\',\'$send_id\',\'$destination\',\'$message\',\'$ +state\',\'$svr_host\')\;\n"; my $statement = $db_handle->prepare($sql) or die "Couldn't prepare query '$sql': $DBI::errst +r\n"; $statement->execute() or die "Couldn't execute query '$sql': $DBI::errst +r\n"; } $statement->finish(); $db_handle->disconnect(); }

Replies are listed 'Best First'.
Re: Need Critique (Good or Bad)
by moritz (Cardinal) on Feb 05, 2008 at 16:08 UTC
    Just a few things from skimming over your code:

    Use Placeholders!.

    Instead of

    my $sql = "INSERT INTO ERRORS (TSTAMP,CMND_ID,ALERT_ID,SEND_ID,DESTIN +ATION,MESSAGE,STATE,SVR_HOST) VALUES (\'$tstamp\',\'$cmnd_id\',\'$ale +rt_id\',\'$send +_id\',\'$destination\',\'$message\',\'$state\',\'$svr_host\')\;\n"; my $statement = $db_handle->prepare($sql) or die "Couldn't prepare query '$sql': $DBI::errst +r\n"; $statement->execute() or die "Couldn't execute query '$sql': $DBI::errst +r\n";
    write
    my $sql = qq{"INSERT INTO ERRORS (TSTAMP,CMND_ID,ALERT_ID,SEND_ID,DEST +INATION,MESSAGE,STATE,SVR_HOST) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?); my $statement = $dbh->prepare($sql); $statement->execute($tstamp, $cmnd_id, ...);

    Also make sure to pass the {RaiseError =>1} argument to DBI->connect, then you don't need the custom error handlers.

    Even more important: you can prepare the statements once outside the loop, and then call execute them inside the loops as many times as you need them. That way you'll gain much performance (if you have many iterations).

    Instead of

    if($line =~ /^SendCleared~(.*)~(.*)~(.*)~(.*)~(.*)~(.*)/){ my $tstamp = $1; my $cmnd_id = $2; my $alert_id = $3; my $send_id = $4; my $state = $5; my $svr_host = $6;
    you can first check if the line starts with SendCleared and then use
    my ($tstamp, $cmnd_id, ...) = split m/~/, $line;

    There are actually even better ways to handle the dispatch with a hash: you can store these sql queries in a has with keys SendOffDuty, AlertCmd and so on, and then do something like that:

    # assume %q contains the quries my ($query, @args) = split $line, '~'; $q{$query}->execute(@args); # assuming the log files are well formed
Re: Need Critique (Good or Bad)
by thparkth (Beadle) on Feb 05, 2008 at 16:38 UTC
    There's always more than one way to do it, but I think you're working harder here than you have to.

    All your

    if ($line =~...

    ...blocks are very similar.

    I would probably suggest not using the heavy regular expressions as you are doing, but rather using the split operator. And I wouldn't treat the first token in the line any differently from the rest. I'd also be lazier than you about picking out each field by name. Something like this:

    my ($type,@tokens)=split(/\~/,$line); if ($type eq "SendFiltered") { my ($tstamp,$cmnd_id,$alert_id,$send_id,$state,$svr_host) = @tokens[0-5]; my $sql = "INSERT ... } elsif ($type eq "AlertError") { ... }

    However even that isn't really sufficiently lazy. The SQL is still there, and it's still ugly. (This isn't your fault - SQL is just plain ugly.)

    It seems to me that since you're not doing anything at all with all of these values except sticking them in the SQL statements, you don't actually need to bother naming them at all. Really all you need to know for each "first token" type is what the SQL statement should be, and which column numbers to plug in.

    With that in mind, you could do something like this:

    # %statements is an array of prepared SQL statement objects ready to h +ave values plugged in %statements=( "AlertError"=>$dbh->prepare("INSERT INTO ERRORS (TSTAMP,CMND_ID,ALER +T_ID,SEND_ID,DESTINATION,MESSAGE,STATE,SVR_HOST) VALUES (?,?,?,?,?,?, +?,?)"), "SendCleared"=>$dbh->prepare("UPDATE SENDS..."), "... ); # %columns is an array of column position indicators corresponding to +the %statements above %columns=( "AlertError"=>[0,1,2,3,4,5], "SendCleared"=>[... );

    Then your actual executable code becomes quite easy...

    while ($line=...) { my ($type,@tokens)=split(/\~/,$line); $statements{$type}->execute(@tokens[@{$columns{$type}}]); }

    There are various other ways you could organise the data, including using OOP. The key point is that the SQL statements and the column mappings really are data, not code, from the point of view of your Perl program.

Re: Need Critique (Good or Bad)
by apl (Monsignor) on Feb 05, 2008 at 16:23 UTC
    In addition to all of the above, I'd suggest taking all of your $line testing and putting it in one procedure. This provides two benefits:
    • You can define $sql once, at the top, and have a single
      my $statement = $db_handle->prepare($sql) or die "Couldn't prepare query '$sql': $DBI::errst +r\n"; $statement->execute() or die "Couldn't execute query '$sql': $DBI::errst +r\n";
      after the conditionals if you have a non-null $sql.
    • The general flow of logic will be more evident
Re: Need Critique (Good or Bad)
by holli (Abbot) on Feb 05, 2008 at 22:57 UTC
    I have refactored your code. it's untested (but should run) and shows some of the principles mentioned in the other replies, including the use of a dispatch table. feel free to ask if you don't understand something.
    #!/usr/local/bin/perl -w use strict; use DBI; use File::Tail; # query subs, fetch and name the parameters and call the prepared stat +ement sub insert_alerts { my ($sth, $tstamp, $cmnd_id, $cmnd , $client_host, $client_user, $ +svr_host, $svr_host) = @_; $svr_host =~ s/\..*//g; $sth->execute( $tstamp, $cmnd_id, $cmnd , $client_host, $client_us +er, $svr_host, $svr_host ); return $DBI::errstr ? 0 : 1; } sub insert_filters { my ($sth, $tstamp, $cmnd_id, $cmnd , $client_host, $client_user, $ +svr_host, $svr_host) = @_; $svr_host =~ s/\..*//g; $sth->execute( $tstamp, $cmnd_id, $cmnd , $client_host, $client_us +er, $svr_host, $svr_host ); return $DBI::errstr ? 0 : 1; } sub insert_sends { my ($sth, $tstamp, $cmnd_id, $alert_id, $send_id, $user, $destinat +ion, $pin, $to, $message, $state, $svr_host) = @_; $svr_host =~ s/\..*//g; $sth->execute( $tstamp, $cmnd_id, $alert_id, $send_id, $user, $des +tination, $pin||$to, $message, $state, $svr_host, 'TRUE' ); return $DBI::errstr ? 0 : 1; } sub update_sends { my ($sth, $tstamp, $cmnd_id, $alert_id, $send_id, $state, $svr_hos +t = $6) = @_; $svr_host =~ s/\..*//g; $sth->execute( $tstamp, $state, $cmnd_id, $alert_id, $send_it, $sv +r_host ); return $DBI::errstr ? 0 : 1; } sub insert_errors { my ($sth, $tstamp, $cmnd_id, $cmnd , $client_host, $client_user, $ +svr_host, $svr_host) = @_; $svr_host =~ s/\..*//g; $sth->execute( $tstamp, $cmnd_id, $cmnd , $client_host, $client_us +er, $svr_host, $svr_host ); return $DBI::errstr ? 0 : 1; } # Create File::Tail object my $file = File::Tail->new("/opt/app/superspy/tmp/commLogger.log"); # Create DB connection my $dbh = DBI->connect("dbi:mysql:database=P0TA6E01;host=myhost02.fcc. +hide.com:3306;user=xxxxxxxx;password=xxxxxxxx") || die "Couldn't conn +ect to database: $DBI::errstr\n"; # dispatch table and prepared statenents for the logfile entries # this whill be faster than preparing the query for each entry my %dispatch = ( AlertCmd => [ \&insert_alerts, $dbh->prepare('INSERT INTO AL +ERTS (TSTAMP,CMND_ID,CMND,CLIENTHOST,CLIENTUSER,SVR_HOST) VALUES (?,? +,?,?,?,?);' ], SendFiltered => [ \&insert_filters, $dbh->prepare('INSERT INTO FI +LTERS (TSTAMP,CMND_ID,CMND,CLIENTHOST,CLIENTUSER,SVR_HOST) VALUES (?, +?,?,?,?,?);' ], SendOffDuty => [ \&insert_filters, $dbh->prepare('INSERT INTO FI +LTERS (TSTAMP,CMND_ID,CMND,CLIENTHOST,CLIENTUSER,SVR_HOST) VALUES (?, +?,?,?,?,?);' ], SendStarted => [ \&insert_sends, $dbh->prepare('INSERT INTO SE +NDS (START_TIME,CMND_ID,ALERT_ID,SEND_ID,USER,DESTINATION,ADDRESS,MES +SAGE,STATE,SVR_HOST,ACTIVE) VALUES (?,?,?,?,?,?,?,?,?,?,?)' ], SendCompleted => [ \&update_sends, $dbh->prepare('UPDATE SENDS S +ET END_TIME=?, STATE=?,ACTIVE=FALSE WHERE CMND_ID=? AND ALERT_ID=? AN +D SEND_ID=? AND SVR_HOST=?;' ], SendCleared => [ \&update_sends, $dbh->prepare('UPDATE SENDS S +ET END_TIME=?, STATE=?,ACTIVE=FALSE WHERE CMND_ID=? AND ALERT_ID=? AN +D SEND_ID=? AND SVR_HOST=?;' ], AlertError => [ \&insert_errors, $dbh->prepare('INSERT INTO ER +RORS (TSTAMP,CMND_ID,ALERT_ID,SEND_ID,DESTINATION,MESSAGE,STATE,SVR_H +OST) VALUES (?,?,?,?,?,?,?,?)' ], ); # read linewise while (my $line = $file->read)) { chomp($line); # skipt reload next if $line =~ /^AlertCmd~.*~.*~-reload.*/; # check for interesting entries, save keyword and the rest in $1 a +nd $2 if ( $line =~ /^(AlertCmd|SendFiltered|SendOffDuty|SendStarted|Sen +dCompleted|SendCleared|AlertError)~(.+)/ ) { # the callback and the prepared statement from the dispatch ta +ble that are appropriate for the entry my ($function, $sth) = @{ $dispatch->{$1} }; # call the function and show error if something went wrong unless ( $function->( $sth, split /~/, $2 ) ) { print "Error ($DBI::errstr) while processing this line:\n$ +line\n\n"; } } }


    holli, /regexed monk/
      Hey Holli,
      A picture is worth a thousand words...thanks for the refactor it explains the use of these concepts and I follwed all parts of the code. I will definately use these concepts in the future...
      Many Thanks to Everyone,
      Danny