Percona Server moved to https://jira.percona.com/projects/PS

Code review comment for lp:~gl-az/percona-server/BT-23310-bug1218664-5.5

BT-23310-bug1218664-5.5
Merge into 5.5

Revision history for this message

George Ormond Lorch III (gl-az) wrote on 2013-09-27:

TABLE_CATEGORY_USER seems to refer to the mysql users table, not an end
user table.

On 9/27/2013 12:06 AM, George Ormond Lorch III wrote:
> On 9/26/2013 11:18 PM, Laurynas Biveinis wrote:
>> Review: Needs Fixing
>>
>> - It is suspicious that we want to include PERCONA_SCHEMA. Why?
>> Shouldn't it be treated as user table for our purposes?
>> Also wouldn't it provide a hole around schema enforcement if
>> the server is missing PERCONA_SCHEMA schema, thus user is free
>> to create it? Such scenario is exactly how the MTR testcase
>> currently works, isn't it?
>>
>> - Assuming PERCONA_SCHEMA is not desired, then there is no need
>> to maintain our own list of special schemas (also what about
>> case sensitivity?) There is get_table_category(), which will
>> return TABLE_CATEGORY_INFORMATION for I_S,
>> TABLE_CATEGORY_PERFROMANCE for P_S, TABLE_CATEGORY_SYSTEM for
>> mysql, TABLE_CATEGORY_LOG for slow/general logs if tables, and
>> TABLE_CATEGORY_USER. Thus it seems that we can simply call
>> get_table_category() and enforce storage engine only if it
>> returned TABLE_CATEGORY_USER. Note that it would cover log
>> tables as well, which we haven't discussed previously, but for
>> which SE enforcement is probably a bug too.
>>
>> - Assuming PERCONA_SCHEMA is not desired, an MTR testcase becomes
>> more complicated. I'd look if it is possible to test through
>> the log tables, if possible.
>>
> I was entirely unaware of this get_table_category(). This is IMHO the
> correct way to do it and I even almost implemented such a call like
> 'get_schema_type' or something along those lines. I do think though that
> PERCONA_SCHEMA does need some protection but it is hard to say right now
> because it is so new and nobody yet seems to have any idea or opinion on
> it. I suppose we could always add it in later but by then it may be too
> late (closing the door after the horse is out of the barn). This is a
> sticky integration point between XB and PS. By not protecting it then
> yes, it does make creating a proper test case more difficult but as you
> say, we could try with the log tables...
>

TABLE_CATEGORY_USER seems to refer to the mysql users table, not an end 
user table.

On 9/27/2013 12:06 AM, George Ormond Lorch III wrote:
> On 9/26/2013 11:18 PM, Laurynas Biveinis wrote:
>> Review: Needs Fixing
>>
>>       - It is suspicious that we want to include PERCONA_SCHEMA.  Why?
>>         Shouldn't it be treated as user table for our purposes?
>>         Also wouldn't it provide a hole around schema enforcement if
>>         the server is missing PERCONA_SCHEMA schema, thus user is free
>>         to create it?  Such scenario is exactly how the MTR testcase
>>         currently works, isn't it?
>>
>>       - Assuming PERCONA_SCHEMA is not desired, then there is no need
>>         to maintain our own list of special schemas (also what about
>>         case sensitivity?)  There is get_table_category(), which will
>>         return TABLE_CATEGORY_INFORMATION for I_S,
>>         TABLE_CATEGORY_PERFROMANCE for P_S, TABLE_CATEGORY_SYSTEM for
>>         mysql, TABLE_CATEGORY_LOG for slow/general logs if tables, and
>>         TABLE_CATEGORY_USER.  Thus it seems that we can simply call
>>         get_table_category() and enforce storage engine only if it
>>         returned TABLE_CATEGORY_USER.  Note that it would cover log
>>         tables as well, which we haven't discussed previously, but for
>>         which SE enforcement is probably a bug too.
>>
>>       - Assuming PERCONA_SCHEMA is not desired, an MTR testcase becomes
>>         more complicated.  I'd look if it is possible to test through
>>         the log tables, if possible.
>>
> I was entirely unaware of this get_table_category(). This is IMHO the
> correct way to do it and I even almost implemented such a call like
> 'get_schema_type' or something along those lines. I do think though that
> PERCONA_SCHEMA does need some protection but it is hard to say right now
> because it is so new and nobody yet seems to have any idea or opinion on
> it. I suppose we could always add it in later but by then it may be too
> late (closing the door after the horse is out of the barn). This is a
> sticky integration point between XB and PS. By not protecting it then
> yes, it does make creating a proper test case more difficult but as you
> say, we could try with the log tables...
>

« Back to merge proposal