Code review comment for lp:~laurynas-biveinis/percona-server/BT-16724-xtradb-bmp-requests-5.1

Hi Laurynas,

On Mon, 21 Jan 2013 07:32:21 -0000, Laurynas Biveinis wrote:
> Alexey -
>
> Thanks for your review.
>
>> - do we really want the ALL clause in FLUSH/RESET/PURGE? It is of
>> course nice when SQL is close to the natural language. But in this
>> case it just leads to confusion:
>> - Does FLUSH ALL ... BITMAPS mean I can flush only some bitmap
>> files?
>> - Why do other FLUSH statements not require the ALL clause? (i.e.
>> there's no
>> "FLUSH ALL HOSTS").
>> - it is even more confusing in case of PURGE ALL ... BITMAPS
>> BEFORE. ALL in SQL is usually the opposite to "some", but in
>> this case we want to purge some files.
>
> IIRC the way "ALL" is used in the parser which comes closest to this usage, especially in 5.5, is that "ALL" would be a wildcard for storage engines that support this request, i.e. "FLUSH INNODB CHANGED PAGE BITMAPS" invokes the InnoDB handler, and "FLUSH ALL ... " invokes all handlers. Of course, the former syntax is not implemented ATM, nor there are current plans to.
>
> Does it makes sense to keep "ALL" as a storage engine wildcard here?
>

That would again be inconsistent with "FLUSH TABLES" and "FLUSH ENGINE
LOGS".

And speaking from the realistic perspective, what other storage engines
could possibly have this support implemented in the next few years?

It's all about keeping consistency and avoiding syntax redundancy.
"CHANGED PAGE BITMAP" is also redundant, and introducing 2 new reserved
words for this specific syntax doesn't look good either. Following the
same logic, it should be "FLUSH DES KEY FILE", rather than "FLUSH
DES_KEY_FILE", or "FLUSH USER RESOURCES" (or "FLUSH ALL USER
RESOURCES"), rather than "FLUSH USER_RESOURCES".

I would implement "FLUSH CHANGED_PAGE_BITMAPS", which doesn't have any
of the shortcomings being discussed.