maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Oleksandr,

ok to push
also, see below

On Mar 20, Oleksandr Byelkin wrote:
> revision-id: d8e915a88fc (mariadb-10.3.33-32-gd8e915a88fc)
> parent(s): 6a2d88c1322
> author: Oleksandr Byelkin
> committer: Oleksandr Byelkin
> timestamp: 2022-03-18 11:13:09 +0100
> message:
> 
> MDEV-26009 Server crash when calling twice procedure using FOR-loop
> 
> The problem was that instructions sp_instr_cursor_copy_struct and
> sp_instr_copen uses the same lex, adding and removing "tail" of
> prelocked tables and forgetting that tail of all tables is kept in
> LEX::query_tables_last. If the LEX used only by one instruction
> or the query do not have prelocked tables it is not important.
> But to work correctly in all cases LEX::query_tables_last should
> be reset to make new tables added in the correct list (after last
> table in the LEX instead after last table of the prelocking "tail"
> which was cut).
> 
> diff --git a/sql/sp_head.cc b/sql/sp_head.cc
> index 57ab31d9edf..96dc78dcf49 100644
> --- a/sql/sp_head.cc
> +++ b/sql/sp_head.cc
> @@ -3486,6 +3486,11 @@ sp_lex_keeper::reset_lex_and_exec_core(THD *thd, uint *nextp,
>      lex_query_tables_own_last= m_lex->query_tables_own_last;
>      prelocking_tables= *lex_query_tables_own_last;
>      *lex_query_tables_own_last= NULL;
> +    /*
> +      Here we cut "tail" of prelocked tables, so have to return last table
> +      to the original position (wuthout the tail
> +    */
> +    m_lex->query_tables_last= m_lex->query_tables_own_last;

1. the comment looks truncated
2. I don't even think this line needs a comment, this code block removes
the list of prelocked tables from the query_tables list. Looking at
how query_tables_last and query_tables_own_last are documented:

    /* Pointer to next_global member of last element in the previous list. */
    TABLE_LIST **query_tables_last;
    /*
      If non-0 then indicates that query requires prelocking and points to
      next_global member of last own element in query table list (i.e. last
      table which was not added to it as part of preparation to prelocking).
      0 - indicates that this query does not need prelocking.
    */
    TABLE_LIST **query_tables_own_last;

it is pretty clear that after the "tail" is removed, one has to set
query_tables_last to point to the end of the list. The fact that it
wasn't done was clearly an omission. That is, existing code and existing
comments document it pretty well, your comment looks redundant and even
confusing to me.

So, please, remove the comment and ok to push

>      m_lex->mark_as_requiring_prelocking(NULL);
>    }
>    thd->rollback_item_tree_changes();

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx