Don't send discard all when state is changed in transaction

[zainkabani]

If a set statement is sent in a transaction then we shouldn't try to send the DISCARD ALL statement

Gets rid of two message clones that are almost always hit

[levkk]

SET leaks into the session even if ran inside a transaction. SET LOCAL does not, but I'm not entirely sure you can tell the difference from the C message.

[drdrsh]

@levkk You are right.
SET LOCAL only works within transaction and gets cleaned up afterwards so we don't need to worry about
SET outside transaction takes effect to the end of the session but we can unambiguously detect it from the C message if we check the transaction state.

SET inside a transaction is problematic. If the transaction is rolled back, the SET will be rolled back as well which means we don't have to worry about. However, if the transaction is committed, the SET value will persist to the end of the session and we cannot easily differentiate between SET and SET LOCAL from the C message.

This feature is generally a best effort to keep the session state clean, if the client enables transaction mode and uses SET calls inside a transaction then there isn't much we can do without parsing the query.

We can revisit this feature if/when we start parsing queries

[drdrsh]

@zainkabani We'll need to add tests to cover the branches I mentioned in the comment above? Even the case where we miss the SET operation and let it leak outside the transaction.

[levkk]

You can detect the problematic case and send the discard message then. Basically, if saw set and transaction committed ok, send it.

[drdrsh]

@levkk That would send DISCARD ALL after we see a SET LOCAL within a transaction (Which is also reported as SET by CommandComplete)

[levkk]

Makes sense. I'm looking at this as a bug in application we're trying to mitigate at the pooler. I agree that SET and SET LOCAL are a bit of a leaked abstraction of transaction mode though. I don't think it's so terrible to send DISCARD ALL in both cases if we don't want to parse SQL and detect the best case. Although it wouldn't be a bad idea to start parsing SQL (using our sqlparser crate) and this could be the first simple test case for that implementation.

[drdrsh]

One less clone to worry about!

[drdrsh]

Maybe add a comment here
// Read the first byte of the message without advancing the internal pointer.

[drdrsh]

Maybe change this to unwrap_or("$") , in the unlikely event that this branch is hit, it will not panic, instead it will hit the unsupported message branch in the match block

[drdrsh]

This comment is wrong, I added it when I used freeze but forgot to remove it after I removed the freeze. Can you remove it as well?

[levkk]

Doesn't look like this is safe to unwrap after all. Why $?

[drdrsh]

I asked for this to be extra cautious, our old implementation would panic if for some reason we didn't have bytes in the message. The $ is just to hit the last branch in the match statement and avoid the panic.

The comment about this being safe is because read_message wouldn't return success unless the entire payload is loaded in the buffer
https://github.com/levkk/pgcat/blob/main/src/messages.rs#L469-L497

We could go without it if it looks confusing

[levkk]

This feels like something we'd do in Ruby :) Let's be a bit more intentional in this mission-critical code.

[drdrsh]

Let's add a test for SET LOCAL to verify we don't send DISCARD ALL in these cases as well.

[drdrsh]

LGTM.
Pinging @levkk for any further comments

[levkk]

if !self.in_transaction {
    self.needs_clean = true;
}

why the continue?

[levkk]

Last nit and we can merge probably.

[levkk]

Sweet!