Skip to content

Conversation

fulghum
Copy link
Contributor

@fulghum fulghum commented Mar 6, 2025

Adds initial support for RAISE statements in PL/pgSQL functions. There are still a few edge case TODOs (e.g. using the client_min_messages config param to determine what level of notices to send to clients), and this initial pass does not include any exception handling.

Notice messages are queued in the current session, then sent to the client from the connection handler, right before results are sent to the client. Support for setting notices in the DoltSession is added in dolthub/dolt#8974.

PostgreSQL Docs

Copy link
Contributor

github-actions bot commented Mar 6, 2025

Main PR
covering_index_scan_postgres 394.21/s 392.18/s -0.6%
index_join_postgres 156.74/s 158.49/s +1.1%
index_join_scan_postgres 189.10/s 188.53/s -0.4%
index_scan_postgres 12.70/s 12.74/s +0.3%
oltp_point_select 2887.06/s 2830.48/s -2.0%
oltp_read_only 1926.31/s 1896.02/s -1.6%
select_random_points 113.63/s 112.77/s -0.8%
select_random_ranges 137.99/s 135.94/s -1.5%
table_scan_postgres 10.46/s 10.41/s -0.5%
types_table_scan_postgres 5.44/s 5.47/s +0.5%

Copy link
Contributor

github-actions bot commented Mar 6, 2025

Main PR
Total 42090 42090
Successful 15676 15710
Failures 26414 26380
Partial Successes1 5207 5194
Main PR
Successful 37.2440% 37.3248%
Failures 62.7560% 62.6752%

${\color{lightgreen}Progressions (47)}$

aggregates

QUERY: CREATE FUNCTION balkifnull(int8, int4)
RETURNS int8
STRICT
LANGUAGE plpgsql AS $$
BEGIN
    IF $1 IS NULL THEN
       RAISE 'erroneously called with NULL argument';
    END IF;
    RETURN NULL;
END$$;
QUERY: CREATE FUNCTION balkifnull(int8, int8)
RETURNS int8
PARALLEL SAFE
STRICT
LANGUAGE plpgsql AS $$
BEGIN
    IF $1 IS NULL THEN
       RAISE 'erroneously called with NULL argument';
    END IF;
    RETURN NULL;
END$$;

alter_table

QUERY: CREATE FUNCTION boo(int) RETURNS int IMMUTABLE STRICT LANGUAGE plpgsql AS $$ BEGIN RAISE NOTICE 'boo: %', $1; RETURN $1; END; $$;

plpgsql

QUERY: create function raise_test3(int) returns int as $$
begin
    raise notice 'This message has no parameters (despite having %% signs in it)!';
    return $1;
end;
$$ language plpgsql;
QUERY: create function excpt_test1() returns void as $$
begin
    raise notice '% %', sqlstate, sqlerrm;
end; $$ language plpgsql;
QUERY: drop function excpt_test1();
QUERY: create function raise_exprs() returns void as $$
declare
    a integer[] = '{10,20,30}';
    c varchar = 'xyz';
    i integer;
begin
    i := 2;
    raise notice '%; %; %; %; %; %', a, a[i], c, (select c || 'abc'), row(10,'aaa',NULL,30), NULL;
end;$$ language plpgsql;
QUERY: drop function raise_exprs();
QUERY: create or replace function raise_test() returns void as $$
begin
  raise notice '% % %', 1, 2, 3
     using errcode = '55001', detail = 'some detail info', hint = 'some hint';
  raise '% % %', 1, 2, 3
     using errcode = 'division_by_zero', detail = 'some detail info';
end;
$$ language plpgsql;
QUERY: create or replace function raise_test() returns void as $$
begin
  raise division_by_zero;
end;
$$ language plpgsql;
QUERY: create or replace function raise_test() returns void as $$
begin
  raise sqlstate '1234F';
end;
$$ language plpgsql;
QUERY: create or replace function raise_test() returns void as $$
begin
  raise division_by_zero using message = 'custom' || ' message';
end;
$$ language plpgsql;
QUERY: create or replace function raise_test() returns void as $$
begin
  raise using message = 'custom' || ' message', errcode = '22012';
end;
$$ language plpgsql;
QUERY: create or replace function raise_test() returns void as $$
begin
  raise notice 'some message' using message = 'custom' || ' message', errcode = '22012';
end;
$$ language plpgsql;
QUERY: create or replace function raise_test() returns void as $$
begin
  raise division_by_zero using message = 'custom' || ' message', errcode = '22012';
end;
$$ language plpgsql;
QUERY: create or replace function raise_test() returns void as $$
begin
  raise;
end;
$$ language plpgsql;
QUERY: create or replace function raise_test() returns void as $$
begin
  raise exception 'custom exception'
     using detail = 'some detail of custom exception',
           hint = 'some hint related to custom exception';
end;
$$ language plpgsql;
QUERY: drop function raise_test();
QUERY: create or replace function strtest() returns text as $$
begin
  raise notice 'foo\\bar\041baz';
  return 'foo\\bar\041baz';
end
$$ language plpgsql;
QUERY: create or replace function strtest() returns text as $$
begin
  raise notice E'foo\\bar\041baz';
  return E'foo\\bar\041baz';
end
$$ language plpgsql;
QUERY: select strtest();
QUERY: create or replace function strtest() returns text as $$
begin
  raise notice 'foo\\bar\041baz\';
  return 'foo\\bar\041baz\';
end
$$ language plpgsql;
QUERY: select strtest();
QUERY: create or replace function strtest() returns text as $$
begin
  raise notice E'foo\\bar\041baz';
  return E'foo\\bar\041baz';
end
$$ language plpgsql;
QUERY: select strtest();
QUERY: drop function strtest();
QUERY: create or replace function outer_func(int)
returns int as $$
declare
  myresult int;
begin
  raise notice 'calling down into inner_func()';
  myresult := inner_func($1);
  raise notice 'inner_func() done';
  return myresult;
end;
$$ language plpgsql;
QUERY: create or replace function outer_outer_func(int)
returns int as $$
declare
  myresult int;
begin
  raise notice 'calling down into outer_func()';
  myresult := outer_func($1);
  raise notice 'outer_func() done';
  return myresult;
end;
$$ language plpgsql;
QUERY: drop function outer_outer_func(int);
QUERY: drop function outer_func(int);
QUERY: create or replace function outer_func(int)
returns int as $$
declare
  myresult int;
begin
  raise notice 'calling down into inner_func()';
  myresult := inner_func($1);
  raise notice 'inner_func() done';
  return myresult;
end;
$$ language plpgsql;
QUERY: create or replace function outer_outer_func(int)
returns int as $$
declare
  myresult int;
begin
  raise notice 'calling down into outer_func()';
  myresult := outer_func($1);
  raise notice 'outer_func() done';
  return myresult;
end;
$$ language plpgsql;
QUERY: drop function outer_outer_func(int);
QUERY: drop function outer_func(int);

polymorphism

QUERY: create function bleat(int) returns int as $$
begin
  raise notice 'bleat %', $1;
  return $1;
end$$ language plpgsql;

psql

QUERY: CREATE FUNCTION warn(msg TEXT) RETURNS BOOLEAN LANGUAGE plpgsql
AS $$
  BEGIN RAISE NOTICE 'warn %', msg ; RETURN TRUE ; END
$$;
QUERY: DROP FUNCTION warn(TEXT);
QUERY: CREATE FUNCTION psql_error(msg TEXT) RETURNS BOOLEAN AS $$
  BEGIN
    RAISE EXCEPTION 'error %', msg;
  END;
$$ LANGUAGE plpgsql;
QUERY: DROP FUNCTION psql_error;

rowsecurity

QUERY: CREATE FUNCTION op_leak(int, int) RETURNS bool
    AS 'BEGIN RAISE NOTICE ''op_leak => %, %'', $1, $2; RETURN $1 < $2; END'
    LANGUAGE plpgsql;

Footnotes

  1. These are tests that we're marking as Successful, however they do not match the expected output in some way. This is due to small differences, such as different wording on the error messages, or the column names being incorrect while the data itself is correct.

@fulghum fulghum requested a review from Hydrocharged March 6, 2025 00:41
Copy link
Collaborator

@Hydrocharged Hydrocharged left a comment

Choose a reason for hiding this comment

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

Really not a fan of moving InterpreterOperation to another package in core, since it's very specific to plpgsql (we may not reuse it for other interpreter backends).

I'm also not a fan of putting the pgproto3.Backend in contextValues. Although we're not necessarily doing it now, contextValues is moreso for temporary usage, and it's fine to completely rebuild it between statements, in the middle of statements, etc. This backend is being set once per query, which puts an unintended requirement on contextValues.

I'd much rather have a separate, more permanent construct where we store the backend, but my true preference would be for the context to hold any notices until the query completes, fails, etc. The logic in /server that sends messages back to the client would read any notices and send those first. I think this also takes care of the import cycle, and it keeps the client-communicating logic all in the same package.

Would like to know your thoughts on this!

@fulghum
Copy link
Contributor Author

fulghum commented Mar 7, 2025

Thanks for taking a look. Yeah, I hear ya – I wasn't crazy about pulling InterpreterOperation into core either, but wanted start somewhere and get the functionality working. Queuing the notices in the context seems like a good idea and shouldn't be a big change from here. I'm happy to try it out. I can also throw in some documentation to contextValues to explain the intent you described above – if that's how contextValues is intended to be used, that seems important to document in the code.

Copy link
Collaborator

@Hydrocharged Hydrocharged left a comment

Choose a reason for hiding this comment

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

Much better! Fantastic work!

@fulghum fulghum merged commit b985cb9 into main Mar 12, 2025
14 checks passed
@fulghum fulghum deleted the fulghum/raise branch March 12, 2025 20:14
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.

2 participants