Bug #79676 for Queue-DBI: notes && errors in Queue::DBI::Admin

Sun Sep 16 13:42:29 2012 bond-spb [...] mail.ru - Ticket created

Subject:	notes && errors in Queue::DBI::Admin
Date:	Sun, 16 Sep 2012 21:43:14 +0400
To:	<bug-queue-dbi [...] rt.cpan.org>
From:	Сергей Бондаренко <bond-spb [...] mail.ru>

1. DB constraints/indexes naming convention I suggest next scheme: idx_<table_name>_<field_name> for indexes unq_<table_name>_<field_name> for unique indexes fk_<table_name>_<field_name> for foreign constraints see: line 181 line 228 line 229 2. Error in fk constraint definition (line 229) 2.1 non unique fk constraint name error when creating more when one queue table pairs (using suggested naming convention will solve this) 2.2 ...REFERENCES `queues`... - replace with real table name from 'queues_table_name' 3. create_tables 'sqlite' parameter (line 131) Are you really need this param? Using $dbh->{Driver}->{Name} or better $dbh->get_info() you can get current db name and croak something like "Only sqlite and mysql supported for now". And you can simply extend module for pgsql, mssql, etc in future 4. Don't forget to list supported databases in POD please. BR Sergey

Tue Sep 18 00:12:14 2012 AUBERTG [...] cpan.org - Correspondence added

Hi Sergey, All excellent points! Regarding (3), I'm already using this in Audit::DBI so it was an easy fix. I also fixed the order in which tables are dropped by create_tables(), since they need to be dropped in reverse creation order. I've released v2.2.0 with the changes. I've done some basic MySQL testing, but I'm going to see if I can rearchitect the tests to run both for SQLite and for MySQL at some point. In the meantime, please let me know what you think! Thank you as always for your great feedback, Guillaume On Sun Sep 16 13:42:29 2012, bond-spb@mail.ru wrote: Show quoted text

> 1. DB constraints/indexes naming convention > I suggest next scheme: > idx_<table_name>_<field_name> for indexes > unq_<table_name>_<field_name> for unique indexes > fk_<table_name>_<field_name> for foreign constraints > > see: > line 181 > line 228 > line 229 > > > 2. Error in fk constraint definition (line 229) > > 2.1 non unique fk constraint name error when creating more when one > queue table pairs (using suggested naming convention will solve this) > 2.2 ...REFERENCES `queues`... - replace with real table name from > 'queues_table_name' > > > 3. create_tables 'sqlite' parameter (line 131) > Are you really need this param? > Using $dbh->{Driver}->{Name} or better $dbh->get_info() you can get > current db name and croak something like "Only sqlite and mysql > supported for now". > And you can simply extend module for pgsql, mssql, etc in future > > > 4. Don't forget to list supported databases in POD please. > > > BR > Sergey

Tue Sep 18 00:12:14 2012 The RT System itself - Status changed from 'new' to 'open'

Sun Sep 23 12:01:16 2012 bond-spb [...] mail.ru - Correspondence added

Subject:	Re: [rt.cpan.org #79676] notes && errors in Queue::DBI::Admin
Date:	Sun, 23 Sep 2012 20:02:10 +0400
To:	<bug-Queue-DBI [...] rt.cpan.org>
From:	Сергей Бондаренко <bond-spb [...] mail.ru>

Hi Guillaume, Thanks for your module improvements and please look at new remarks: 1. You need to add croak with dbi error after any $database_handle->do in Admin.pm just like line 328 2. For completeness of the Admin interface I suggest two additional functions: drop_tables() and has_tables(). It will be convenient for install/uninstall scripts. What you think? 3. You forgot to describe return value in POD for purge() function (DBI.pm) BR Sergey Show quoted text

----- Original Message ----- From: "Guillaume B. Aubert via RT" <bug-Queue-DBI@rt.cpan.org> To: <bond-spb@mail.ru> Sent: Tuesday, September 18, 2012 8:12 AM Subject: [rt.cpan.org #79676] notes && errors in Queue::DBI::Admin

> <URL: https://rt.cpan.org/Ticket/Display.html?id=79676 > > > Hi Sergey, > > All excellent points! Regarding (3), I'm already using this in > Audit::DBI so it was an easy fix. > > I also fixed the order in which tables are dropped by create_tables(), > since they need to be dropped in reverse creation order. > > I've released v2.2.0 with the changes. I've done some basic MySQL > testing, but I'm going to see if I can rearchitect the tests to run both > for SQLite and for MySQL at some point. In the meantime, please let me > know what you think! > > Thank you as always for your great feedback, > > Guillaume > > On Sun Sep 16 13:42:29 2012, bond-spb@mail.ru wrote:

>> 1. DB constraints/indexes naming convention >> I suggest next scheme: >> idx_<table_name>_<field_name> for indexes >> unq_<table_name>_<field_name> for unique indexes >> fk_<table_name>_<field_name> for foreign constraints >> >> see: >> line 181 >> line 228 >> line 229 >> >> >> 2. Error in fk constraint definition (line 229) >> >> 2.1 non unique fk constraint name error when creating more when one >> queue table pairs (using suggested naming convention will solve this) >> 2.2 ...REFERENCES `queues`... - replace with real table name from >> 'queues_table_name' >> >> >> 3. create_tables 'sqlite' parameter (line 131) >> Are you really need this param? >> Using $dbh->{Driver}->{Name} or better $dbh->get_info() you can get >> current db name and croak something like "Only sqlite and mysql >> supported for now". >> And you can simply extend module for pgsql, mssql, etc in future >> >> >> 4. Don't forget to list supported databases in POD please. >> >> >> BR >> Sergey

> > > >

Sun Sep 23 22:57:29 2012 AUBERTG [...] cpan.org - Correspondence added

Hi Sergey, Show quoted text

> 1. You need to add croak with dbi error after any $database_handle->do in > Admin.pm just like line 328

Indeed! https://github.com/guillaumeaubert/Queue-DBI/commit/bf5e86ba2d7e0d692ead3749e0c54b02f4ef34cc Show quoted text

> 2. For completeness of the Admin interface I suggest two additional > functions: drop_tables() and has_tables(). It will be convenient for > install/uninstall scripts. What you think?

It's a great idea, and it should simplify the create_tables() function. I'll work on it sometime this week. Show quoted text

> 3. You forgot to describe return value in POD for purge() function

(DBI.pm) Good catch! https://github.com/guillaumeaubert/Queue-DBI/commit/8d36c7143304e978acb0fcab1b39f78d54d7e3e3 Would you like me to release a separate v2.2.2 for the fixes in (1) and (3)? Otherwise, I will bundle them into v2.3.0 with the new features in (2). Thank you as always! Best, Guillaume

Mon Sep 24 05:01:13 2012 bond-spb [...] mail.ru - Correspondence added

Subject:	Re: [rt.cpan.org #79676] notes && errors in Queue::DBI::Admin
Date:	Mon, 24 Sep 2012 13:02:09 +0400
To:	<bug-Queue-DBI [...] rt.cpan.org>
From:	Сергей Бондаренко <bond-spb [...] mail.ru>

Hi Guillaume, Show quoted text

> Would you like me to release a separate v2.2.2 for the fixes in (1) and > (3)? Otherwise, I will bundle them into v2.3.0 with the new features in > (2).

Do as you comfortably BR Sergey

Tue Sep 25 11:29:09 2012 AUBERTG [...] cpan.org - Correspondence added

Hi Sergey, Show quoted text

> > Would you like me to release a separate v2.2.2 for the fixes in (1) and > > (3)? Otherwise, I will bundle them into v2.3.0 with the new features in > > (2).

> > Do as you comfortably

Thank you, I just released v2.3.0 with the fixes and the features in (2). Please let me know what you think! Best, Guillaume

Fri Sep 28 17:10:04 2012 bond-spb [...] mail.ru - Correspondence added

Subject:	Re: [rt.cpan.org #79676] notes && errors in Queue::DBI::Admin
Date:	Sat, 29 Sep 2012 01:11:11 +0400
To:	<bug-Queue-DBI [...] rt.cpan.org>
From:	Сергей Бондаренко <bond-spb [...] mail.ru>

Hi Guillaume, Show quoted text

> Thank you, I just released v2.3.0 with the fixes and the features in > (2). Please let me know what you think!

1. Please add local $database_handle->{'RaiseError'} = 1; after lines 353, 374, Admin.pm otherwise connecting db with 'RaiseError'=0 will result wrong return of has_tables() function 2. Seems you need replace "select *" to "select queue_id, name" line358 "select *" to "select queue_element_id, queue_id, data, lock_time, requeue_count, created" line379 to ensure has_tables() the real queue tables, but not arbitrary existing tables BR Sergey

Sat Sep 29 19:13:27 2012 AUBERTG [...] cpan.org - Correspondence added

Hi Sergey, Show quoted text

> 1. Please add > local $database_handle->{'RaiseError'} = 1; > after lines 353, 374, Admin.pm > otherwise connecting db with 'RaiseError'=0 will result wrong return of > has_tables() function

Very good point - fixed in https://github.com/guillaumeaubert/Queue-DBI/commit/3abe2708339900ea0ccf393c29a97f89a69a1d61 Show quoted text

> 2. Seems you need replace > "select *" to "select queue_id, name" line358 > "select *" to "select queue_element_id, queue_id, data, lock_time, > requeue_count, created" line379 > to ensure has_tables() the real queue tables, but not arbitrary existing > tables

I've been wondering about this. My concern is that if has_tables() returns 0, people will expect to be able to call create_tables(). But create_tables() will fail if: a) one table exists but not the other; or b) the tables exists but the return of 0 was due to incorrect fields. Would it make sense to have the following logic for has_tables(): a) If both tables exist and have the right fields, return 1; b) If both tables don't exist, return 0; c) If one table exists but not the other, croak with 'Table "table_name" is missing'; d) If a table exists but its fields are incorrect, croak with 'Table "table_name" exists but is missing mandatory fields'. This would also allow me to remove the dual scalar/list return context in has_tables(), so that the only way to call has_tables would become: my $tables_exist = $queues_admin->has_tables(); What do you think? Guillaume

Sun Sep 30 06:18:01 2012 bond-spb [...] mail.ru - Correspondence added

Subject:	Re: [rt.cpan.org #79676] notes && errors in Queue::DBI::Admin
Date:	Sun, 30 Sep 2012 14:19:11 +0400
To:	<bug-Queue-DBI [...] rt.cpan.org>
From:	Сергей Бондаренко <bond-spb [...] mail.ru>

Hi Guillaume, Show quoted text

> Would it make sense to have the following logic for has_tables(): > a) If both tables exist and have the right fields, return 1; > b) If both tables don't exist, return 0; > c) If one table exists but not the other, croak with 'Table "table_name" > is missing'; > d) If a table exists but its fields are incorrect, croak with 'Table > "table_name" exists but is missing mandatory fields'. > > This would also allow me to remove the dual scalar/list return context > in has_tables(), so that the only way to call has_tables would become: > my $tables_exist = $queues_admin->has_tables();

Of course this best solution Sergey

Mon Oct 01 09:52:43 2012 AUBERTG [...] cpan.org - Correspondence added

Hi Sergey, Show quoted text

> Of course this best solution

I just uploaded v2.3.1, which has all the modifications we discussed. Let me know when you have a chance to look at it and please tell me what you think. Thank you again for all your feedback, I very appreciate it! Guillaume

Sun Oct 07 11:39:37 2012 bond-spb [...] mail.ru - Correspondence added

Subject:	Re: [rt.cpan.org #79676] notes && errors in Queue::DBI::Admin
Date:	Sun, 7 Oct 2012 19:40:58 +0400
To:	<bug-Queue-DBI [...] rt.cpan.org>
From:	Сергей Бондаренко <bond-spb [...] mail.ru>

Hi Guillaume, Show quoted text

> I just uploaded v2.3.1, which has all the modifications we discussed. > Let me know when you have a chance to look at it and please tell me what > you think.

I just finished testing Admin.pm. everything is ok, but one note: drop_tables() must check tables before dropping with has_tables() to prevent accidental deletion of arbitrary existing table. BR Sergey

Mon Oct 08 22:39:14 2012 AUBERTG [...] cpan.org - Correspondence added

Hi Sergey, Show quoted text

> I just finished testing Admin.pm. everything is ok, but one note: > drop_tables() must check tables before dropping with has_tables() to > prevent accidental deletion of arbitrary existing table.

Ah yes I see - checking for the fields will ensure that someone cannot just pass any random table name in Queue::DBI::Admin->new() and then call drop_tables() to delete them. It's a good idea for security, I will add it now. Guillaume

Wed Oct 10 01:17:57 2012 AUBERTG [...] cpan.org - Correspondence added

Hi Sergey, Show quoted text

> I just finished testing Admin.pm. everything is ok, but one note: > drop_tables() must check tables before dropping with has_tables() to > prevent accidental deletion of arbitrary existing table.

I just released v2.4.2 that includes this, please let me know if you see anything else. Thank you again for all your suggestions! Guillaume

Wed Oct 10 01:17:58 2012 AUBERTG [...] cpan.org - Status changed from 'open' to 'resolved'

Sun Oct 21 14:38:17 2012 bond-spb [...] mail.ru - Correspondence added

Subject:	Re: [rt.cpan.org #79676] notes && errors in Queue::DBI::Admin
Date:	Sun, 21 Oct 2012 22:40:01 +0400
To:	<bug-Queue-DBI [...] rt.cpan.org>
From:	Сергей Бондаренко <bond-spb [...] mail.ru>

Hi Guillaume, Show quoted text

> I just released v2.4.2 that includes this, please let me know if you see > anything else.

1. has_queue() method return 0 when queue table does not exists. I think it would be better to add has_table() and croak before to inform about problem. (Admin.pm line 205) 2. Croak with DBI error needed in Queue::DBI->new() (DBI.pm line 216). Otherwise using RaiseError=0 we get the wrong message when there will be any sql error. Thank you for your work! Sergey

Sun Oct 21 14:38:18 2012 The RT System itself - Status changed from 'resolved' to 'open'

Fri Oct 26 00:25:27 2012 AUBERTG [...] cpan.org - Correspondence added

Hi Sergey, Show quoted text

> 1. has_queue() method return 0 when queue table does not exists. I > think it > would be better to add has_table() and croak before to inform about > problem. > (Admin.pm line 205)

I'm a bit concerned that it adds the overhead of an extra query to a function that can be called often. What would you think of verifying only once per Queue::DBI::Admin object instead: https://github.com/guillaumeaubert/Queue-DBI/commit/eb75aa4c53cfaacb179aee52dc04fcea05fdbff4 https://github.com/guillaumeaubert/Queue-DBI/commit/88b68c2d41dbb09d8265b500d5f4dfd72bdb435f Show quoted text

> 2. Croak with DBI error needed in Queue::DBI->new() (DBI.pm line 216). > Otherwise using RaiseError=0 we get the wrong message when there will > be any sql error.

The problem here is that for DBI, selectrow_arrayref() returning undef can mean either "SQL error" or "no rows found". I'm thinking here that the easiest is to switch RaiseError locally each time there's a selectrow_arrayref() in Queue::DBI: my $data; { local $database_handle->{'RaiseError'} = 1; $data = $dbh->selectrow_arrayref( ... ); } The alternative is to test $dbh->errstr() after each SQL statement, but I'm not sure if it is a very reliable way to detect errors. What do you think? Guillaume

Tue Oct 30 14:30:34 2012 bond-spb [...] mail.ru - Correspondence added

Subject:	Re: [rt.cpan.org #79676] notes && errors in Queue::DBI::Admin
Date:	Tue, 30 Oct 2012 22:32:17 +0400
To:	<bug-Queue-DBI [...] rt.cpan.org>
From:	Сергей Бондаренко <bond-spb [...] mail.ru>

Hi Guillaume, Show quoted text

> What would you think of verifying > only once per Queue::DBI::Admin object instead: > > https://github.com/guillaumeaubert/Queue-DBI/commit/eb75aa4c53cfaacb179aee52dc04fcea05fdbff4 > https://github.com/guillaumeaubert/Queue-DBI/commit/88b68c2d41dbb09d8265b500d5f4dfd72bdb435f

ok Show quoted text

> I'm thinking here that the easiest is to switch RaiseError locally each > time there's a selectrow_arrayref() in Queue::DBI: > > my $data; > { > local $database_handle->{'RaiseError'} = 1; > $data = $dbh->selectrow_arrayref( ... ); > }

this good solution BR Sergey

Wed Oct 31 21:53:15 2012 AUBERTG [...] cpan.org - Correspondence added

Hi Sergey, Show quoted text

> > What would you think of verifying > > only once per Queue::DBI::Admin object instead: > > [...]

> > ok > >

> > I'm thinking here that the easiest is to switch RaiseError locally

> each

> > time there's a selectrow_arrayref() in Queue::DBI: > > [...]

> > this good solution

I just released v2.5.0 with those two improvements. Thank you so much for your feedback and review of the code, I really appreciate it. Please let me know if you notice anything else! Guillaume

Wed Oct 31 21:53:16 2012 AUBERTG [...] cpan.org - Status changed from 'open' to 'resolved'

Bug #79676 for Queue-DBI: notes && errors in Queue::DBI::Admin

Preferred bug tracker