* marc.alff@stripped <marc.alff@stripped> [07/11/16 10:38]:
> ChangeSet@stripped, 2007-11-16 00:10:19-07:00, malff@stripped. +36
> -0
> Bug#12093 (SP not found on second PS execution if another thread drops other
> SP in between)
Hi Marc,
Please see my comments inline.
A general comment about prepared statements is that, if for
backward compatibility purposes only, they should not be
invalidated, but automatically reprepared.
If the new prepared statement produces a different metadata, then
and only then we should produce 'statement was invalidated' error.
But since we have discussed this in detail on the phone, I shall
not explore further.
I deliberately do not make comments about the coding style --
there are numerous deviations. I sincerely hope that they all will
be fixed without me having to point them out.
> +/**
> + This helper ensures that a given Stored Function or Stored Procedure
> + is present in the corresponding per session cache,
> + and loads the Stored Function/Procedure from storage into the cache,
> + if necessary.
> + @param [in] thd Current thread
> + @param [in] type Type of stored object to lookup/load
> + @param [in] name Name of the stored object
> + @param [out] sphp Stored object refreshed in the cache, if found
> + @return An error status
> + @retval 0 Success, sphp will contain the stored object fetched, if any
> +*/
> +
> +static int
> +sp_cache_load(THD *thd, int type, sp_name *name, sp_head **sphp)
> +{
<cut>
> + if (! sp)
> + {
> + switch ((ret= db_find_routine(thd, type, name, &sp)))
> + {
> + case SP_OK:
> + sp_cache_insert(cp, sp);
> +
> + if (thd->m_object_observer)
> + {
> + sp->make_object_version(& sp_version);
> +
> + if (thd->m_object_observer->notify(thd, sp_version))
Let us align the terminology here:
- a version of the object reflects its change over time.
I.e. for stored procedures we chose md5 to represent a version.
- a fully qualified name of the object reflects its place in the
universe. I.e. for stored procedures a fully qualified name is
db_name.sp_name
- object type defines object type
- an object identifier is something that identifies the object
fully, in time, in space and in type.
In other words, my expectation is that an object identifier is a
triage <type, fully qualiifed name, version>
If I understand the intent correctly, object observer should
have <object identifier> as the parameter of 'notify'.
<object identifier> should be member of the object, in this case,
please make it a member of class sp_head.
> + DBUG_RETURN(1);
> + }
> +
> + *sphp= sp;
> + ret= 0;
> + break;
> + case SP_KEY_NOT_FOUND:
> + if (thd->m_object_observer)
> + {
> + sp_head::make_object_null_version(& sp_version,
> + type,
> + name->m_db,
> + name->m_name);
> + if (thd->m_object_observer->notify(thd, sp_version))
> + DBUG_RETURN(1);
> + }
Good, OK to create an object identifier for NULL version of
the object on stack.
> +
> +
> +/**
> + Refresh all the routines used by a statement in the session cache.
> + Note that routines that can be found are reloaded,
> + while routines that can not be found are ignored.
> + @param [in] thd Current thread
> + @param [in] lex Statement semantic tree
> + @return An error status
> + @retval 0 Success
> +*/
> +int
> +sp_refresh_routines(THD *thd, LEX *lex)
> +{
> + Sroutine_hash_entry *start;
> + int ret= 0;
> +
> + DBUG_ENTER("sp_recache_routines");
> + start= (Sroutine_hash_entry *)lex->sroutines_list.first;
> +
> + for (Sroutine_hash_entry *rt= start; rt; rt= rt->next)
> + {
> + sp_name name(thd, rt->key.str, rt->key.length);
> + int type= rt->key.str[0];
> + sp_head *sp;
> +
> + ret= sp_cache_load(thd, type, &name, &sp);
> +
> + if (ret)
> + break;
> + }
> + DBUG_RETURN(ret);
> +}
OK, but please see below about when this function should be called
from.
A comment on preloading approach: at first I was opposed to
pre-loading since we can always load a stored routine on demand,
e.g. in Item_func_sp::fix_fields().
But then I realized that a statement abort due to a version
mismatch is not friendly towards non-transactional engines -- we
should try to compute as much as possible up-front.
> +int sp_head::make_object_version(Object_version * version)
> +{
> + int rc;
> +
> + switch(m_type)
> + {
> + case TYPE_ENUM_FUNCTION:
> + rc= version->m_oid.set_function(m_db, m_name);
> + break;
> + case TYPE_ENUM_PROCEDURE:
> + rc= version->m_oid.set_procedure(m_db, m_name);
> + break;
> + case TYPE_ENUM_TRIGGER:
> + rc= version->m_oid.set_trigger(m_db, m_name);
> + break;
> + default:
> + DBUG_ASSERT(false);
> + rc= 1;
> + }
> +
> + version->m_vid.set_long(m_modified);
> +
> + return rc;
> +}
> +
> +int sp_head::make_object_null_version(
> + Object_version * version,
> + int type,
> + const LEX_STRING& db,
> + const LEX_STRING& name)
> +{
> + int rc;
> +
> + switch(type)
> + {
> + case TYPE_ENUM_FUNCTION:
> + rc= version->m_oid.set_function(db, name);
> + break;
> + case TYPE_ENUM_PROCEDURE:
> + rc= version->m_oid.set_procedure(db, name);
> + break;
> + case TYPE_ENUM_TRIGGER:
> + rc= version->m_oid.set_trigger(db, name);
> + break;
> + default:
> + DBUG_ASSERT(false);
> + rc= 1;
> + }
> +
> + version->m_vid.set_null();
> +
> + return rc;
> +}
OK, no additional comments except the suppressed ones about
egregious violations of the coding style.
> +TABLE *open_table(THD *thd, TABLE_LIST *table_list, MEM_ROOT *mem_root,
> + bool *refresh, uint flags)
> +{
> + TABLE *table;
> +
> + table= open_table_impl(thd, table_list, mem_root, refresh, flags);
> +
> + if (thd->m_object_observer)
> + {
> + Object_version table_version;
> +
> + if (table)
> + {
> + table->s->make_object_version(& table_version);
> + if (thd->m_object_observer->notify(thd, table_version))
> + {
> + *refresh= FALSE;
> + return(NULL);
> + }
> + }
> + else
> + {
> + TABLE_SHARE::make_object_null_version(& table_version,
> + table_list->db,
> + table_list->table_name);
> + if (thd->m_object_observer->notify(thd, table_version))
> + return(NULL);
> + }
> + }
> + return table;
No comments, since it's on your plan to clean up.
Table identifier should be a member of TABLE_SHARE.
>
> +enum Object_type
> +{
> + NO_OBJECT_TYPE= 0,
> + OBJECT_TYPE_TABLE= 1,
> + OBJECT_TYPE_VIEW= 2,
> + OBJECT_TYPE_UDF= 3,
> + OBJECT_TYPE_PROCEDURE= 4,
> + OBJECT_TYPE_FUNCTION= 5,
> + OBJECT_TYPE_TRIGGER= 6,
> + OBJECT_TYPE_EVENT= 7
> +};
If your task is to produce an exhausitve list, it's more than that:
USER
TABLESPACE
PARTITION
And with help of PeterG:
ASSERTION
CHARACTER SET
COLLATION
LOG FILE GROUP
DOMAIN
ROLE
SEQUENCE
SERVER
TYPE
I think you should either remove all unused values or add
everything to the enumeration and mark unused members as such.
> +class Object_id
> +{
> +public:
> + Object_id();
> + Object_id(const Object_id& oid);
> + ~Object_id() {}
> +
> + bool is_equal(const Object_id& oid) const;
> +
> + Object_type get_type() const
> + { return m_type; }
> +
> + void reset_type(Object_type type);
> +
> + int set(const Object_id& oid);
> +
> + int set_table(const char* db, const char* name);
> +
> + int set_table(const LEX_STRING& db, const LEX_STRING& name)
> + {
> + m_type= OBJECT_TYPE_TABLE;
> + return set_key('T', db, name);
> + }
> +
> + int set_view(const char* db, const char* name);
> +
> + int set_view(const LEX_STRING& db, const LEX_STRING& name)
> + {
> + m_type= OBJECT_TYPE_VIEW;
> + return set_key('V', db, name);
> + }
> +
> + int set_procedure(const LEX_STRING& db, const LEX_STRING& name)
> + {
> + m_type= OBJECT_TYPE_PROCEDURE;
> + return set_key('P', db, name);
> + }
> +
> + int set_function(const LEX_STRING& db, const LEX_STRING& name)
> + {
> + m_type= OBJECT_TYPE_FUNCTION;
> + return set_key('F', db, name);
> + }
> +
> + int set_trigger(const LEX_STRING& db, const LEX_STRING& name)
> + {
> + m_type= OBJECT_TYPE_TRIGGER;
> + return set_key('G', db, name);
> + }
> +
> + uchar* get_hash_key(size_t *length) const
> + {
> + *length= m_key_length;
> + return (uchar*) & m_key[0];
> + }
> +
> + const char* get_printable_type() const;
> +
> + const char* get_printable_name() const;
> +
> +private:
> + int set_key(char kind, const LEX_STRING& db, const LEX_STRING& name);
> +
> +private:
> + Object_type m_type;
> + char m_key[MAX_OBJ_ID_KEY_LEN];
> + int m_key_length;
> +
> +private: // Not implemented, use set()
> + Object_id& operator = (const Object_id&);
> +};
Why do you need a separate class for this?
> +
> +/**
> + A Version_id identifies a version of a given object.
> + Different objects (TABLE, VIEW, FUNCTION, PROCEDURE, ...)
> + can use version ids of different types (MD5, timestampt, counter, ...),
> + so that a Version_id is an heterogeneous type.
> + Versions can be compared for equality, but don't define any order.
> +*/
> +
> +class Version_id
Good, Object_version in the terminology above.
> +{
> +public:
> + Version_id();
> + Version_id(const Version_id& vid);
> + ~Version_id();
> +
> + bool is_null() const;
> + bool is_equal(const Version_id& v) const;
> + bool is_null_or_equal(const Version_id& v) const;
> +
> + void set_null();
> + void set_long(longlong value);
> + void set_md5(const LEX_STRING& md5);
> +
> +private:
> + enum kind {
> + NULL_VERSION,
> + LONG_VERSION,
> + MD5_VERSION
> + };
> +
> + kind m_kind;
> + longlong m_long_value;
> + char m_md5_value[32];
> +
> +private: // Non implemented
> + Version_id& operator = (const Version_id&);
> +};
> +
> +
> +class Object_version
Good, Object_identifier in the terminology above.
> +{
> +public:
> + static void* operator new(size_t size, MEM_ROOT* mem_root) throw();
> + static void operator delete(void *ptr, MEM_ROOT* mem_root) throw();
> +
> + Object_version()
> + {}
> +
> + Object_version(const Object_id& oid, const Version_id& vid)
> + : m_oid(oid), m_vid(vid)
> + {}
> +
> + ~Object_version() {}
> +
> + Object_id m_oid;
> + Version_id m_vid;
> +
> +private: // Non implemented
> + Object_version(const Object_version&);
> + Object_version& operator = (const Object_version&);
> +};
> +
> +
> +class Object_version_collection
> +{
> +public:
> + Object_version_collection()
> + : m_mem_root(NULL)
> + {}
> +
> + ~Object_version_collection()
> + {}
> +
> + int init(MEM_ROOT *mem_root);
> + int add(const Object_id& oid, const Version_id & vid);
> + int find(const Object_id& oid, const Object_version ** over) const;
> +
> +private:
> + HASH m_hash;
> + MEM_ROOT *m_mem_root;
> +
> +private: // Non implemented
> + Object_version_collection(const Object_version_collection&);
> + Object_version_collection& operator=(const Object_version_collection&);
> +};
Object_identifier_collection
> +
> +/**
> + Interface class for observers, used to inspect objects referenced
> + by a statement.
> + An Object_observer can inspect existing objects,
> + as well as get notified that some objects used in a statement are missing.
> + Observers can cause the current operation to fail, by returning an error
> + from the notification methods.
> +*/
> +
> +class Object_observer
> +{
> +protected:
> + Object_observer() {}
> + virtual ~Object_observer() {}
> +
> +public:
> + /**
> + Notify that an object is used in a statement.
> + @param [in] thd Current thread
> + @param [in] over object version
> + @return An error status
> + @retval 0 Success
> + */
> + virtual int notify(THD *thd, const Object_version& over) = 0;
> +
> +private: // Non implemented
> + Object_observer(const Object_observer&);
> + Object_observer& operator = (const Object_observer&);
> +};
OK.
> +int recheck_all_views(THD *thd, TABLE_LIST *start)
> +{
> + Object_observer *observer;
> + TABLE_LIST *tables;
> + TABLE_LIST dummy;
> + int rc;
> + Object_version view_version;
> +
> + DBUG_ASSERT(thd->m_object_observer);
> +
> + observer= thd->m_object_observer;
> +
> + /*
> + FIXME: There is no equivalent of sroutines_list for views,
> + so we have to iterate in TABLE_LIST.
> + This is poor, since the same view might be expanded
> + in multiple places, and will cause multiple parsing
> + */
> + for (tables= start; tables; tables= tables->next_global)
> + {
> + if (tables->view)
> + {
> +#ifdef LATER
> + dummy.db= tables->db;
> + dummy.table_name= tables->table_name;
> +#endif
> +
> + /*
> + WARNING: [1] and [2] are due to the implementation of views,
> + where the view logical name is found:
> + - sometime in (db, table_name) if the view has not be processed,
> + - sometime in (view_db, view_name) if the view has been processed.
> + See the comments in mysql_make_view()
> + */
> +
> + /* [1] mysql_load_view_md5 uses (db, table_name) */
> + dummy.db= tables->view_db.str;
> + dummy.table_name= tables->view_name.str;
> + rc= mysql_load_view_md5(thd, &dummy);
Object identifier should be a member of the object.
You should not attempt to access view definition outside of
locking.
If notify() for views is done in open_table/open_tables,
> +
> + if (rc == 0)
> + {
> + /* [2] TABLE_LIST::make_object_version uses (view_db, view_name) */
> + LEX fake_lex;
> + dummy.view_db= tables->view_db;
> + dummy.view_name= tables->view_name;
> + dummy.view= & fake_lex;
> + rc= dummy.make_object_version(& view_version);
> + }
> + else
> + rc= TABLE_LIST::make_object_null_version(& view_version,
> + tables->db,
> + tables->table_name);
> +
> + if (rc != 0)
> + return 1;
> +
> + if (observer->notify(thd, view_version))
> + return 1;
> + }
> + }
> +
> + return 0;
> +}
I believe you should not need this, but rather call notify() at the
right place in open_table or open_tables.
Speaking of which, I wonder how your patch will work with
INFORMATION_SCHEMA tables. Temporary tables for these are created
on the fly, so have a different version each time.
But table metadata didn't change, so prepared statement should
continue to function.
Please add a test for that.
> +
> +int
> +Prepare_observer::notify(THD *thd, const Object_version& over)
> +{
> + switch(over.m_oid.get_type())
> + {
> + case OBJECT_TYPE_TABLE:
> + case OBJECT_TYPE_VIEW:
> + case OBJECT_TYPE_PROCEDURE:
> + case OBJECT_TYPE_FUNCTION:
> + return m_coll->add(over.m_oid, over.m_vid);
> + default:
> + return 0;
> + }
> +}
OK.
> +
> +bool
> +Execute_observer::is_safe_table_operation(LEX *lex) const
> +{
> + switch(lex->sql_command)
> + {
> + case SQLCOM_ALTER_TABLE:
> + case SQLCOM_REPAIR:
> + case SQLCOM_ANALYZE:
> + case SQLCOM_OPTIMIZE:
> + return TRUE;
> +
> + default:
> + return FALSE;
> + }
> +}
I don't understand this, please explain.
> +
> +int
> +Execute_observer::notify(THD *thd, const Object_version& over)
> +{
> + const Object_version *collected;
> + int rc;
> +
> + DBUG_ENTER("Execute_observer::notify()");
> +
> + switch(over.m_oid.get_type())
> + {
> + case OBJECT_TYPE_TABLE:
> + case OBJECT_TYPE_VIEW:
> + rc= m_coll->find(over.m_oid, & collected);
> + if (rc != 0)
> + goto err;
> +
> + if (collected)
> + {
> + if (! collected->m_vid.is_equal(over.m_vid))
> + {
> + if (! m_expected_table_change.is_equal(over.m_oid) &&
> + ! is_safe_table_operation(thd->lex))
> + {
> + my_error(ER_PS_INVALIDATED, MYF(0),
> + over.m_oid.get_printable_type(),
> + over.m_oid.get_printable_name());
> + rc= 1;
> + }
> + }
> + }
> + else
> + {
> + Object_id shadowing_oid(over.m_oid);
> + if (over.m_oid.get_type() == OBJECT_TYPE_TABLE)
> + shadowing_oid.reset_type(OBJECT_TYPE_VIEW);
> + else
> + shadowing_oid.reset_type(OBJECT_TYPE_TABLE);
> +
> + rc= m_coll->find(shadowing_oid, & collected);
> + if (rc != 0)
> + goto err;
> +
> + if (collected)
> + {
> + my_error(ER_PS_INVALIDATED, MYF(0),
> + shadowing_oid.get_printable_type(),
> + shadowing_oid.get_printable_name());
> + rc= 1;
> + }
> + else
> + {
> + /*
> + - SHOW COLUMNS FROM <table>
> + --> open the table <table>
> + - INSTALL PLUGIN ... SONAME ...
> + --> open the table mysql.plugin
> + - SHOW CREATE TRIGGER t1_bi
> + --> open the table t1 (that the trigger relates to)
> + */
> + /*
> + - SHOW COLUMNS FROM <view>
> + --> open the view <view>
> + */
> + if (! over.m_vid.is_null())
> + rc= m_coll->add(over.m_oid, over.m_vid);
> + }
> + }
> + break;
> + case OBJECT_TYPE_PROCEDURE:
> + case OBJECT_TYPE_FUNCTION:
> + rc= m_coll->find(over.m_oid, & collected);
> + if (rc != 0)
> + goto err;
> +
> + if (! collected || ! collected->m_vid.is_equal(over.m_vid))
> + {
> + if (! is_safe_sp_operation(thd->lex))
> + {
> + /*
> + For these objects, we do require an exact match:
> + - collected != NULL means the object name must have been seen
> + during prepare, and not be discovered during execute.
> + - Version_id::is_equal() means that:
> + - either the object did not exist, and still does not exist
> + - or the object existed and is unchanged.
> + */
> + my_error(ER_PS_INVALIDATED, MYF(0),
> + over.m_oid.get_printable_type(),
> + over.m_oid.get_printable_name());
> + rc= 1;
> + }
> + }
> + break;
> + case OBJECT_TYPE_TRIGGER:
> + case OBJECT_TYPE_UDF:
> + case OBJECT_TYPE_EVENT:
> + case NO_OBJECT_TYPE:
> + rc= 0;
> + break;
> + }
> +
> +err:
> + DBUG_RETURN(rc);
Would be nice to have an algorithm description
in a comment, not inline.
Not following all the details yet.
>
> + flags((uint) IS_IN_USE),
> + m_versions(),
> + m_expected_table_change(),
> + m_expected_view_change()
> {
> init_alloc_root(&main_mem_root, thd_arg->variables.query_alloc_block_size,
> thd_arg->variables.query_prealloc_size);
> *last_error= '\0';
> +
> + if (m_versions.init(& main_mem_root))
> + {
> + state= Query_arena::ERROR;
> + last_errno= ER_OUTOFMEMORY;
> + sprintf(last_error, ER(ER_OUTOFMEMORY), 0);
> + }
Why do you need to allocate memory here?
Not necessary to call my_error if out of memory.
> /*
> The only case where we should have items in the thd->free_list is
> after stmt->set_params_from_vars(), which may in some cases create
> @@ -2907,6 +3221,14 @@ bool Prepared_statement::prepare(const c
> if (error == 0)
> error= check_prepared_statement(this, name.str != 0);
>
> + if (error == 0)
> + error= sp_refresh_routines(thd, thd->lex);
Please move this to open_tables, the branch which is executed when
the prelocking list is pre-built, after all tables are opened.
This functionality is not specific to prepared statements -- it's
a coincidence that we invalidate all stored procedures and
functions at once, when it is not the case, one will have to
pre-load routines in a similar fashion individually for each stored
procedure statement.
>
> +void Prepared_statement::expect_table_change(const Object_id& oid)
> +{
> + DBUG_ASSERT(m_expected_table_change.get_type() == NO_OBJECT_TYPE);
> + m_expected_table_change.set(oid);
> +}
> +
> +void Prepared_statement::expect_view_change(const Object_id& oid)
> +{
> + DBUG_ASSERT(m_expected_view_change.get_type() == NO_OBJECT_TYPE);
> + m_expected_view_change.set(oid);
Hmm, why do you need a separate method for tables and views...
> +int mysql_load_view_md5(THD *thd, TABLE_LIST *view);
> +
This should be removed. View definition should never be accessed
outside of locking. If notification of views is done inside
open_table/open_tables, this is unnecessary.
These are all comments on the substance that I have.
Thank you for working on this.
--
-- Konstantin Osipov Software Developer, Moscow, Russia
-- MySQL AB, www.mysql.com The best DATABASE COMPANY in the GALAXY
