cockroachdb / cockroach

CockroachDB — the cloud native, distributed SQL database designed for high availability, effortless scale, and control over data placement.
https://www.cockroachlabs.com
Other
30.1k stars 3.81k forks source link

sql,plpgsql: RAISE statements prevent automatic retries #119632

Open DrewKimball opened 8 months ago

DrewKimball commented 8 months ago

When a PL/pgSQL RAISE statement is executed, its message is immediately flushed to the client in order to mirror postgres behavior. However, this interferes with transaction retries, because the transaction can no longer be automatically retried once results have been flushed to the client. This is demonstrated by this test:

CREATE SEQUENCE s1;
CREATE SEQUENCE s2;

CREATE PROCEDURE p_no_raise() LANGUAGE PLpgSQL AS $$
  BEGIN
    SELECT IF(nextval('s1')<3, crdb_internal.force_retry('1h':::INTERVAL), 0);
  END
$$;

CREATE PROCEDURE p_raise() LANGUAGE PLpgSQL AS $$
  BEGIN
    RAISE NOTICE 'foo';
    SELECT IF(nextval('s2')<3, crdb_internal.force_retry('1h':::INTERVAL), 0);
  END
$$;

root@localhost:26257/defaultdb> CALL p_no_raise();
CALL

Time: 39ms total (execution 39ms / network 0ms)

root@localhost:26257/defaultdb> CALL p_raise();
NOTICE: foo
ERROR: restart transaction: crdb_internal.force_retry(): TransactionRetryWithProtoRefreshError: forced by crdb_internal.force_retry()
SQLSTATE: 40001
HINT: See: https://www.cockroachlabs.com/docs/v24.1/transaction-retry-error-reference.html

We probably don't want to start buffering notices until the client connection is closed, since that changes the behavior of RAISE statements and makes them less useful. We could avoid updating flushInfo.lastFlushed for notices, and that would allow retries again. However, that would mean that the client would see messages from the old, restarted transactions. Another possibility is to offer a setting that would change the behavior to buffering.

Notably, this limitation applies to read committed retries as well as serializable.

Jira issue: CRDB-36215

blathers-crl[bot] commented 7 months ago

Hi @mgartner, please add branch-* labels to identify which branch(es) this GA-blocker affects.

:owl: Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

mgartner commented 7 months ago

@DrewKimball and I discussed this in person. I think where we ended up was that:

Drew, correct me if I'm wrong, or feel free to add some more details.

DrewKimball commented 7 months ago

I'm leaning toward keeping the current behavior by default, and adding a setting that will cause notices to buffer.

mgartner commented 7 months ago

@DrewKimball we're not going to add the setting you suggest in 24.1, correct? If so, let's move to 24.2 or backlog.