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

Laurynas Biveinis (laurynas-biveinis) wrote on 2013-09-30:

Oh, right. In that case we'd need combined solution: if get_table_category() == USER, then compare its db name against MYSQL_SCHEMA_NAME.

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

Oh, right. In that case we'd need combined solution: if get_table_category() == USER, then compare its db name against MYSQL_SCHEMA_NAME.

> 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