Hi, Alexander,
Phew, it was a big patch. Took me 20 days.
Please, see comments below.
On May 09, Alexander Barkov wrote:
> commit 39f1b4c93e7
> Author: Alexander Barkov <bar(a)mariadb.com>
> Date: Mon Apr 14 12:00:57 2025 +0400
>
> MDEV-36053 CURSOR declarations in PACKAGE BODY
>
> This change adds CURSOR declarations inside PACKAGE BODY.
>
> PL/SQL mode:
>
> SET sql_mode=ORACLE;
> CREATE PACKAGE BODY pkg AS
> CURSOR mc0 IS SELECT c0, c1 FROM t1;
> PROCEDURE p1 AS
> rec mc0%ROWTYPE;
> BEGIN
> OPEN mc0;
> FETCH mc0 INTO rec;
> CLOSE mc0
> END;
> END;
> /
>
> SQL/PSM mode:
>
> SET sql_mode=DEFAULT;
> CREATE PACKAGE BODY pkg
> mc0 CURSOR FOR SELECT c0, c1 FROM t1;
> PROCEDURE p1()
> BEGIN
> DECLARE rec ROW TYPE OF mc0;
> OPEN mc0;
> FETCH mc0 INTO rec;
> CLOSE mc0
> END;
> END;
> /
> PACKAGE BODY cursors like local cursors
> (declared inside a FUNCTION or a PROCEDURE) support:
> - OPEN/FETCH/CLOSE
> - FOR rec IN cur - an explicit cursor loop
> - Using a cursor row as an anchored variable data type:
> * DECLARE var cur%ROWTYPE; -- sql_mode=ORACLE
> * DECLARE var ROW TYPE OF cur; -- sql_mode=DEFAULT
>
> The patch details:
>
> - Changing various class members and function/method parameters which
> store a CURSOR run-time address from "uint" to sp_rcontext_addr.
> A few classes now derive from sp_rcontext_addr instead of having
> a "uint m_cursor;" member.
>
> This change uses the same idea with what we did for SP variables,
> when we implemented PACKAGE BODY variables a few years ago.
>
> - Fixing the grammar in sql_yacc.yy to allow CURSOR declarations
> inside PACKAGE BODY.
>
> - Moving the C++ code handing the "CLOSE_SYM ident" grammar
> and creating an sp_instr_cclose instance from sql_yacc.yy
> into a new method LEX::sp_close().
> This is to have the grammar file smaller.
> Also, this code has significantly changed anyway.
>
> - Adding a new class sp_pcontext_top. It's used for the top level parse
> context (of an sp_head). Note, its children contexts still use the old
> class sp_pcontext.
>
> sp_pcontext_top context additionally to sp_pcontext has:
>
> const sp_head *m_sp; -- The pointer to the sp_head owning this context
> Dynamic_array<sp_pcursor> m_member_cursors; -- PACKAGE BODY wide cursors
>
> m_sp->m_parent->get_parse_context() is used to find the sp_pcontext
> belonging to the parent PACKAGE BODY from a sp_pcontext_top instance
> belonging to a PROCEDURE/FUNCTION sp_pcontext_top.
>
> - Adding a new member in sp_rcontext:
> Dynamic_array<sp_cursor*> m_member_cursors;
> It's used to store run-time data of PACKAGE BODY wide cursors.
>
> - Adding a new class sp_instr_copen2. It's used to open PACKAGE BODY cursors.
> Unlike the usual cursors, PACKAGE BODY cursors:
> * do not use the cursor stack (sp_rcontext::m_cstack)
> * do not need a preceeding sp_instr_cpush
> * do not need a following sp_instr_cpop
>
> All cursor information such as "sp_lex_cursor" resides inside
> sp_instr_copen2 itself (rather than inside sp_instr_cpush which is
> used to store "sp_lex_cursor" in case of sp_instr_copen).
>
> Note, the other cursor related instructions:
> sp_instr_cfetch
> sp_instr_cclose
> sp_instr_cursor_copy_struct
> do not need sp_instr_xxx2 counter-parts.
> Thy just use sp_rcontext_addr to address cursors.
>
> - Adding Sp_rcontext_handler_member
> It's used to handle PACKAGE BODY members:
> cursors and variables declared in the PACKAGE BODY,
> when they are accessed from its executable initialization section:
>
> CREATE PACKAGE BODY pkg AS
> CURSOR mc0 IS SELECT c0, c1 FROM t1; -- A member (PACKAGE BODY cursor)
> mv0 mc0%ROWTYPE; -- A member (PACKAGE BODY variable)
> PROCEDURE p1 AS
> BEGIN
> -- Accessing members from here use sp_rcontext_handler_package_body
> -- (members of the parent PACKAGE BODY)
> OPEN mc0;
> FETCH mc0 INTO mv0;
> CLOSE mc0;
> END;
> BEGIN
> -- NEW:
> -- Accessing members from here use sp_rcontext_handler_member
> -- (PACKAGE BODY own members)
> OPEN mc0;
> FETCH mc0 INTO mv0;
> CLOSE mc0;
> END;
> /
>
> Member variables and cursor are now marked with the "MEMBER." prefix
> in the "SHOW PACKAGE BODY code" output.
> Some old MTR tests have been re-recorded accordingly.
>
> - Adding new virtual methods into Sp_rcontext_handler:
>
> virtual const sp_variable *get_pvariable(const sp_pcontext *pctx,
> uint offset) const;
>
> virtual const sp_pcursor *get_pcursor(const sp_pcontext *pctx,
> uint offset) const;
>
> They're used from sp_instr::print() virtual implementations.
> They internally calculate a proper sp_pcontext using as a parameter
> the sp_pcontext pointed by sp_instr::m_ctx.
>
> For example, Sp_handler_package_body::get_pvariable()/get_pcursor()
> accesses to this sp_pcontext:
> m_ctx->top_context()->m_sp->m_parent->get_parse_context(),
> i.e. the parse context of the PACKAGE BODY which is the parent for
> the current package PROCEDURE of FUNCTION an sp_instr belongs to.
>
> - Adding a new method LEX::find_cursor(). It searches for a cursor in
> this order:
> * Local cursors in the nearst upper BEGIN/END block.
> * A member cursor of the current PACKAGE BODY
> (used from the PACKAGE BODY initialization section)
> * A member cursor of the parrent PACKAGE BODY
> (used from a package PROCEDURE or a package FUNCTION)
better write here (and in the function comment below), like
1. Local cursors...
2. A member cursor of the current PACKAGE BODY
OR
2. A member cursor of the parrent PACKAGE BODY
to emphasize that second and third are alternatives
and it never search both in "current PACKAGE BODY"
and then in the "parrent PACKAGE BODY" in this order.
>
> Adding a new method LEX::find_cursor_with_error().
> In case when a cursor is not found, it automatically
> raises the ER_SP_CURSOR_MISMATCH SQL condition into
> the diagnostics area.
>
> - Adding a new method sp_head::add_instr_copenX().
> It creates sp_instr_copen for local cursors,
> or sp_instr_copen2 for non-local cursors.
>
> - Adding a new abstract class sp_lex_cursor_instr.
> It's used a common parent class for a few sp_instr_xxx classes,
> including the new sp_instr_copen2.
> This change is needed to avoid code duplication.
>
> - Adding a new protected method sp_instr::print_cmd_and_var(), to print
> an instruction using this format: "command name@offset".
> It's used from a few implementations of sp_instr_xxx::print(),
> including sp_instr_copen2::print().
> This change is also needed to avoid code duplication.
>
> - Moving the definition of "class Sp_rcontext_handler" from item.h
> into a new file sp_rcontext_handler.h
> This is to maitain header dependencies easier, as well as to move
> declarations not directly related to "class Item" outside of item.h
>
> - Adding a new method sp_pcontext::frame_for_members(), to distinguish
> easier between local cursors/variables and PACKAGE BODY cursors/variables.
>
> - Fixing "struct Lex_for_loop_st" to addionally store
> a const pointer to Sp_rcontext_handler, to distinguish between:
> * FOR rec IN local_cursor
> * FOR rec IN package_body_cursor
>
> diff --git a/mysql-test/include/sp/sp-cursor-package-body-init.inc b/mysql-test/include/sp/sp-cursor-package-body-init.inc
> --- /dev/null
> +++ b/mysql-test/include/sp/sp-cursor-package-body-init.inc
> @@ -0,0 +1,4 @@
> +--disable_query_log
> +--let $oracle=`SELECT @@sql_mode LIKE '%ORACLE%'`
> +--let $code=`SELECT COALESCE(@code, 0)`
well, sure, but wouldn't it be simpler to do
let $code=1;
in sp-cursor-package-body-code.test ?
> +--enable_query_log
> diff --git a/mysql-test/include/sp/sp-cursor-package-body-06.inc b/mysql-test/include/sp/sp-cursor-package-body-06.inc
> --- /dev/null
> +++ b/mysql-test/include/sp/sp-cursor-package-body-06.inc
> @@ -0,0 +1,87 @@
> +--echo #
> +--echo # PACKAGE BODY initialization:
> +--echo # OPEN package_body_cursor
> +--echo # FUNCTION:
> +--echo # FETCH package_body_cursor INTO package_body_variable
> +--echo # PROCEDURE:
> +--echo # CLOSE package_body_cursor
> +--echo #
> +
> +--source sp-cursor-package-body-init.inc
> +
> +if ($oracle == 0)
> +{
> +
> +DELIMITER /;
> +CREATE PACKAGE pkg
> + FUNCTION f1() RETURNS INT;
> + PROCEDURE p1close();
> +END;
> +/
> +CREATE PACKAGE BODY pkg
> + DECLARE cur CURSOR FOR SELECT 1 AS c FROM DUAL;
> + DECLARE vc0 INT;
> + FUNCTION f1() RETURNS INT
> + BEGIN
> + FETCH cur INTO vc0;
> + RETURN vc0;
> + END;
> + PROCEDURE p1close()
> + BEGIN
> + CLOSE cur;
> + END;
> + -- initialization
> + OPEN cur;
> +END;
cool. that's what I was asking for :)
> +/
> +DELIMITER ;/
> +
> +}
> +
> +
> +if ($oracle > 0)
> +{
> +
> +DELIMITER /;
> +CREATE PACKAGE pkg AS
> + FUNCTION f1 RETURN INT;
> + PROCEDURE p1close;
> +END;
> +/
> +CREATE PACKAGE BODY pkg AS
> + CURSOR cur IS SELECT 1 AS c FROM DUAL;
> + vc0 INT;
> + FUNCTION f1 RETURN INT AS
> + BEGIN
> + FETCH cur INTO vc0;
> + RETURN vc0;
> + END;
> + PROCEDURE p1close AS
> + BEGIN
> + CLOSE cur;
> + END;
> +BEGIN
> + OPEN cur;
> +END;
> +/
> +DELIMITER ;/
> +
> +}
> +
> +
> +--disable_view_protocol
> +--disable_ps2_protocol
> +SELECT pkg.f1() FROM DUAL;
> +--enable_ps2_protocol
> +CALL pkg.p1close();
> +--error ER_SP_CURSOR_NOT_OPEN
> +SELECT pkg.f1() FROM DUAL;
> +--enable_view_protocol
> +
> +if ($code > 0)
> +{
> +SHOW PACKAGE BODY CODE pkg;
> +SHOW FUNCTION CODE pkg.f1;
> +SHOW PROCEDURE CODE pkg.p1close;
> +}
> +DROP PACKAGE pkg;
> diff --git a/sql/sp_rcontext_handler.h b/sql/sp_rcontext_handler.h
> --- /dev/null
> +++ b/sql/sp_rcontext_handler.h
> @@ -0,0 +1,141 @@
> +#ifndef SP_RCONTEXT_HANDLER_INCLUDED
> +#define SP_RCONTEXT_HANDLER_INCLUDED
> +
> +/* Copyright (c) 2009, 2025, MariaDB Corporation.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; version 2 of the License.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program; if not, write to the Free Software
> + Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335 USA */
> +
> +
> +class sp_rcontext;
> +class sp_pcontext;
> +class sp_pcursor;
> +class sp_cursor;
> +
> +/**
> + A helper class to handle the run time context of various components of SP:
> + Variables:
> + - local SP variables and SP parameters
> + - PACKAGE BODY routine variables
> + - (there will be more kinds in the future)
> + Cursors:
> + - static local cursors
> + - static PACKAGE BODY cursors of the parent PACKAGE BODY
> + - static PACKAGE BODY own cursors (when used in the executable secion)
> +*/
> +
> +class Sp_rcontext_handler
> +{
> +public:
> + virtual ~Sp_rcontext_handler() = default;
> +
> +
> + /**
> + A prefix used for SP variable names in queries:
> + - EXPLAIN EXTENDED
> + - SHOW PROCEDURE CODE
> + Local variables and SP parameters have empty prefixes.
> + Package body variables are marked with a special prefix.
> + This improves readability of the output of these queries,
> + especially when a local variable or a parameter has the same
> + name with a package body variable.
> + */
> + virtual const LEX_CSTRING *get_name_prefix() const= 0;
> +
> + // Find a parse time SP variable
> + virtual const sp_variable *get_pvariable(const sp_pcontext *pctx,
> + uint offset) const= 0;
> +
> + // Find a parse time SP cursor
> + virtual const sp_pcursor *get_pcursor(const sp_pcontext *pctx,
> + uint offset) const= 0;
> +
> + /**
> + At execution time THD->spcont points to the run-time context (sp_rcontext)
> + of the currently executed routine.
> + Local variables store their data in the sp_rcontext pointed by thd->spcont.
> + Package body variables store data in separate sp_rcontext that belongs
> + to the package.
> + This method provides access to the proper sp_rcontext structure,
> + depending on the SP variable kind.
> + */
> + virtual sp_rcontext *get_rcontext(sp_rcontext *ctx) const= 0;
> +
> + // Find a run time SP cursor
> + virtual sp_cursor *get_cursor(THD *thd, uint offset) const= 0;
> +};
> +
> +
> +/*
> + A handler to access local variables and cursors.
> +*/
> +class Sp_rcontext_handler_local final: public Sp_rcontext_handler
> +{
> +public:
> + const LEX_CSTRING *get_name_prefix() const override;
> + const sp_variable *get_pvariable(const sp_pcontext *pctx,
> + uint offset) const override;
> + const sp_pcursor *get_pcursor(const sp_pcontext *pctx,
> + uint offset) const override;
> + sp_rcontext *get_rcontext(sp_rcontext *ctx) const override;
> + sp_cursor *get_cursor(THD *thd, uint offset) const override;
> +};
> +
> +
> +/*
> + A handler to access parent members, e.g.:
> + PACKAGE BODY variables and cursors when used in package routines.
> +*/
> +class Sp_rcontext_handler_package_body final :public Sp_rcontext_handler
> +{
> +public:
> + const LEX_CSTRING *get_name_prefix() const override;
> + const sp_variable *get_pvariable(const sp_pcontext *pctx,
> + uint offset) const override;
> + const sp_pcursor *get_pcursor(const sp_pcontext *pctx,
> + uint offset) const override;
> + sp_rcontext *get_rcontext(sp_rcontext *ctx) const override;
> + sp_cursor *get_cursor(THD *thd, uint offset) const override;
> +};
> +
> +
> +/*
> + A handler to access its own members, e.g.:
> + PACKAGE BODY variables and cursors when used in
> + the initialization section of PACKAGE BODY.
> +*/
> +class Sp_rcontext_handler_member final :public Sp_rcontext_handler
> +{
> +public:
> + const LEX_CSTRING *get_name_prefix() const override;
> + const sp_variable *get_pvariable(const sp_pcontext *pctx,
> + uint offset) const override;
> + const sp_pcursor *get_pcursor(const sp_pcontext *pctx,
> + uint offset) const override;
> + sp_rcontext *get_rcontext(sp_rcontext *ctx) const override;
> + sp_cursor *get_cursor(THD *thd, uint offset) const override;
> +};
> +
> +
> +extern MYSQL_PLUGIN_IMPORT
> + Sp_rcontext_handler_local sp_rcontext_handler_local;
why is it MYSQL_PLUGIN_IMPORT? I know that it was and you've just copied it,
but still? Plugins aren't supposed to use it, are they?
Same below.
> +
> +
> +extern MYSQL_PLUGIN_IMPORT
> + Sp_rcontext_handler_package_body sp_rcontext_handler_package_body;
> +
> +
> +extern MYSQL_PLUGIN_IMPORT
> + Sp_rcontext_handler_member sp_rcontext_handler_member;
> +
> +#endif // SP_RCONTEXT_HANDLER_INCLUDED
> diff --git a/sql/sp_head.h b/sql/sp_head.h
> --- a/sql/sp_head.h
> +++ b/sql/sp_head.h
> @@ -222,6 +223,7 @@ class sp_head :private Query_arena,
> LEX_CSTRING m_defstr;
> AUTHID m_definer;
>
> + Query_arena *query_arena() { return this; }
this is practically equivalent to inheriting from Query_arena publicly.
Why not to do it instead?
> const st_sp_chistics &chistics() const { return m_chistics; }
> const LEX_CSTRING &comment() const { return m_chistics.comment; }
> void set_suid(enum_sp_suid_behaviour suid) { m_chistics.suid= suid; }
> diff --git a/sql/sp_pcontext.h b/sql/sp_pcontext.h
> --- a/sql/sp_pcontext.h
> +++ b/sql/sp_pcontext.h
> @@ -319,6 +320,10 @@ class sp_pcursor: public LEX_CSTRING
> class sp_pcontext *param_context() const { return m_param_context; }
> class sp_lex_cursor *lex() const { return m_lex; }
> bool check_param_count_with_error(uint param_count) const;
> + bool eq_name(const LEX_CSTRING &name) const
Why name isn't Lex_ident_column?
> + {
> + return !system_charset_info->strnncoll(name.str, name.length, str, length);
> + }
> };
>
>
> @@ -826,4 +861,105 @@ class sp_pcontext : public Sql_alloc
> }; // class sp_pcontext : public Sql_alloc
>
>
would be good to have a comment here.
and why was it not needed for package-wide variables?
> +class sp_pcontext_top: public sp_pcontext
> +{
> +public:
> + sp_pcontext_top(const sp_head *sp);
> +
> + const sp_head *sp() const
> + {
> + return m_sp;
> + }
> +
> + bool add_member_cursor(const LEX_CSTRING *name, sp_pcontext *param_ctx,
> + class sp_lex_cursor *lex)
> + {
> + uint dummy_offset;
> + if (find_member_cursor(name, &dummy_offset))
> + {
> + my_error(ER_SP_DUP_CURS, MYF(0), name->str);
> + return true;
> + }
> + return m_member_cursors.append(sp_pcursor(name, param_ctx, lex));
> + }
> +
> + const sp_pcursor *find_member_cursor(const LEX_CSTRING *name,
> + uint *poff) const
> + {
> + for (uint i= 0; i < m_member_cursors.elements(); i++)
> + {
> + if (m_member_cursors.at(i).eq_name(*name))
> + {
> + *poff= i;
> + return &m_member_cursors.at(i);
> + }
> + }
> + return nullptr;
> + }
> +
> + const sp_pcursor *find_member_cursor(uint offset) const
> + {
> + return &m_member_cursors.at(offset);
> + }
> +
> + uint member_cursor_count() const
> + {
> + return (uint) m_member_cursors.elements();
> + }
> +
> + /*
> + Return a possible parse context frame for members.
> +
> + Members are:
> + - CREATE PACKAGE wide variables and cursors (coming soon - MDEV-13139)
> + - CREATE PACKAGE BODY wide variables and cursors
> +
> + Example:
"Example (using Oracle syntax):"
> +
> + CREATE PACKAGE BODY pkg AS -- sp_package
> + a0 INT; -- A member
> + CURSOR c0; -- A member
> +
> + PROCEDURE p1() AS -- A method sp_head (package routine)
> + a1 INT; -- A local variable - not an sp_package member
> + CURSOR c1; -- A local cursor - not an sp_package member
> + BEGIN
> + OPEN c0; -- Open a cursor member of the parent sp_package
> + CLOSE c0; -- Close a cursor member of the parent sp_package
> + END;
> +
> + BEGIN -- Executable initialization section (constructor)
> + DECLARE
> + a2 INT; -- a local variable - not a member
> + CURSOR c2; -- a local cursor - not a member
> + BEGIN
> + OPEN c0; -- Open its own cursor member (of sp_package)
> + CLOSE c0; -- Close its own cursor member (of sp_package)
> + END;
> + END;
> +
> + Members are visible in all subroutines and are treated in a different way
> + comparing to local variables/cursors. For example, member cursors
> + do not need sp_instr_cpush and sp_instr_cpop, unlike local cursors.
> +
> + The top level parse context frame of an sp_head stores formal parameters:
> + - PROCEDURE and FUNCTION can have formal parameters.
> + - PACKAGE and PACKAGE BODY cannot have formal parameters,
> + but still the top level context frame exists (it's just empty).
> + Members reside in child(0) of the top level parse context.
> +
> + The owning sp_head will check if this frame is really for members,
> + depending on the sp_head type (a package vs a routine).
> + */
> + sp_pcontext *frame_for_members_candidate() const
> + {
> + return child_context(0);
> + }
> +
> +private:
> + const sp_head *m_sp;
> + Dynamic_array<sp_pcursor> m_member_cursors;
> +};
> +
> +
> #endif /* _SP_PCONTEXT_H_ */
> diff --git a/sql/sp_head.cc b/sql/sp_head.cc
> --- a/sql/sp_head.cc
> +++ b/sql/sp_head.cc
> @@ -3818,7 +3821,26 @@ sp_head::set_local_variable_row_field_by_name(THD *thd, sp_pcontext *spcont,
> }
>
>
> +bool sp_head::add_instr_copenX(THD *thd, sp_pcontext *spcont,
> + const sp_rcontext_addr &addr)
> +{
> + sp_instr *i;
> + if (addr.rcontext_handler() == &sp_rcontext_handler_local)
> + i= new (thd->mem_root) sp_instr_copen(instructions(), spcont,
> + addr.offset());
> + else
> + {
> + const sp_pcursor *pcursor= addr.rcontext_handler()->
> + get_pcursor(spcont, addr.offset());
or spcont->get_pcursor(addr)
that's why you've added it, after all :)
> + i= new (thd->mem_root) sp_instr_copen2(instructions(), spcont,
> + addr, pcursor->lex());
> + }
> + return i == NULL || add_instr(i);
> +}
> +
> +
> -bool sp_head::add_open_cursor(THD *thd, sp_pcontext *spcont, uint offset,
> +bool sp_head::add_open_cursor(THD *thd, sp_pcontext *spcont,
> + const sp_rcontext_addr &addr,
> sp_pcontext *param_spcont,
> List<sp_assignment_lex> *parameters)
> {
> diff --git a/sql/sp_instr.cc b/sql/sp_instr.cc
> --- a/sql/sp_instr.cc
> +++ b/sql/sp_instr.cc
> @@ -1912,22 +1925,10 @@ sp_instr_cclose::execute(THD *thd, uint *nextp)
> void
> sp_instr_cclose::print(String *str)
> {
> - const LEX_CSTRING *cursor_name= m_ctx->find_cursor(m_cursor);
> -
> + const LEX_CSTRING cmd= {STRING_WITH_LEN("cclose ")};
> + const sp_pcursor *cursor= m_ctx->get_pcursor(*this);
> /* cclose name@offset */
> - size_t rsrv= SP_INSTR_UINT_MAXLEN+8;
> -
> - if (cursor_name)
> - rsrv+= cursor_name->length;
> - if (str->reserve(rsrv))
> - return;
> - str->qs_append(STRING_WITH_LEN("cclose "));
> - if (cursor_name)
> - {
> - str->qs_append(cursor_name->str, cursor_name->length);
> - str->qs_append('@');
> - }
> - str->qs_append(m_cursor);
> + print_cmd_and_var(cmd, cursor ? *cursor : empty_clex_str, *this, str);
may be print_cmd_and_var() should take LEX_CSTRING* as a second
argument, not LEX_CSTRING&, then you wouldn't need to repeat
cursor ? *cursor : empty_clex_str
every time
> }
>
>
> @@ -2068,10 +2060,64 @@ sp_instr_cursor_copy_struct::exec_core(THD *thd, uint *nextp)
> {
> sp_cursor tmp(thd, true);
> // Open the cursor without copying data
> + // TODO: Check with DmitryS if hiding ROOT_FLAG_READ_ONLY is OK.
and?
> + DBUG_ASSERT(thd->lex == m_lex_keeper.lex());
> + auto backup_flags= thd->lex->query_arena()->mem_root->flags;
> + thd->lex->query_arena()->mem_root->flags&= ~ROOT_FLAG_READ_ONLY;
> +
> + /*
> + Open the cursor in its sp_rcontext, because
> + Items inside the cursor statement have rcontext handler
> + relative to the cursor rcontext. In this example package:
> +
> + CREATE PACKAGE BODY pkg AS
> + CURSOR c0(p0 INT, p1 VARCHAR(10)) IS SELECT p0, p1 FROM DUAL;
> + PROCEDURE p1 AS
> + BEGIN
> + FOR r0 IN c0(1,'c1')
> + LOOP
> + SELECT r0.c0, r0.c1;
> + END LOOP;
> + END;
> + END;
> +
> + Item_splocal instances for the cursor parameters p0 and p1
> + (in the cursor SELECT statement) use sp_rcontext_handler_local.
> + But the cursor is opened from the PROCEDURE p1, which
> + has its own sp_rcontext. thd->spcont is pointing to the
> + PROCEDURE p1 sp_rcontext at the time of the current
> + sp_instr::exec_core().
> +
> + Using the above package as an example again, let's do the following:
> + - backup thd->spcont (pointing to the PROCEDURE p1 sp_rcontext)
> + - reset thd->spcont to the sp_rcontext of the PACKAGE BODY pkg
> + - open the cursor in its sp_rcontext
> + - restore thd->spcont to the sp_rcontext of the PROCEDURE p1
> + */
> + sp_rcontext *rcontext_backup= thd->spcont;
> + thd->spcont= m_rcontext_handler->get_rcontext(thd->spcont);
> + ret= tmp.open(thd);
> + thd->spcont= rcontext_backup;
wouldn't it be logical, if the cursor would know its context instead
of relying on thd->spcont? then you wouldn't need this hackish swapping.
> + thd->lex->query_arena()->mem_root->flags= backup_flags;
> +
> - if (!(ret= tmp.open(thd)))
> + if (!ret)
> {
> Row_definition_list defs;
> /*
> + If m_rcontext_handler is sp_rcontext_handler_member then the cursor
> + structure is being exported from a package wide cursor to a
> + variable, e.g.:
> + CREATE PACKAGE BODY pkg AS
> + CURSOR c0 IS SELECT 1 AS c0, 'c1' AS c1 FROM DUAL;
> + mr0 c0%ROWTYPE;
> + ...
> + END;
may be you should use primarily non-ORACLE-mode syntax in code comments?
> + In this case the cursor structure must have the same life cycle with
> + the sp_package (pointed by thd->spcont->m_sp), thus let's use
> + thd->spcont->m_sp->query_arena() as the arena.
> +
> + Otherwise, the structure is being exported for a local variable
> + whose life cycle is the current sp_head::execute().
> Create row elements on the caller arena.
> It's the same arena that was used during sp_rcontext::create().
> This puts cursor%ROWTYPE elements on the same mem_root
> @@ -2106,14 +2155,95 @@ sp_instr_cursor_copy_struct::execute(THD *thd, uint *nextp)
> void
> sp_instr_cursor_copy_struct::print(String *str)
> {
> - sp_variable *var= m_ctx->find_variable(m_var);
> - const LEX_CSTRING *name= m_ctx->find_cursor(m_cursor);
> - str->append(STRING_WITH_LEN("cursor_copy_struct "));
> - str->append(name);
> + const LEX_CSTRING cmd= {STRING_WITH_LEN("cursor_copy_struct ")};
> + const sp_pcursor *cursor= m_ctx->get_pcursor(*this);
> + const sp_variable *var= m_ctx->get_pvariable(m_var);
> + // cursor_copy_struct cur@0 var@0
> + print_cmd_and_var(cmd, cursor ? *cursor : empty_clex_str, *this, str);
> str->append(' ');
> + str->append(*m_var.rcontext_handler()->get_name_prefix());
> str->append(&var->name);
> str->append('@');
> - str->append_ulonglong(m_var);
> + str->append_ulonglong(m_var.offset());
> +}
> +
> +
> +/*
> + sp_instr_copen2 class functions.
> +*/
> +
> +PSI_statement_info sp_instr_copen2::psi_info=
> +{ 0, "copen2", 0};
> +
> +
> +int
> +sp_instr_copen2::execute(THD *thd, uint *nextp)
> +{
> + DBUG_ENTER("sp_instr_copen2::execute");
> + m_lex_keeper.disable_query_cache();
> + int res= m_lex_keeper.cursor_reset_lex_and_exec_core(thd, nextp, false, this);
> + *nextp= m_ip + 1;
> + DBUG_RETURN(res);
> +}
> +
> +
> +int sp_instr_copen2::exec_core(THD *thd, uint *nextp)
> +{
> + DBUG_ENTER("sp_instr_copen2::exec_core");
> + sp_cursor *cursor= m_rcontext_handler->get_cursor(thd, m_offset);
> + /*
> + CREATE PACKAGE BODY pkg
> + CURSOR package_body_wide_cursor;
> + PROCEDURE p1()
> + BEGIN
> + OPEN package_body_wide_cursor; -- #2
> + CLOSE package_body_wide_cursor;
> + END;
> + BEGIN
> + OPEN package_body_wide_cursor; -- #1
> + CLOSE package_body_wide_cursor;
> + END;
> +
> + "OPEN package_body_wide_cursor" can be called from two different places:
> + #1. From the package initialization secion. In this case
> + sp_head::mem_main_root::flags does not have the ROOT_FLAG_READ_ONLY bit
> + yet - it will be set in the end of the current sp_head::execute().
> + #2. From a package routine. In this case sp_head::execute() representing
> + the initialization section of the package was called earlier and
> + its mem_main_root::flags already has the ROOT_FLAG_READ_ONLY flag.
Why is it set?
> + Unset it temporarily.
> + */
> + /*
> + TODO:
> + this assert crashes due to a problem in the metadata modification code
what problem?
> + */
> + //DBUG_ASSERT(thd->spcont->m_sp->get_package() ||
> + // (thd->lex->query_arena()->mem_root->flags & ROOT_FLAG_READ_ONLY));
> + DBUG_ASSERT(thd->lex == m_lex_keeper.lex());
> + auto backup_flags= thd->lex->query_arena()->mem_root->flags;
> + thd->lex->query_arena()->mem_root->flags&= ~ROOT_FLAG_READ_ONLY;
> +
> + /*
> + Open the cursor in its sp_rcontext.
> + See sp_instr_cursor_copy_struct::exec_core() for details.
> + */
> + sp_rcontext *ctx_backup= thd->spcont;
> + thd->spcont= m_rcontext_handler->get_rcontext(thd->spcont);
> + int rc= cursor->open(thd);
> + thd->spcont= ctx_backup;
> +
> + thd->lex->query_arena()->mem_root->flags= backup_flags;
> + DBUG_RETURN(rc);
> +}
> +
> +
> +void
> +sp_instr_copen2::print(String *str)
> +{
> + const LEX_CSTRING cmd= {STRING_WITH_LEN("copen2 ")};
> + const sp_pcursor *cursor= m_ctx->get_pcursor(*this);
> + /* copen2 name@offset */
> + print_cmd_and_var(cmd, cursor ? *cursor : empty_clex_str, *this, str);
> }
>
>
> diff --git a/sql/sp_rcontext.cc b/sql/sp_rcontext.cc
> --- a/sql/sp_rcontext.cc
> +++ b/sql/sp_rcontext.cc
> @@ -34,17 +34,48 @@
>
> Sp_rcontext_handler_local sp_rcontext_handler_local;
> Sp_rcontext_handler_package_body sp_rcontext_handler_package_body;
> +Sp_rcontext_handler_member sp_rcontext_handler_member;
>
> sp_rcontext *Sp_rcontext_handler_local::get_rcontext(sp_rcontext *ctx) const
> {
> return ctx;
> }
>
> +const sp_variable *
> +Sp_rcontext_handler_local::get_pvariable(const sp_pcontext *pctx, uint i) const
> +{
> + return pctx->find_variable(i);
> +}
> +
> +const sp_pcursor *
> +Sp_rcontext_handler_local::get_pcursor(const sp_pcontext *pctx, uint i) const
> +{
> + return pctx->find_cursor(i);
> +}
> +
> +
> sp_rcontext *Sp_rcontext_handler_package_body::get_rcontext(sp_rcontext *ctx) const
> {
> return ctx->m_sp->m_parent->m_rcontext;
> }
>
> +const sp_variable *
> +Sp_rcontext_handler_package_body::get_pvariable(const sp_pcontext *pctx, uint i)
> + const
> +{
> + DBUG_ASSERT(0);
> + return nullptr;
What do you mean? One cannot define a variable in a package body?
> +}
> +
> +const sp_pcursor *
> +Sp_rcontext_handler_package_body::get_pcursor(const sp_pcontext *pctx, uint i)
> + const
> +{
> + return pctx->top_context()->sp()->m_parent->
> + get_parse_context()->find_member_cursor(i);
> +}
> +
> +
> const LEX_CSTRING *Sp_rcontext_handler_local::get_name_prefix() const
> {
> return &empty_clex_str;
> @@ -57,6 +88,48 @@ const LEX_CSTRING *Sp_rcontext_handler_package_body::get_name_prefix() const
> return &sp_package_body_variable_prefix_clex_str;
> }
>
> +sp_cursor *Sp_rcontext_handler_local::get_cursor(THD *thd, uint offset) const
> +{
> + return thd->spcont->get_cursor(offset);
this is starting to look fragile, because you're now
swapping thd->spcont back and forth.
One more argument, why it wasn't a good idea and better to
let cursor know its context instead.
> +}
> +
> +sp_cursor *Sp_rcontext_handler_package_body::get_cursor(THD *thd,
> + uint offset) const
> +{
> + return get_rcontext(thd->spcont)->get_member_cursor(offset);
> +}
> +
> +
> +sp_rcontext *Sp_rcontext_handler_member::get_rcontext(sp_rcontext *ctx) const
> +{
> + return ctx;
> +}
> +
> +const LEX_CSTRING *Sp_rcontext_handler_member::get_name_prefix() const
> +{
> + static const LEX_CSTRING sp_package_body_variable_prefix_clex_str=
> + {STRING_WITH_LEN("MEMBER.")};
> + return &sp_package_body_variable_prefix_clex_str;
> +}
> +
> +sp_cursor *Sp_rcontext_handler_member::get_cursor(THD *thd,
> + uint offset) const
> +{
> + return thd->spcont->get_member_cursor(offset);
> +}
> +
> +const sp_variable *
> +Sp_rcontext_handler_member::get_pvariable(const sp_pcontext *pctx, uint i) const
> +{
> + return pctx->find_variable(i);
> +}
> +
> +const sp_pcursor *
> +Sp_rcontext_handler_member::get_pcursor(const sp_pcontext *pctx, uint i) const
> +{
> + return pctx->top_context()->find_member_cursor(i);
> +}
> +
>
> ///////////////////////////////////////////////////////////////////////////
> // sp_rcontext implementation.
> diff --git a/sql/structs.h b/sql/structs.h
> --- a/sql/structs.h
> +++ b/sql/structs.h
> @@ -862,26 +862,47 @@ class Lex_for_loop_bounds_intrange: public Lex_for_loop_bounds_st
> };
>
>
> +/*
> + This structure is used as a Bison token type.
> + It must stay as small as possible, hence it uses a "union" declaration:
> + - m_target_bound is used only in "FOR idx IN 0..9" style loops
> + - m_cursor_rcontext_handler is used only in "FOR rec IN cursor" style loops
> + They are never needed at the same time.
> + See "%union size check" in sql_yacc.yy for details on the token size limit.
> +*/
> struct Lex_for_loop_st
> {
> public:
> + enum class Type : uchar
> + {
> + INT_RANGE= 0, // FOR i IN 1..10
> + CURSOR_IMPLICIT= 1, // FOR r IN (SELECT 1 FROM DUAL)
> + CURSOR_EXPLICIT= 2 // FOR r IN cursor1
> + };
> class sp_variable *m_index; // The first iteration value (or cursor)
> - class sp_variable *m_target_bound; // The last iteration value
> + union
> + {
> + class sp_variable *m_target_bound; // The last iteration value
> + const class Sp_rcontext_handler *m_cursor_rcontext_handler;
> + };
> int m_cursor_offset;
> int8 m_direction;
> - bool m_implicit_cursor;
> + Type m_type;
you can also use bit fields to reduce sizeof(Lex_for_loop_st)
even further. Like m_type:2, m_direction:1, m_cursor_offset:29
> void init()
> {
> m_index= 0;
> m_target_bound= 0;
> m_cursor_offset= 0;
> m_direction= 0;
> - m_implicit_cursor= false;
> + m_type= Type::INT_RANGE;
> + }
> + bool is_for_loop_cursor() const
> + {
> + return m_type == Type::CURSOR_IMPLICIT || m_type == Type::CURSOR_EXPLICIT;
> }
> - bool is_for_loop_cursor() const { return m_target_bound == NULL; }
> bool is_for_loop_explicit_cursor() const
> {
> - return is_for_loop_cursor() && !m_implicit_cursor;
> + return m_type == Type::CURSOR_EXPLICIT;
> }
> };
>
> diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc
> --- a/sql/sql_lex.cc
> +++ b/sql/sql_lex.cc
> @@ -6537,16 +6537,20 @@ LEX::find_variable(const LEX_CSTRING *name,
> const Sp_rcontext_handler **rh) const
> {
> sp_variable *spv;
> - if (spcont && (spv= spcont->find_variable(name, false)))
> + const sp_pcontext *frame_pctx;
> + if (spcont && (spv= spcont->find_variable(name, &frame_pctx, false)))
> {
> *ctx= spcont;
> - *rh= &sp_rcontext_handler_local;
> + if (frame_pctx == sphead->frame_for_members())
> + *rh= &sp_rcontext_handler_member;
> + else
> + *rh= &sp_rcontext_handler_local;
> return spv;
> }
> sp_package *pkg= sphead ? sphead->m_parent : NULL;
> - if (pkg && (spv= pkg->find_package_variable(name)))
> + if (pkg && (spv= pkg->find_member_variable(name)))
> {
> - *ctx= pkg->get_parse_context()->child_context(0);
> + *ctx= pkg->frame_for_members();
> *rh= &sp_rcontext_handler_package_body;
I don't quite understand. Before your change the first if() was
handling local variables, and the second - package-wide, as far as I
understand.
afte your change the first if() apparently also can find package-wide
variables. What does the second if() do now?
> return spv;
> }
> @@ -7182,20 +7248,26 @@ bool LEX::sp_for_loop_cursor_declarations(THD *thd,
> thd->parse_error();
> DBUG_RETURN(true);
> }
> - if (unlikely(!(pcursor= spcont->find_cursor_with_error(&name, &coffs,
> - false)) ||
> +
> + sp_rcontext_addr cursor_addr(nullptr, 0);
> +
> + if (unlikely(!(pcursor= find_cursor_with_error(&name, &cursor_addr)) ||
> pcursor->check_param_count_with_error(param_count)))
> DBUG_RETURN(true);
>
> if (!(loop->m_index= sp_add_for_loop_cursor_variable(thd, index,
> - pcursor, coffs,
> + pcursor,
> + cursor_addr,
> bounds.m_index,
> item_func_sp)))
> DBUG_RETURN(true);
> loop->m_target_bound= NULL;
> loop->m_direction= bounds.m_direction;
> - loop->m_cursor_offset= coffs;
> - loop->m_implicit_cursor= bounds.m_implicit_cursor;
> + loop->m_cursor_rcontext_handler= cursor_addr.rcontext_handler();
> + loop->m_cursor_offset= cursor_addr.offset();
why do you store m_cursor_rcontext_handler and m_cursor_offset
separately, wouldn't it be more natural to store, like
loop->m_cursor_addr= cursor_addr;
?
> + loop->m_type= bounds.m_implicit_cursor ?
> + Lex_for_loop_st::Type::CURSOR_IMPLICIT :
> + Lex_for_loop_st::Type::CURSOR_EXPLICIT;
> DBUG_RETURN(false);
> }
>
> @@ -8128,21 +8221,21 @@ Item *LEX::make_item_colon_ident_ident(THD *thd,
> Item *LEX::make_item_plsql_cursor_attr(THD *thd, const LEX_CSTRING *name,
> plsql_cursor_attr_t attr)
> {
> - uint offset;
> - if (unlikely(!spcont || !spcont->find_cursor(name, &offset, false)))
> + Cursor_ref ref(name, sp_rcontext_addr(nullptr, 0));
> + if (unlikely(!spcont || !find_cursor(name, &ref)))
wait, why find_cursor() gets the name as an argument
if name is already set in Cursor_ref?
> {
> my_error(ER_SP_CURSOR_MISMATCH, MYF(0), name->str);
> return NULL;
> }
> switch (attr) {
> case PLSQL_CURSOR_ATTR_ISOPEN:
> - return new (thd->mem_root) Item_func_cursor_isopen(thd, name, offset);
> + return new (thd->mem_root) Item_func_cursor_isopen(thd, ref);
> case PLSQL_CURSOR_ATTR_FOUND:
> - return new (thd->mem_root) Item_func_cursor_found(thd, name, offset);
> + return new (thd->mem_root) Item_func_cursor_found(thd, ref);
> case PLSQL_CURSOR_ATTR_NOTFOUND:
> - return new (thd->mem_root) Item_func_cursor_notfound(thd, name, offset);
> + return new (thd->mem_root) Item_func_cursor_notfound(thd, ref);
> case PLSQL_CURSOR_ATTR_ROWCOUNT:
> - return new (thd->mem_root) Item_func_cursor_rowcount(thd, name, offset);
> + return new (thd->mem_root) Item_func_cursor_rowcount(thd, ref);
> }
> DBUG_ASSERT(0);
> return NULL;
Regards,
Sergei
Chief Architect, MariaDB Server
and security(a)mariadb.org
