Re: [Maria-developers] 5265f7001c6: MDEV-17124: mariadb 10.1.34, views and prepared statements: ERROR 1615 (HY000): Prepared statement needs to be re-prepared
The check of triggers (if present) need only if definition did not changed.
Before the check need all that if would be just "return true".
Sergei Golubchik schrieb am Mi., 28. Sept. 2022, 19:15:
> Hi, Oleksandr,
>
> On Sep 28, Oleksandr Byelkin wrote:
> > > > +bool TABLE_LIST::is_the_same_definition(THD* thd, TABLE_SHARE *s)
> > > > +{
> > > > + enum enum_table_ref_type tp= s->get_table_ref_type();
> > > > + if (m_table_ref_type == tp)
> > > > + {
> > > > +bool res= m_table_ref_version == s->get_table_ref_version();
> > > > +
> > > > +/*
> > > > + If definition is different check content version
> > > > +*/
> > >
> > > if res == true, why do you need to check tabledef_version?
> > >
> >
> > You are right, fixed:
> > --- a/sql/table.cc
> > +++ b/sql/table.cc
> > @@ -8881,10 +8881,11 @@ bool TABLE_LIST::is_the_same_definition(THD* thd,
> > TABLE_SHARE *s)
> > /*
> >If definition is different check content version
> > */
> > -if (tabledef_version.length &&
> > -tabledef_version.length == s->tabledef_version.length &&
> > -memcmp(tabledef_version.str, s->tabledef_version.str,
> > - tabledef_version.length) == 0)
> > +if (res ||
> > +(tabledef_version.length &&
> > + tabledef_version.length == s->tabledef_version.length &&
> > + memcmp(tabledef_version.str, s->tabledef_version.str,
> > +tabledef_version.length) == 0))
>
> I still don't understand. I'd expect something like
>
>if (!res &&
>(tabledef_version.length && ...
>
> that is, if m_table_ref_version matches, then you don't need to do any
> further checks, the table is fine. If m_table_ref_version differs,
> meaning, the table was reopened, then you check tabledef_version and
> triggers.
>
> Perhaps the simplest fix would be to remove res and instead do
>
> if (m_table_ref_version == s->get_table_ref_version())
> return TRUE;
>
> if (tabledef_version.length && ...
>
> > {
> >if (table && table->triggers)
> >{
>
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and secur...@mariadb.org
>
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 5265f7001c6: MDEV-17124: mariadb 10.1.34, views and prepared statements: ERROR 1615 (HY000): Prepared statement needs to be re-prepared
Hi, Oleksandr,
On Sep 28, Oleksandr Byelkin wrote:
> > > +bool TABLE_LIST::is_the_same_definition(THD* thd, TABLE_SHARE *s)
> > > +{
> > > + enum enum_table_ref_type tp= s->get_table_ref_type();
> > > + if (m_table_ref_type == tp)
> > > + {
> > > +bool res= m_table_ref_version == s->get_table_ref_version();
> > > +
> > > +/*
> > > + If definition is different check content version
> > > +*/
> >
> > if res == true, why do you need to check tabledef_version?
> >
>
> You are right, fixed:
> --- a/sql/table.cc
> +++ b/sql/table.cc
> @@ -8881,10 +8881,11 @@ bool TABLE_LIST::is_the_same_definition(THD* thd,
> TABLE_SHARE *s)
> /*
>If definition is different check content version
> */
> -if (tabledef_version.length &&
> -tabledef_version.length == s->tabledef_version.length &&
> -memcmp(tabledef_version.str, s->tabledef_version.str,
> - tabledef_version.length) == 0)
> +if (res ||
> +(tabledef_version.length &&
> + tabledef_version.length == s->tabledef_version.length &&
> + memcmp(tabledef_version.str, s->tabledef_version.str,
> +tabledef_version.length) == 0))
I still don't understand. I'd expect something like
if (!res &&
(tabledef_version.length && ...
that is, if m_table_ref_version matches, then you don't need to do any
further checks, the table is fine. If m_table_ref_version differs,
meaning, the table was reopened, then you check tabledef_version and
triggers.
Perhaps the simplest fix would be to remove res and instead do
if (m_table_ref_version == s->get_table_ref_version())
return TRUE;
if (tabledef_version.length && ...
> {
>if (table && table->triggers)
>{
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 5265f7001c6: MDEV-17124: mariadb 10.1.34, views and prepared statements: ERROR 1615 (HY000): Prepared statement needs to be re-prepared
Hi, Sergei!
On Tue, Sep 27, 2022 at 2:36 PM Sergei Golubchik wrote:
> Hi, Oleksandr,
>
> Few minor comments, see below
>
> On Sep 27, Oleksandr Byelkin wrote:
> > revision-id: 5265f7001c6 (mariadb-10.3.36-60-g5265f7001c6)
> > parent(s): 86953d3df0a
> > author: Oleksandr Byelkin
> > committer: Oleksandr Byelkin
> > timestamp: 2022-09-27 10:23:31 +0200
> > message:
> >
> > MDEV-17124: mariadb 10.1.34, views and prepared statements: ERROR 1615
> (HY000): Prepared statement needs to be re-prepared
> >
> > The problem is that if table definition cache (TDC) is full of real
> tables
> > which are in tables cache, view definition can not stay there so will be
> > removed by its own underlying tables.
> > In situation above old mechanism of detection matching definition in PS
> > and current version always require reprepare and so prevent executing
> > the PS.
> >
> > One work around is to increase TDC, other - improve version check for
> > views/triggers (which is done here). Now in suspicious cases we check:
> > - timestamp (microseconds) of the view to be sure that version really
> >have changed;
> > - time (microseconds) of creation of a trigger related to time
> >(microseconds) of statement preparation.
>
> > diff --git a/sql/table.cc b/sql/table.cc
> > index 506195127b2..f289c2052cb 100644
> > --- a/sql/table.cc
> > +++ b/sql/table.cc
> > @@ -8861,6 +8861,63 @@ bool TABLE_LIST::is_with_table()
> >return derived && derived->with_element;
> > }
> >
> > +
> > +/**
> > + Check if the definition are the same.
> > +
> > + If versions do not match it check definitions (with checking and
> setting
> > + trigger definition versions (times)
> > +
> > + @sa check_and_update_table_version()
> > +*/
> > +
> > +bool TABLE_LIST::is_the_same_definition(THD* thd, TABLE_SHARE *s)
> > +{
> > + enum enum_table_ref_type tp= s->get_table_ref_type();
> > + if (m_table_ref_type == tp)
> > + {
> > +bool res= m_table_ref_version == s->get_table_ref_version();
> > +
> > +/*
> > + If definition is different check content version
> > +*/
>
> if res == true, why do you need to check tabledef_version?
>
You are right, fixed:
--- a/sql/table.cc
+++ b/sql/table.cc
@@ -8881,10 +8881,11 @@ bool TABLE_LIST::is_the_same_definition(THD* thd,
TABLE_SHARE *s)
/*
If definition is different check content version
*/
-if (tabledef_version.length &&
-tabledef_version.length == s->tabledef_version.length &&
-memcmp(tabledef_version.str, s->tabledef_version.str,
- tabledef_version.length) == 0)
+if (res ||
+(tabledef_version.length &&
+ tabledef_version.length == s->tabledef_version.length &&
+ memcmp(tabledef_version.str, s->tabledef_version.str,
+tabledef_version.length) == 0))
{
if (table && table->triggers)
{
>
> > +if (tabledef_version.length &&
> > +tabledef_version.length == s->tabledef_version.length &&
> > +memcmp(tabledef_version.str, s->tabledef_version.str,
> > + tabledef_version.length) == 0)
> > +{
> > + if (table && table->triggers)
> > + {
> > +my_hrtime_t hr_stmt_prepare= thd->hr_prepare_time;
> > +if (hr_stmt_prepare.val)
> > + for(uint i= 0; i < TRG_EVENT_MAX; i++)
> > +for (uint j= 0; j < TRG_ACTION_MAX; j++)
> > +{
> > + Trigger *tr=
> > +table->triggers->get_trigger((trg_event_type)i,
> > + (trg_action_time_type)j);
> > + if (tr)
> > +if (hr_stmt_prepare.val <= tr->hr_create_time.val)
> > +{
> > + set_tabledef_version(s);
> > + return FALSE;
> > +}
> > +}
> > + }
> > + set_table_id(s);
> > + return TRUE;
> > +}
> > +else
> > + tabledef_version.length= 0;
> > +return res;
> > + }
> > + else
> > +set_tabledef_version(s);
>
> why not set_table_id() ?
>
It is a case when type does not patch, and so type and version will be
assigned on reopening.
>
> > + return FALSE;
> > +}
> > +
> > +
> > uint TABLE_SHARE::actual_n_key_parts(THD *thd)
> > {
> >return use_ext_keys &&
> > diff --git a/sql/sql_trigger.h b/sql/sql_trigger.h
> > index ae3d1738b16..f7a2ccf2abc 100644
> > --- a/sql/sql_trigger.h
> > +++ b/sql/sql_trigger.h
> > @@ -198,7 +198,7 @@ class Table_triggers_list: public Sql_alloc
> >*/
> >List definition_modes_list;
> >/** Create times for triggers */
> > - List create_times;
> > + List hr_create_times;
>
> I suspect it might be UB. You use FILE_OPTIONS_ULLLIST for hr_create_times,
> perhaps it's safer to use List here.
>
Yes, probably it can be undefined behaviour, so I changed it to ulonglong.
>
> >
> >List definers_list;
> >
> > @@ -328,4 +328,14 @@ bool mysql_create_or_drop_trigger(THD *thd,
> TABLE_LIST *tables, bool create);
>
Re: [Maria-developers] 5265f7001c6: MDEV-17124: mariadb 10.1.34, views and prepared statements: ERROR 1615 (HY000): Prepared statement needs to be re-prepared
Hi, Oleksandr,
Few minor comments, see below
On Sep 27, Oleksandr Byelkin wrote:
> revision-id: 5265f7001c6 (mariadb-10.3.36-60-g5265f7001c6)
> parent(s): 86953d3df0a
> author: Oleksandr Byelkin
> committer: Oleksandr Byelkin
> timestamp: 2022-09-27 10:23:31 +0200
> message:
>
> MDEV-17124: mariadb 10.1.34, views and prepared statements: ERROR 1615
> (HY000): Prepared statement needs to be re-prepared
>
> The problem is that if table definition cache (TDC) is full of real tables
> which are in tables cache, view definition can not stay there so will be
> removed by its own underlying tables.
> In situation above old mechanism of detection matching definition in PS
> and current version always require reprepare and so prevent executing
> the PS.
>
> One work around is to increase TDC, other - improve version check for
> views/triggers (which is done here). Now in suspicious cases we check:
> - timestamp (microseconds) of the view to be sure that version really
>have changed;
> - time (microseconds) of creation of a trigger related to time
>(microseconds) of statement preparation.
> diff --git a/sql/table.cc b/sql/table.cc
> index 506195127b2..f289c2052cb 100644
> --- a/sql/table.cc
> +++ b/sql/table.cc
> @@ -8861,6 +8861,63 @@ bool TABLE_LIST::is_with_table()
>return derived && derived->with_element;
> }
>
> +
> +/**
> + Check if the definition are the same.
> +
> + If versions do not match it check definitions (with checking and setting
> + trigger definition versions (times)
> +
> + @sa check_and_update_table_version()
> +*/
> +
> +bool TABLE_LIST::is_the_same_definition(THD* thd, TABLE_SHARE *s)
> +{
> + enum enum_table_ref_type tp= s->get_table_ref_type();
> + if (m_table_ref_type == tp)
> + {
> +bool res= m_table_ref_version == s->get_table_ref_version();
> +
> +/*
> + If definition is different check content version
> +*/
if res == true, why do you need to check tabledef_version?
> +if (tabledef_version.length &&
> +tabledef_version.length == s->tabledef_version.length &&
> +memcmp(tabledef_version.str, s->tabledef_version.str,
> + tabledef_version.length) == 0)
> +{
> + if (table && table->triggers)
> + {
> +my_hrtime_t hr_stmt_prepare= thd->hr_prepare_time;
> +if (hr_stmt_prepare.val)
> + for(uint i= 0; i < TRG_EVENT_MAX; i++)
> +for (uint j= 0; j < TRG_ACTION_MAX; j++)
> +{
> + Trigger *tr=
> +table->triggers->get_trigger((trg_event_type)i,
> + (trg_action_time_type)j);
> + if (tr)
> +if (hr_stmt_prepare.val <= tr->hr_create_time.val)
> +{
> + set_tabledef_version(s);
> + return FALSE;
> +}
> +}
> + }
> + set_table_id(s);
> + return TRUE;
> +}
> +else
> + tabledef_version.length= 0;
> +return res;
> + }
> + else
> +set_tabledef_version(s);
why not set_table_id() ?
> + return FALSE;
> +}
> +
> +
> uint TABLE_SHARE::actual_n_key_parts(THD *thd)
> {
>return use_ext_keys &&
> diff --git a/sql/sql_trigger.h b/sql/sql_trigger.h
> index ae3d1738b16..f7a2ccf2abc 100644
> --- a/sql/sql_trigger.h
> +++ b/sql/sql_trigger.h
> @@ -198,7 +198,7 @@ class Table_triggers_list: public Sql_alloc
>*/
>List definition_modes_list;
>/** Create times for triggers */
> - List create_times;
> + List hr_create_times;
I suspect it might be UB. You use FILE_OPTIONS_ULLLIST for hr_create_times,
perhaps it's safer to use List here.
>
>List definers_list;
>
> @@ -328,4 +328,14 @@ bool mysql_create_or_drop_trigger(THD *thd, TABLE_LIST
> *tables, bool create);
> extern const char * const TRG_EXT;
> extern const char * const TRN_EXT;
>
> +
> +/**
> + Make time compatible with MySQL 5.7 trigger time.
> +*/
> +
> +inline my_hrtime_t make_hr_time(my_time_t time, ulong time_sec_part)
> +{
> + return my_hrtime_t({((ulonglong) time)*100 + time_sec_part});
> +}
better put it next to hrtime_to_time/etc
and, please, always 'static inline'
> +
> #endif /* SQL_TRIGGER_INCLUDED */
> diff --git a/sql/sql_view.cc b/sql/sql_view.cc
> index 17dea643144..177d01a312e 100644
> --- a/sql/sql_view.cc
> +++ b/sql/sql_view.cc
> @@ -1154,7 +1163,32 @@ static int mysql_register_view(THD *thd, TABLE_LIST
> *view,
>DBUG_RETURN(error);
> }
>
> +/**
> + Check is TABLE_LEST and SHARE match
> + @param[in] viewTABLE_LIST of the view
> + @param[in] share Share object of view
> +
> + @return false on error or misspatch
Eh. I don't understad this comment at all. First, I thought you made two
typos, s/is/if/ and s/misspatch/mismatch/. But this function doesn't check if
something matches, if gets the view version, as the name says, the name's
good. But the comment seems to be from is_the_same_definition()
> +*/
> +
