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: add support for INTO STRICT option #120486

Merged
merged 3 commits into from Mar 15, 2024

Conversation

DrewKimball
Copy link
Collaborator

logictest: move plpgsql SELECT/RETURNING INTO tests to new file

This commit is test-only refactor that moves the tests for SELECT
and RETURNING INTO PL/pgSQL syntax to their own file.

Epic: None

Release note: None

plpgsql: add support for INTO STRICT syntax

This commit adds support for SELECT INTO STRICT and
RETURNING INTO STRICT for SQL statements within a PL/pgSQL routine.
This option causes the routine to error when the SQL statement doesn't
return exactly one row. This is handled via a ScalarGroupBy to compute
the row count, followed by a call to crdb_internal.plpgsql_raise if
necessary to throw the error.

Fixes #115297

Release note (sql change): It is now possible to use the STRICT option
with SELECT ... INTO and RETURNING INTO in order to enforce that the
SQL statement returns exactly one row.

sql: add setting to make PL/pgSQL INTO strict by default

This commit adds a new session setting, plpgsql_use_strict_into, that
causes PL/pgSQL SELECT ... INTO and RETURNING INTO syntax to behave
as if the STRICT option was specified. This causes the SQL statement
to result in an error if it doesn't return exactly one row. This is
useful for applications migrating from oracle, since it has this "strict"
behavior by default.

Informs #115297

Release note (sql change): Added a setting plpgsql_use_strict_into,
which causes PL/pgSQL SELECT ... INTO and RETURNING INTO to require
exactly one row from the SQL statement, similar to oracle behavior.

@DrewKimball DrewKimball requested a review from a team as a code owner March 14, 2024 10:20
Copy link

blathers-crl bot commented Mar 14, 2024

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.

@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 19 of 19 files at r1, 2 of 2 files at r2, 10 of 10 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball and @mgartner)


-- commits line 34 at r3:
For my own edification, how do you opt out of STRICT default behavior in Oracle? Via a similar setting that disables "strict by default"?


pkg/sql/vars.go line 3243 at r3 (raw file):

			return nil
		},
		GlobalDefault: globalFalse,

nit: should we introduce a cluster setting too?


pkg/sql/opt/optbuilder/plpgsql.go line 1014 at r2 (raw file):

}

// buildPLpgSQLRaise builds a Project expression which calls the implements the

nit: "which calls the implements ..."


pkg/ccl/logictestccl/testdata/logic_test/plpgsql_into line 398 at r2 (raw file):


query II rowsort
SELECT * FROM xy;

nit: might be a bit less verbose to use SELECT count(*) FROM xy; or SELECT count(*) == 8 FROM xy; since we only care whether another row was inserted or not.

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! 1 of 0 LGTMs obtained (waiting on @mgartner and @yuzefovich)


-- commits line 34 at r3:

Previously, yuzefovich (Yahor Yuzefovich) wrote…

For my own edification, how do you opt out of STRICT default behavior in Oracle? Via a similar setting that disables "strict by default"?

The docs claim this behavior is "by default", but I haven't been able to find a way to do that. See the "Usage Notes" section in the docs. I guess maybe you could use LIMIT 1 plus an exception handler that catches NO_DATA_FOUND and sets variables to NULL?


pkg/sql/vars.go line 3243 at r3 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: should we introduce a cluster setting too?

I think it's probably fine to leave as-is, since the default can be set for a user (right?). I'm open to convincing otherwise, though.


pkg/sql/opt/optbuilder/plpgsql.go line 1014 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: "which calls the implements ..."

Done.


pkg/ccl/logictestccl/testdata/logic_test/plpgsql_into line 398 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: might be a bit less verbose to use SELECT count(*) FROM xy; or SELECT count(*) == 8 FROM xy; since we only care whether another row was inserted or not.

Good idea, done.

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.

:lgtm:

Reviewed 10 of 10 files at r4, 10 of 10 files at r6, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball and @mgartner)


pkg/sql/vars.go line 3243 at r3 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

I think it's probably fine to leave as-is, since the default can be set for a user (right?). I'm open to convincing otherwise, though.

Yeah, sgtm.

@DrewKimball
Copy link
Collaborator Author

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 14, 2024

Build failed:

This commit is test-only refactor that moves the tests for SELECT
and RETURNING INTO PL/pgSQL syntax to their own file.

Epic: None

Release note: None
This commit adds support for `SELECT INTO STRICT` and
`RETURNING INTO STRICT` for SQL statements within a PL/pgSQL routine.
This option causes the routine to error when the SQL statement doesn't
return exactly one row. This is handled via a `ScalarGroupBy` to compute
the row count, followed by a call to `crdb_internal.plpgsql_raise` if
necessary to throw the error.

Fixes cockroachdb#115297

Release note (sql change): It is now possible to use the `STRICT` option
with `SELECT ... INTO` and `RETURNING INTO` in order to enforce that the
SQL statement returns exactly one row.
This commit adds a new session setting, `plpgsql_use_strict_into`, that
causes PL/pgSQL `SELECT ... INTO` and `RETURNING INTO` syntax to behave
as if the `STRICT` option was specified. This causes the SQL statement
to result in an error if it doesn't return exactly one row. This is
useful for applications migrating from oracle, since it has this "strict"
behavior by default.

Informs cockroachdb#115297

Release note (sql change): Added a setting `plpgsql_use_strict_into`,
which causes PL/pgSQL `SELECT ... INTO` and `RETURNING INTO` to require
exactly one row from the SQL statement, similar to oracle behavior.
@DrewKimball
Copy link
Collaborator Author

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 15, 2024

@craig craig bot merged commit ebf92d2 into cockroachdb:master Mar 15, 2024
22 checks passed
@DrewKimball DrewKimball deleted the no-data-found branch March 15, 2024 10:48
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.

Nicely done!!!
:lgtm:

Reviewed 1 of 19 files at r1, 27 of 27 files at r7, 2 of 2 files at r8, 10 of 10 files at r9, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale)

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 handling NO DATA FOUND exception
4 participants