RE: bzr commit into mysql-6.0 branch (cbell:2717) Bug#33364

[Chuck Bell]

Oystein,

A new patch is ready for review.

http://lists.mysql.com/commits/57156

I have replied to your requests and suggestions below. I have a request as
well: Can you please cut the 'extra' bits from the patch when replying? It
would make for easier going if we eliminate the extra text.

Chuck 

> 1. With this patch, test sp-code fails for me on Solaris x86 

Problem was traced to placement of the new SQLCOM_ enum. It seems part of
the server (SHOW PROCEDURE) is sensitive to sql_lex.h modifications -- a
pitty really but nothing I can change. Test no longer fails.

> 2. Is it really necessary to require bin logging for this test to be
>     run? 

Not anymore. I removed extra --source statements.

> 3. For the same reason as above, I do not see why requiring InnoDb
>     support is necessary.

See #2.

> 4. The new test backup_logs_purge generates large amounts of output in
>     master.err due to turning on tracing.  Is it not possible to turn
>     on and off the debug flags without turning on tracing?  (I think
>     you can use SET SESSION debug="+d,..." and "-d,...")

Ok, I did as you suggest. But it makes me nervous as no other test uses that
option. We shall see if PB likes it, but it seems it's ok on Windows and
Ubuntu. ;)

> 5. Fix typos [1], [2], [3], [11]

Done.

> 6. I think we should have tests that also test commands that will
>     delete either none or all the entries in log tables.

Done. 

> 7. I think we need a test that verify that content of files are
>     actually purged.

We cannot. The MTR does not support this. I had this problem when I made the
backup_logs test.

> 8. scan_backup_log: I think the function name should reflect that log
>     entries are actually deleted, and not just scanned. 

Renamed method.
>      Also, the documentation does not reflect the actual implementation
since it
>     is not required that datetime_val=0 for entries to be deleted by
>     id. (In fact, this method is called with both backup_id and
>     datatime_val set, see [4]).  I guess the easiest is to change the
>     doc here.  

Corrected behaviour.

>     Documentation also seems to imply that it is possible to
>     use this method to delete by timestamp from process 
>     table, but I am not convinced that will work. 
>     (Since ET_OBH_FIELD_START_TIME is used to find time field).

Added note to clarify and assertion to punish the wicked.

> 9. scan_backup_log, error handling: The call to ha_rnd_end()
>     overwrites the result value from the original error.  Don't you
>     risk ignoring errors here?

Refactored.

> 10. Do we really a specific error message for datetime errors for this
>      command?  Please, consider whether there is a more generic message
>      that can be used for such issues.

No, we need a specific one because the generic ones, well, suck. They're not
descriptive. This one is.

> 11. Comment [9] seems to be misplaced.  Please, move or remove.

Fixed.

> 12. Please explain the check for backupdir [10].

Not needed. Removed. Good catch! This was left over from when I stripped out
the PURGE BACKUP IMAGES code.


---> Answering those suggestions where you had questions otherwise I
implemented as many as I could.


> SUGGESTIONS
> ===========
> 
> 1. I am not sure I agree that it should give errors to purge backup
>     logs to id/table if logging to table is not enabled.  I suggest
>     just ignoring the command since tables are empty anyway.

I disagree. As a DBA, I want to know if my delete fails and why --
especially if it applies to table output only and I haven't set my default
destination to include logging to tables.

> 2. scan_backup_log: I think this implementation is a bit confusing
>     since deletion by id and time is handled differently, one is
>     cascading into the progress table the other is not.  I think you
>     are trying to do too much with a single function, and I would
>     suggest you use separate methods for these purposes.  If you are
>     concerned about duplicating code, you could make a method for
>     sharing the table_list setup.  (However, duplicating code seems to
>     be the "name of the game" in log.cc.)

The code as it is now is a consolidation of a bunch of repetitive code. I
really don't want to go back and undo all of that. 

> 4. Log_to_csv_event_handler::purge_backup_logs_before_date: It is a
>     bit unclear whether the Note in doc header is an interface note or
>     an implementation spec.  From an interface point of view the
>     cascading part is of no interest, purge_backup_logs_before_id and
>     purge_backup_logs_before_date behaves the same way.  Both deletes
>     entries in history and progress table.  Hence, I do not understand
>     why one of them need a specific note.

Really, just a note to the developer to clarify that the cascade is
implemented. I see no need to remove it.

> 6. Please, consider adding an HA_EXTRA_DISALLOW_LOG_DELETE option too.
>     I think it would be easier to follow if this LOG_DELETE was
>     completly independent of MARK_AS_LOG_TABLE.  It seems strange to
>     mark a table as a log table several times, just in order 
> to get the
>     side effect of disallowing deletes.

I don't see a need for this as this would only add yet another extra enum.
Having the developer specifically set it back to a log table is better and
harder to screw up. ;)


---> and now the specifics questions...

...

>  > +ER_BACKUP_LOGS_DELETED
>  > +  eng "Backup log entries deleted: "
> 
> [8] Is the colon intentional?  Will something more be added by another
>      statement?

Yes, see sql_parse.cc @2217.