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

[Øystein Grøvlen]

Chuck,

Thanks for addressing my comments.  I think the new patch looks pretty 
good.  Some comments below.

Chuck Bell wrote:
> 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.

Test no longer fails for me either.
(I think it may be the tests more than the server that is sensitive 
here.  I am not sure the numbering that failed is really something that 
needs to be stable.)

...

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

I think I was not quite clear here.  When I said all the entries, I was 
thinking of  ... BEFORE and ... TO commands that delete all entries.  I 
think the BACKUP PURGE LOGS command is well tested by the tidying up 
that is done during the test.

> 
>> 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.

Too bad.  We have a hammer, but we can't get everything to look like a 
nail.  :-(

...

>> 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.

OK.  I trust you on that.

> ---> 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.

Fair enough.

> 
>> 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. 

OK.  Since time is important here, I agree.

> 
>> 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.

OK.  I think it would be a bit clearer if it said that this was an 
implementation description.

> 
>> 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. ;)

Rethinking my suggestions, I think the best would be to make the new 
flag independent of whether it was a log table or not.  That is, 
HA_EXTRA_ALLOW_DELETE and HA_EXTRA_DISALLOW_DELETE as general 
primitives, not just associated to log tables.  However, I will not push 
this any further.

-- 
Øystein