plpgsql: implement RAISE statements #106351

DrewKimball · 2023-07-06T22:52:01Z

builtins: add builtin function to raise a notice synchronously to the client

This patch adds a builtin function, crdb_internal.plpgsql_raise, which allows
the caller to send a notice to the client with specified severity, message,
detail, hint, and PG code. The notice is immediately flushed to the client
instead of being buffered until the query result is closed. This functionality
will be used to implement the PLpgSQL RAISE statement.

The crdb_internal.plpgsql_raise builtin is undocumented and intended only
for internal use.

plpgsql: implement parsing for RAISE statements

This patch adds parser support for PLpgSQL RAISE statements. This ncludes
all syntax forms apart from the empty RAISE, which is only valid in
combination with (currently unimplemented) EXCEPTION blocks. A future
commit will add support in the optbuilder as well.

plpgsql: implement RAISE statement

This patch adds support for the PLpgSQL RAISE statement. The RAISE
statement can send messages back to the client during execution, as well
as raise a user-specified error. There are a few variations on the syntax,
but in general RAISE statements have a log level (default EXCEPTION),
a message (if not specified, the code string is used), and various options:
DETAIL, HINT, ERRCODE etc.

With log level EXCEPTION the error is returned just like any other error,
but for other levels it is sent as a notice to the client and flushed
synchronously before execution continues. This feature is often used to
track progress, since the notices are sent before execution finishes.

Fixes #105251

Release note (sql change): Added support for the PLpgSQL RAISE statement,
which allows sending notices to the client and raising errors. Currently the
notice is only sent to the client; support for logging notices is left for
future work.

cockroach-teamcity · 2023-07-06T22:52:14Z

This change is

rharding6373

Impressive work!

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

-- commits line 20 at r2:
s/ncludes/includes

pkg/sql/conn_io.go line 1087 at r1 (raw file):

// SendNotice is part of the RestrictedCommandResult interface.
func (r *streamingCommandResult) SendNotice(ctx context.Context, notice pgnotice.Notice) error {
	// Unimplemented: the internal executor does not support notices.

nit: indentation.

Also, if this is unimplemented or unsupported, should we return an unimplemented error?

pkg/sql/notice.go line 55 at r1 (raw file):

func (p *planner) SendClientNotice(ctx context.Context, notice pgnotice.Notice) error {
	if log.V(2) {
		log.Infof(ctx, "sending notice: %+v", notice)

Would it be useful to include the client we're sending to as part of the logging?

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

	buildOptionExpr := func(name string, expr plpgsqltree.PLpgSQLExpr, isDup bool) opt.ScalarExpr {
		if isDup {
			panic(pgerror.Newf(pgcode.PLpgSQL, "RAISE option already specified: %s", name))

nit: in postgres, dupes use the syntax error code:

rharding=# create function f() returns void language plpgsql as $$ 

  begin raise notice using message = 'hello', detail='world', detail='again'; end $$;

CREATE FUNCTION

rharding=# select f();

ERROR:  42601: RAISE option already specified: DETAIL

CONTEXT:  PL/pgSQL function f() line 2 at RAISE

LOCATION:  exec\_stmt\_raise, pl\_exec.c:3812

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

	}
	if argIdx < len(args) {
		panic(pgerror.Newf(pgcode.PLpgSQL, "too many parameters specified for RAISE"))

In postgres this error happens during function creation, but IIUC this error happens when the function with the raise is called. Could you please clarify?

pkg/sql/plpgsql/parser/plpgsql.y line 1118 at r2 (raw file):

option_expr:
  USING option_type assign_operator

I don't think this will correctly parse multiple options correctly. In postgres, you specify multiple options like this:

create function f() returns void language plpgsql as $$ 
  begin 
    raise notice using message = 'hello', detail='world'; 
  end $$;

But it looks like the parser currently looks for:

raise notice using message = 'hello', using detail='world';

This would be incorrect and doesn't parse in postgres.

[Coming back to this after reviewing everything]: I saw in a later TODO that you noticed this too but it seemed allowed by the syntax. I sympathize that specifications likeRAISE \[ _level_ \] SQLSTATE '_sqlstate_' \[ USING _option_ = _expression_ \[, ... \] \]; in https://www.postgresql.org/docs/current/plpgsql-errors-and-messages.html are hard to parse. Ultimately I think that what postgres is doing is consistent with the spec and should be the source of truth for the syntax.

pkg/sql/plpgsql/parser/plpgsql.y line 1046 at r3 (raw file):

    return unimplemented(plpgsqllex, "empty RAISE statement")
  }
| RAISE error_level SCONST optional_format_exprs optional_option_exprs ';'

minor nit: we seem to default to using "opt_" instead of "optional_" in names in this file.

pkg/sql/plpgsql/parser/plpgsql.y line 1080 at r3 (raw file):

;

error_level:

nit: since error_level is optional, I think this should be renamed.

pkg/sql/sem/builtins/builtins.go line 8051 at r1 (raw file):

					var code string
					if regexp.MustCompile(`[A-Z0-9]{5}`).MatchString(codeString) {
						// The supplied argument is a valid PG code.

Isn't it only a valid PG code if it's one of the codes mapped to in PLpgSQLConditionNameToCode or in https://www.postgresql.org/docs/current/errcodes-appendix.html?

pkg/sql/logictest/testdata/logic_test/udf_plpgsql line 571 at r3 (raw file):

CREATE OR REPLACE FUNCTION f() RETURNS INT AS $$
  BEGIN
    RAISE DEBUG 'foo';

Could you add a test without a level, e.g. RAISE 'foo'?

pkg/sql/plpgsql/parser/testdata/stmt_raise line 36 at r2 (raw file):

DECLARE
BEGIN
  RAISE LOG 'Nonexistent ID --> %', user_id;

Also add a test that parses multiple format expressions.

pkg/sql/plpgsql/parser/testdata/stmt_raise line 122 at r2 (raw file):

  RAISE internal_screaming
  USING MESSAGE = 'blah blah blah',
  USING COLUMN = 'foo';

As mentioned in a previous comment, this doesn't parse in postgres, so these parser tests need an update.

knz · 2023-07-15T09:09:14Z

I'm curious, why does the built-in not interrupt execution by returning an error when the severity "error" is specified?

DrewKimball · 2023-07-16T00:54:24Z

I'm curious, why does the built-in not interrupt execution by returning an error when the severity "error" is specified?

It should, what makes you think it doesn't?

knz · 2023-07-16T11:09:14Z

It should, what makes you think it doesn't?

Your own PR description:

With log level EXCEPTION the error is returned just like any other error, but for other levels it is sent as a notice to the client and flushed synchronously before execution continues.

If ERROR stops execution too, the PR description should mention that.

DrewKimball · 2023-07-17T07:54:51Z

Your own PR description:

With log level EXCEPTION the error is returned just like any other error, but for other levels it is sent as a notice to the client and flushed synchronously before execution continues.

If ERROR stops execution too, the PR description should mention that.

I see, I'll make sure to clarify things, thanks.

chengxiong-ruan

The parser change LGTM. nice work.

DrewKimball

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @rharding6373)

-- commits line 20 at r2: