Re: Persistent Fault Tolerant SQL (MySQL + DBI) connections!

[afoken]

I literally had to look twice at the date of this posting. I had the strong feeling that I had stumbled over one of the ancient nodes created in the early stages of perlmonks.org, perhaps as a repost of much older code from the ages of Perl 4. But that posting is dated Dec 23, 2009.

So, what's wrong with that code?

• Perl 4 is dead. Don't beat that horse, it won't run any more. Prefixing a subroutine call with an ampersand does something very different in Perl 5 (disabling subroutine prototype checks) than it did in Perl 4 (just calling the function). And most times, you don't want that to happen. Drop that ampersand.
• Of course, your code explicitly states that row_sql, get_sql, and hash_sql DO NOT take any parameters - they have an empty prototype. Still, your code reads those parameters that should not exist. This works only when you explicitly disable prototype checks by prefixing the function name with an ampersand. Drop that empty prototypes.
• Perl 5 has modules. Don't put code into scripts loaded at runtime by do, use modules. This way, the namespace of the caller is not polluted. If you need to pollute the caller's namespace, use the Exporter or newer modules like Sub::Exporter. Preferably, export (pollute) only on demand.
• No strict, no warnings. Both would have complained about several errors in the code.
• No way to setup the database connection, especially database driver, username and password, without changing the code for each and every application that wants to include this feature. I.e. you have to make a copy of this code for each application. This ends in a maintainance nightmare.
• High risk for SQL injection, because placeholders are not supported.
• Despite the effords to enable caching, the cache is poisoned by not using placeholders. So, performance will be far less than optimal.
• The code is limited to only one DB connection, without any obvious reason. Had this been written as a class with attributes instead of tons of global variables, this limit would not exist.
• print and exit instead of die prevents any attempts of error recovery. And error messages end where they don't belong: STDOUT.
• eval blocks that hide errors, e.g. while fetching data rows from the database. Again, this prevents error recovery, simply because nobody can detect errors! Either don't use eval or re-throw the errors.
• No concept of how to indent code. Instead, a wild mix of tabs, spaces, and missing newlines. This makes the code hard to read, hard to understand, hard to maintain.
• A constantly increased index counter ($c) in get_sql(). Why don't you just use push @results,$row[0]?
• Reinventing the wheel in inefficient perl code. Had you read the DBI manual, you would have found that your get_sql is a poor reimplementation of DBI's selectcol_arrayref. Of course, your hash_sql function has a superiour DBI equivalent: selectall_hashref. And your row_sql is the dumb cousin of DBI's selectrow_array. All of those DBI methods are written in fast C/XS code (with pure-perl fallback routines).
• Your code is not safe for transactions. Reconnecting to the database ends an old transaction (does it rollback or commit?) and creates a new one, damaging data.
• What is the reason for del_sql, mod_sql, and put_sql? They are all just wrappers for sql. And instead of aborting with a useful error message when called with an empty SQL statement, they print a warning and CONTINUE calling sql. sql still aborts, but gives a wrong error message.
• What's the purpose of the various sleep(1) calls?
• What's the purpose of $retry_count? It is incremented quite often, but never compared to anything.
• Why do you emit "warnings" to STDOUT using print instead of using warn? warn allows your caller to handle warnings ($SIG{__WARN__}), and unless your caller traps them, they end where they belong: STDERR.
• DBIx::AutoReconnect implements the only "new" feature in your code, i.e. automatic reconnect to the database. But DBIx::AutoReconnect implements this feature in a way that is nearly completely transparent to the application. You just add use DBIx::AutoReconnect to your application, and change DBI->connect(...) to DBIx::AutoReconnect->connect(...), and every other feature of DBI behaves as usual. Of course, it works with every database, and with all connect parameters. It adds three additional connect attributes that allow fine-tuning. So, what is the advantage of your code over the nearly five year old DBIx::AutoReconnect that's available for free at CPAN?
• And finally: Use sql.pl as you wish, its [sic] now yours. This reads like: Take this crap and don't ever ask me for help. (OK, one could also read this as "I hereby put this code into public domain.")

I would recommend to use this sql.pl only as a example of how not to write Perl code. In clear words: DO NOT USE THIS CRAP!

Code like this is responsible for Perl's bad reputation. So: --

Alexander