Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Protocol level transaction support #170

Closed
LjPalle opened this issue Apr 4, 2024 · 13 comments · Fixed by #200
Closed

Protocol level transaction support #170

LjPalle opened this issue Apr 4, 2024 · 13 comments · Fixed by #200

Comments

@LjPalle
Copy link
Contributor

LjPalle commented Apr 4, 2024

(Only slightly related to: #79)

So I encountered some problems regarding transactions on the network protocol level when communicating with certain clients. I found two places where transactions are relevant in the PostgreSQL protocol.

One is in the ReadyForQuery message which carries a status flag (see https://www.postgresql.org/docs/current/protocol-message-formats.html#PROTOCOL-MESSAGE-FORMATS-READYFORQUERY) - it seems that pgwire currently uses only the "idle" status and the library user cannot change this after e.g. a client sends a "BEGIN" query.

The other place is the tag of the CommandComplete message (see https://www.postgresql.org/docs/current/protocol-message-formats.html#PROTOCOL-MESSAGE-FORMATS-COMMANDCOMPLETE) - currently pgwire always uses the "SELECT" tag whenever rows are returned, but e.g. a fetch may also retrieve rows during a transaction and in this case a "FETCH" tag is expected, so the library user should be able to specify the tag in this case too.

@sunng87
Copy link
Owner

sunng87 commented Apr 4, 2024

hello @LjPalle , while it's possible to completely customize on_query of SimpleQueryHandler for that, I agree we can have built-in support for transaction feature on wire protocol.

My proposal is to add new member in our Response enum for both requirements:

pub enum Response<'a> {
    EmptyQuery,
    Query(QueryResponse<'a>, Tag), // For command tag, add a tag field to customize tag name
    Execution(Tag),
    Error(Box<ErrorInfo>),
    TransactionFailed, // return ready state `E`
    TransactionInProgress, // maybe we can have better name, returns ready state `T`
}

An alternative idea for fetch is to add new member called Fetch(QueryResponse<'a>), just depending on how many commands to be included.

I'd like to know your idea for this when you are writing a transaction-enable application based on this library. Also one more question, is it for SimpleQueryHandler only, do we need to support it for ExtendedQueryHandler too?

@LjPalle
Copy link
Contributor Author

LjPalle commented Apr 5, 2024

Hey @sunng87, I tweaked pgwire for my purposes in the following way:

  • regarding the tag, I added a command_tag String field inside the QueryResponse struct so that inside the send_query_response I could simply use that instead of "SELECT"
  • regarding the transaction state I've modified the ClientInfo trait so that there is also a transaction_state getter and setter, and then I've used that wherever ReadyForQuery message was created, instead of using only READY_STATUS_IDLE (the DefaultClient should use it of course as the initial value)

This was an expedient way for me to solve this issue, but I'm not sure whether it is the best. Yeah, I think this should include ExtendedQueryHandler too (my tweak above encompasses this implicitly). I'm not sure where are you aiming at with expanding the Response enum.

@sunng87
Copy link
Owner

sunng87 commented Apr 5, 2024

Thank you @LjPalle

For tag inclusion, I think it's totally OK to put a name into QueryResponse. Could you please make a pull request for that?

For transaction state, the reasons why I'm not maintaining it with the client are:

  • I think the state transition only happens when a particular query is executed, so the new state can be a return type of a query.
  • At the framework level, we don't know the exact time when state will change. It depends on how query is processed. So if we are going to maintain the state with ClientInfo, we will be asking user to keep it update to date accordingly, which makes the implementation complex. For example, we can't say the client state is idle when it's running a query.

But I think it's totally OK for your own application to track it that way since you have full control how the state is used.

@LjPalle
Copy link
Contributor Author

LjPalle commented Apr 8, 2024

Yeah sure, I'll send a pull request when I find the time.

Regard the state transition, won't the user need to somehow update or signal it anyway?

@sunng87
Copy link
Owner

sunng87 commented Apr 9, 2024

My idea is we only allow user to update it via return value of do_query

@LjPalle
Copy link
Contributor Author

LjPalle commented Apr 11, 2024

Ah, ok, now I understand what you meant. But note that one still has to store the transaction state somewhere since the ReadyForQuery message is sent even after parse and describe requests (unless the plan is to change their interface too).

@syedzayyan
Copy link

Hi, Is it possible to handle transactions yet?
We are playing around with supporting foreign tables and it sends a transaction as follows:

Simple Query Handler:

START TRANSACTION ISOLATION LEVEL REPEATABLE READ;

Extended Query Handler:


describe: Portal { name: "POSTGRESQL_DEFAULT_NAME", statement: StoredStatement { id: "POSTGRESQL_DEFAULT_NAME", statement: "DECLARE c1 CURSOR FOR\nSELECT id, name, ts, signed FROM public.test", parameter_types: [] }, parameter_format: UnifiedText, parameters: [], result_column_format: UnifiedText }

extended query: "DECLARE c1 CURSOR FOR\n  SELECT id, name, ts, signed FROM public.test"

It's unclear what the response to start transaction should be. Investigating for postgres_fdw the cursor response needs to be a non-error so in theory getting past the transaction it should be possible to respond to the fetches.

Any pointers are much appreciated.

@sunng87
Copy link
Owner

sunng87 commented Jun 26, 2024

@syedzayyan At the moment, we haven't cover transaction state in this library. You might be able to track it with a custom field in ClientInfo::metadata().

For users who has a scenario for this, contribution or just ideas are welcomed.

@jennykwan
Copy link
Contributor

Is there anything I can do to help? I'm implementing a POC of a DB engine that requires transactions.

@sunng87
Copy link
Owner

sunng87 commented Oct 19, 2024

@jennykwan thank you! The transaction support is almost finished in #200. I just need more time to review, document and test it. But note that in this library, we only have wire protocol level transaction semantic support. If you are implementing a transactional database, you will need to build internals by yourself.

@jennykwan
Copy link
Contributor

Thanks, @sunng87 ! Out of curiosity, are the changes in the PR compatible with psql's prompting behavior? Will a client like psql pick up the client status?

psql prompts:

  • Outside transaction: =#
  • Inside transaction in valid state: =*#
  • Inside failed transaction: =!#
  • In transaction but unknown (disconnected): =?#

@sunng87
Copy link
Owner

sunng87 commented Oct 20, 2024

I just added an example to #200 and it's able to control the psql prompts like:

❯ psql -h 127.0.0.1 -p 5432 -d public
NOTICE:  Supported queries in this example:
- BEGIN;
- ROLLBACK;
- COMMIT;
- SELECT 1;
psql (16.4, server 0.24.2)
WARNING: psql major version 16, server major version 0.24.
         Some psql features might not work.
Type "help" for help.

public=> BEGIN;
BEGIN
public=*> SELECT 1;
 SELECT 1 
----------
        1
(1 row)

public=*> NOT SUPPORTED;
FATAL:  Unsupported statement.
public=!> ROLLBACK;
ROLLBACK
public=> 

This prompt is controlled by transaction state in postgres message readyForQuery.

@jennykwan
Copy link
Contributor

Perfect! Thank you!

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 a pull request may close this issue.

4 participants