plpgsql: add support for nested blocks #117710
Conversation
DrewKimball
requested review from
adityamaru and
rharding6373
and removed request for
a team
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
DrewKimball
requested review from
mgartner,
a team and
msbutler
and removed request for
a team,
adityamaru and
msbutler
|
The first two commits are from #117608. |
There was a problem hiding this comment.
This looks very clean! I want to take a closer look at r5, but on a first glance, it looks good.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball and @mgartner)
pkg/sql/opt/optbuilder/plpgsql.go line 746 at r5 (raw file):
func (b *plpgsqlBuilder) resolveOpenQuery(open *ast.Open) tree.Statement { var boundStmt tree.Statement for i := len(b.blocks) - 1; i >= 0; i-- {
Is there a reason why this has to be done in reverse block order?
pkg/sql/sem/plpgsqltree/statements.go line 120 at r4 (raw file):
newStmt = s.CopyNode() } newStmt.(*Block).Decls[i] = decl
s/decl/newDecl
Should fix the rewrite test.
pkg/sql/sem/plpgsqltree/statements.go line 120 at r4 (raw file):
newStmt = s.CopyNode() } newStmt.(*Block).Decls[i] = decl
Speaking of the rewrite test, it would be great if you could add some nested blocks with references that need to be rewritten. It's a good way to test the visitors.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/optbuilder/plpgsql.go line 746 at r5 (raw file):
Previously, rharding6373 (Rachael Harding) wrote…
Is there a reason why this has to be done in reverse block order?
Variable shadowing isn't allowed now, but when it is, we want to look at more recent blocks first. I'll add a comment.
pkg/sql/sem/plpgsqltree/statements.go line 120 at r4 (raw file):
Previously, rharding6373 (Rachael Harding) wrote…
s/decl/newDecl
Should fix the rewrite test.
Agh, thanks for catching that. Done.
pkg/sql/sem/plpgsqltree/statements.go line 120 at r4 (raw file):
Previously, rharding6373 (Rachael Harding) wrote…
Speaking of the rewrite test, it would be great if you could add some nested blocks with references that need to be rewritten. It's a good way to test the visitors.
Good idea, done.
There was a problem hiding this comment.
Reviewed 1 of 3 files at r3, 51 of 51 files at r7, 31 of 31 files at r8, 28 of 28 files at r9, 45 of 45 files at r10, all commit messages.
Reviewable status:
pkg/sql/opt/optbuilder/plpgsql.go line 1376 at r9 (raw file):
} args := make(memo.ScalarListExpr, 0, len(con.def.Params)) addArg := func(name ast.Variable, typ *types.T) {
nit: typ is not used in this function
pkg/sql/opt/optbuilder/plpgsql.go line 1579 at r9 (raw file):
} func (b *plpgsqlBuilder) pushBlock(bs plBlock) *plBlock {
nit: add short description
pkg/sql/opt/optbuilder/plpgsql.go line 1584 at r9 (raw file):
} func (b *plpgsqlBuilder) popBlock() {
nit: add short description
pkg/sql/opt/optbuilder/plpgsql.go line 1604 at r9 (raw file):
var count int for i := range b.blocks { count += len(b.blocks[i].varTypes)
Why len(varTypes) and not len(vars)?
pkg/ccl/logictestccl/testdata/logic_test/plpgsql_block line 251 at r10 (raw file):
# If an exception is thrown and caught within an inner block, the outer block # is not rolled back, and execution can continue.
Very cool!
pkg/ccl/logictestccl/testdata/logic_test/plpgsql_block line 461 at r10 (raw file):
# A block cannot currently be nested in a block with an exception handler # (tracked in #foo).
Did you mean to put an issue number here?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/optbuilder/plpgsql.go line 746 at r5 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
Variable shadowing isn't allowed now, but when it is, we want to look at more recent blocks first. I'll add a comment.
Ok, thanks for the clarification!
Previously, the parameters of a continuation sub-routine would be chosen by first appending the variables of the (single) block, then the parameters of the PL/pgSQL routine. In preparation for supporting nested blocks, this commit changes the order to first add the routine's parameters, and then the variables from the block. This prepares to enforce an invariant that the parameters/arguments of a parent sub-routine are always a prefix of those of a child sub-routine. This will simplify exception handling later on. Informs cockroachdb#114775 Release note: None
This commit makes two changes to the plpgsql Visitor interface and implementation - first, the `changed` return value has been replaced with pointer comparisons. Second, a new return value, `recurse`, indicates whether a node's children should be visited. This will allow for Visitor implementations that have to return early. Epic: None Release note: None
This commit pulls the logic for handling a PL/pgSQL block into a single method. Information specific to a given block (including a pointer to the parent block) is stored in a new struct, `blockScope`, which is allocated when the block is built. Note that nested blocks are still not allowed in this commit. Informs cockroachdb#114775 Release note: None
There was a problem hiding this comment.
TFTRs!
Reviewable status:
pkg/sql/opt/optbuilder/plpgsql.go line 1376 at r9 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
nit:
typis not used in this function
Good catch, Done.
pkg/sql/opt/optbuilder/plpgsql.go line 1579 at r9 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
nit: add short description
Done.
pkg/sql/opt/optbuilder/plpgsql.go line 1584 at r9 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
nit: add short description
Done.
pkg/sql/opt/optbuilder/plpgsql.go line 1604 at r9 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
Why
len(varTypes)and notlen(vars)?
No particular reason. vars does seem more self-explanatory, so I changed it to that.
pkg/ccl/logictestccl/testdata/logic_test/plpgsql_block line 461 at r10 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
Did you mean to put an issue number here?
Oops, fixed that.
This commit makes the final change to support nested blocks with some limitations, and adds corresponding tests. Currently, variable shadowing is not allowed, and neither are exception handlers for routines with nested blocks. Fixes cockroachdb#114775 Release note (sql change): PL/pgSQL now supports nested blocks, with the following limitations for now: variable shadowing is disallowed, and exception handlers cannot be used in a routine with nested blocks.
|
bors r+ |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
plpgsql: change order of params and variables
Previously, the parameters of a continuation sub-routine would be chosen
by first appending the variables of the (single) block, then the parameters
of the PL/pgSQL routine. In preparation for supporting nested blocks, this
commit changes the order to first add the routine's parameters, and then the
variables from the block. This prepares to enforce an invariant that the
parameters/arguments of a parent sub-routine are always a prefix of those
of a child sub-routine. This will simplify exception handling later on.
plpgsql: allow early exit for visitor
This commit makes two changes to the plpgsql Visitor interface and
implementation - first, the
changedreturn value has been replacedwith pointer comparisons. Second, a new return value,
recurse,indicates whether a node's children should be visited. This will
allow for Visitor implementations that have to return early.
plpgsql: refactor logic for building PL/pgSQL blocks
This commit pulls the logic for handling a PL/pgSQL block into a single
method. Information specific to a given block (including a pointer to the
parent block) is stored in a new struct,
blockScope, which is allocatedwhen the block is built. Note that nested blocks are still not allowed in
this commit.
plpgsql: add support for nested blocks
This commit makes the final change to support nested blocks with some
limitations, and adds corresponding tests. Currently, variable shadowing
is not allowed, and a block cannot be nested in another block that has an
exception handler.
Fixes #114775
Release note (sql change): PL/pgSQL now supports nested blocks, with
the following limitations for now: variable shadowing is disallowed,
and exception handlers cannot be used in a routine with nested blocks.