Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

plpgsql: implement labels for EXIT and CONTINUE #120733

Merged
merged 2 commits into from Mar 25, 2024

Conversation

DrewKimball
Copy link
Collaborator

plpgsql: implement labels for EXIT and CONTINUE

This commit adds support for specifying loop and block labels as the
target of EXIT and CONTINUE statements. This can allow an outer
loop to be the target of the statement instead of the innermost loop.
It also allows for control flow to jump to the end of a labeled block.
The following commit will fix handling for the root block and correct
some error messages.

Fixes #115271

Release note (sql change): The PL/pgSQL EXIT and CONTINUE statements
can now specify which loop or block is the target via labels.

plpgsql: handle EXIT/CONTINUE for root block and correct errors

This commit cleans up EXIT and CONTINUE with label handling. It is
now possible to EXIT from the root block or the routine itself, and
the various error messages that can result from incorrect usage have
been corrected.

Informs #115271

Release note: None

@DrewKimball DrewKimball requested a review from a team as a code owner March 20, 2024 05:32
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! :lgtm:

Reviewed 3 of 3 files at r1, 4 of 4 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball and @mgartner)


pkg/sql/opt/optbuilder/plpgsql.go line 1377 at r3 (raw file):

// that type-checking works out.
//
// returns a continuation which will throw the

nit: seems like a leftover sentence.


pkg/ccl/logictestccl/testdata/logic_test/procedure_plpgsql line 255 at r2 (raw file):

      EXIT foo;
      RAISE NOTICE 'after EXIT';
    END;

nit: should we add another RAISE after <<foo>> block to ensure it gets executed?

@DrewKimball DrewKimball force-pushed the labels branch 2 times, most recently from 3e12070 to 12a7d37 Compare March 20, 2024 22:03
Copy link
Collaborator Author

@DrewKimball DrewKimball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFYR!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner and @yuzefovich)


pkg/sql/opt/optbuilder/plpgsql.go line 1377 at r3 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: seems like a leftover sentence.

Done.


pkg/ccl/logictestccl/testdata/logic_test/procedure_plpgsql line 255 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: should we add another RAISE after <<foo>> block to ensure it gets executed?

Good idea, Done.

Copy link
Collaborator

@mgartner mgartner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Very nice!

Reviewed 2 of 4 files at r2, 5 of 5 files at r4, 5 of 5 files at r5, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @DrewKimball and @yuzefovich)


pkg/sql/opt/optbuilder/plpgsql.go line 156 at r4 (raw file):

	// of a statement block to call a sub-routine that models the statements that
	// come next after the block. Each continuation stores its context, and
	// callers filter depending on this context; see also continuationType.

nit: The head of the stack is the continuation sub-routine that models the statements after the current block, correct? Maybe worth making that a little more clear here, but up to you.


pkg/sql/opt/optbuilder/plpgsql.go line 1831 at r4 (raw file):

	// continuationLoopExit encapsulates the statements following a loop. Only
	// EXIT statements can jump execution to this type of continuation.

Terminal statements are not mentioned here because there is always an implicit EXIT, is that right? Or those continuations are continuationDefault?


pkg/sql/opt/optbuilder/plpgsql.go line 1906 at r4 (raw file):

// rootBlock returns the root block that encapsulates the entire routine.
func (b *plpgsqlBuilder) rootBlock() *plBlock {

I don't see where this is used.


pkg/sql/opt/optbuilder/plpgsql.go line 590 at r5 (raw file):

			if t.Label == b.routineName {
				// An EXIT from the routine name itself results in an end-of-function
				// error, even for a VOID-returning routine or one with OUT-parameters.

What's the reason for this being an evaluation-time error, and not an error when the function is created? This is what Postgres does?

@mgartner
Copy link
Collaborator

pkg/sql/opt/optbuilder/plpgsql.go line 1906 at r4 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

I don't see where this is used.

To be clear, I mean the rootBlock method.

Copy link
Collaborator Author

@DrewKimball DrewKimball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTRs!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @mgartner and @yuzefovich)


pkg/sql/opt/optbuilder/plpgsql.go line 156 at r4 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: The head of the stack is the continuation sub-routine that models the statements after the current block, correct? Maybe worth making that a little more clear here, but up to you.

Sure, I simplified it a bit and added a brief example. Done.


pkg/sql/opt/optbuilder/plpgsql.go line 1831 at r4 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Terminal statements are not mentioned here because there is always an implicit EXIT, is that right? Or those continuations are continuationDefault?

Do you mean the last statement in the loop body? Those will jump to the "continue" continuation. EXIT or RETURN could jump "past" this continuation, but not "to" it.


pkg/sql/opt/optbuilder/plpgsql.go line 1906 at r4 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

To be clear, I mean the rootBlock method.

It's used to handle EXIT from the root block. I meant to put it in the second commit - I've moved it now.


pkg/sql/opt/optbuilder/plpgsql.go line 590 at r5 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

What's the reason for this being an evaluation-time error, and not an error when the function is created? This is what Postgres does?

Yes, postgres throws it lazily. I'm not sure why anyone would want to do this. I could be talked into making this a creation-time error instead.

This commit adds support for specifying loop and block labels as the
target of `EXIT` and `CONTINUE` statements. This can allow an outer
loop to be the target of the statement instead of the innermost loop.
It also allows for control flow to jump to the end of a labeled block.
The following commit will fix handling for the root block and correct
some error messages.

Fixes cockroachdb#115271

Release note (sql change): The PL/pgSQL `EXIT` and `CONTINUE` statements
can now specify which loop or block is the target via labels.
This commit cleans up `EXIT` and `CONTINUE` with label handling. It is
now possible to `EXIT` from the root block or the routine itself, and
the various error messages that can result from incorrect usage have
been corrected.

Informs cockroachdb#115271

Release note: None
@DrewKimball
Copy link
Collaborator Author

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 25, 2024

@craig craig bot merged commit e354a4d into cockroachdb:master Mar 25, 2024
22 checks passed
@DrewKimball DrewKimball deleted the labels branch March 25, 2024 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

plpgsql: support label and condition for EXIT and CONTINUE
4 participants