Unable to type check procedure CALL in postgresql

[cortopy]

I have a simple query like this

CALL my_procedure($1)

and I'm trying to use it with the type-checking macro like:

sqlx::query!(
    "CALL my_procedure($1)",
    &param,
)

But I'm getting this error

error occurred while decoding column 0: invalid type: string "Utility Statement", expected struct Explain at line 2 column 21

If I use the query without type checking like:

sqlx::query(
    "CALL my_procedure($1)",
).bind(&param);

it works fine, so it seems an issue with the macro and the parsing of procedure calls

[abonander]

Do you mind giving the output of EXPLAIN (VERBOSE, FORMAT JSON) CALL my_procedure(NULL);? That's what the macro's trying to parse here.

A minimal reproduction would be fine (preferable, actually) if you don't want to give the real query plan.

[abonander]

Hmm, trying to reproduce it myself, you apparently can't do an EXPLAIN on a CALL command as you get a syntax error.

Although what the macro is doing it's more like:

PREPARE call_foo AS CALL foo();
EXPLAIN (VERBOSE, FORMAT JSON) EXECUTE call_foo;

But even then you can't use CALL with the textual PREPARE, it only works with the extended query flow in the binary protocol which is difficult to invoke manually.

I suspect, however, that the output of EXPLAIN is more or less completely useless here as it's probably returning the literal string "Utility Statement".

Does my_procedure use any OUT parameters?

[cortopy]

thanks @abonander for answering so quickly!

This is what I got from the terminal for the query plan

ERROR:  syntax error at or near "CALL"
LINE 1: EXPLAIN (VERBOSE, FORMAT JSON) CALL append_to_store(NULL);

The procedure has more arguments but pg seems to complain about the syntax anyway

The procedure is this one:

CREATE OR REPLACE PROCEDURE append_to_store(
    notify_aggregate_type_id    TEXT,
    aggregate_id                UUID,
    current_version             INTEGER,
    perform_version_check       BOOLEAN,
    names                       TEXT[],
    timestamps                  TIMESTAMPTZ[],
    events                      JSONB[],
    INOUT aggregate_version     INTEGER DEFAULT NULL,
    INOUT sequence_number       BIGINT DEFAULT NULL
) 
LANGUAGE PLPGSQL
AS $$
DECLARE
    "event"                     JSONB;
    event_name                  TEXT;
    timestamp                   TIMESTAMPTZ;
BEGIN

    -- Retrieve the global offset value from the aggregate_type_id.
    PERFORM FROM aggregate_types WHERE id = notify_aggregate_type_id;
    IF NOT FOUND THEN
        RAISE EXCEPTION 'invalid aggregate type provided: %', notify_aggregate_type_id;
    END IF;

    -- Retrieve the latest aggregate version for the specified aggregate id.
    SELECT a."version" INTO aggregate_version FROM aggregates a WHERE id = aggregate_id AND a.aggregate_type_id = notify_aggregate_type_id;
    IF NOT FOUND THEN
        -- Add the initial aggregate representation inside the `aggregates` table.
        INSERT INTO aggregates (id, aggregate_type_id)
        VALUES (aggregate_id, notify_aggregate_type_id);

        -- Make sure to initialize the aggregate version in case
        aggregate_version = 0;
    END IF;

    -- Perform optimistic concurrency check.
    IF perform_version_check AND aggregate_version <> current_version THEN
        RAISE EXCEPTION 'invalid aggregate version provided: %, expected: %', current_version, aggregate_version;
    END IF;

    SELECT events.sequence_number INTO sequence_number from events ORDER BY time DESC LIMIT 1;

    FOR i IN 1 .. array_upper(events, 1)
    LOOP
        event_name = names[i];
        "event" = events[i];
        timestamp = timestamps[i];
        -- Increment the aggregate version prior to inserting the new event.
        aggregate_version = aggregate_version + 1;

        -- Insert the event into the events table.
        -- Version numbers should start from 1; sequence numbers should start from 0.
        INSERT INTO events
            (time, aggregate_id, aggregate_type_id, "version", name, "event")
        VALUES
            (coalesce(timestamp, now()), aggregate_id, notify_aggregate_type_id, aggregate_version, event_name, "event")
        RETURNING
            events.sequence_number INTO sequence_number;

        -- Send a notification to all listeners of the newly added events.
        PERFORM pg_notify(notify_aggregate_type_id, ''
            || '{'
            || '"source_id": "'      || aggregate_id      || '" ,'
            || '"version": '         || aggregate_version || ', '
            || '"sequence_number": ' || sequence_number   || ', '
            || '"event": '           || "event"::TEXT
            || '}');
        COMMIT;
    END LOOP;

    -- Update the aggregate with the latest version computed.
    UPDATE aggregates a SET "version" = aggregate_version WHERE id = aggregate_id AND a.aggregate_type_id = notify_aggregate_type_id;

    -- Update the global offset with the latest sequence number.
    UPDATE aggregate_types SET "offset" = sequence_number WHERE id = notify_aggregate_type_id;

END;
$$;

It's a custom version of this procedure

[abonander]

Hmm, okay. So there's a couple of orthogonal issues here:

1. since there's no output parameters, we really shouldn't even be running an EXPLAIN because we only use that for nullability inference for a query's output columns. That might actually speed up the compilation of a number of sqlx::query!() invocations since right now it appears that we're doing this for every query regardless of whether it outputs anything or not.
2. even if it had output parameters, we shouldn't be dying on EXPLAIN returning "Utility Statement", though I suspect it's actually returning something like this:

[
    "Utility Statement",
    // corresponding to the first `SELECT` statement
    {
        "plan": { ... }
    },
    ...
]

I'd need to experiment with this, preferably with a minimal reproduction, to see what it actually returns.

If I might make a suggestion though, I would recommend not baking lots of business logic into stored procedures in the database like this. It might seem more efficient but it's going to be much more difficult to maintain and reason about.

[abonander]

Okay, we already aren't doing EXPLAIN for queries without output columns. I failed to notice the INOUT parameters in the linked procedure.

And I can now confirm that it does just return ["Utility Statement"] for any procedure call so the fix here is to handle this in the EXPLAIN parsing. This does unfortunately mean that we can't do any kind of nullability inference here since the procedure call is effectively a black box, so we're forced to assume all the output columns are nullable.

[cortopy]

Totally agree with not putting too much "hidden" logic in procedures. It does feel like it's too much magic in a black box. However, in this case this is part of an event sourcing implementation, and so it relies a lot on integrity checks and the right sequences. I'm actually working right now in simplifying this a bit

Postgres 14 will support OUT parameters for procedures, but until then or if older versions support is required, it would have to be INOUT.

I'm not sure if I can do anything to help. I'm more than happy to try anything out

[abonander]

The fix is relatively simple, it just sadly means that the macros are rather limited in their ability to handle procedure calls.

[bgeron]

I think my PR #2271 will fix this issue.