sqlpage should close open transactions after each HTTP request

[lovasoa]

Discussed in #710

^{Originally posted by bever1337 November 25, 2024}
Following example 11: https://sql.datapage.app/component.sql?component=form

I've created a sql script that creates a temporary table, copies to it, and then merges from the temporary table to the permanent table.

Typically, I use 'ON COMMIT DROP' when making temporary tables. On my second upload, sqlpage throws the error that my temporary table already exists. I kill the server, then put begin and commit as the first and last lines of my sql script. I test again, and get a similar error. Sometimes I receive postgres transaction warnings, ex beginning before committing.

Finally, I add commit; begin and commit as the first and last lines. This works, but means I've thrown away a transaction I wasn't managing.

How should users work with sqlpage transactions? Is there a way to align transactions with request lifecycles?

This pseudo code seems to work:

COMMIT;

BEGIN;

CREATE TEMPORARY TABLE example_temp (
  LIKE example
) ON COMMIT DROP;

COPY example_temp (my_id)
FROM
  'example_form_input' (FORMAT 'csv', HEADER);

MERGE INTO example from example_temp;

SELECT
  'redirect' AS component,
  '/example' AS link;

COMMIT;

Judiciously beginning and ending every sqlpage script with begin and commit negates the need for commit;begin; and commit; in some files. Not sure which is best.

[lovasoa]

This was also reported in #695

[vtzi]

I don't think SQLPage should try to "fix" the connection. Rather the problem should be reported and connection recycled (close/open).

If I'm not wrong there are two cases to get into such situation:

1. Developer just forgets to commit/rollback the transaction - clearly a bug to be fixed in the app code.
2. Error (e.g. constraint violation) inside the transaction.

The second case is more interesting since ideally the developer should be able to handle such cases.

[lovasoa]

I think the best option for users is just to launch a ROLLBACK before returning the connection to the pool whenever an error occurs. If there was no active transaction, let the rollback fail without raising an additional error. This would have fixed both your problem and @bever1337 . But this would keep the current intuitive behavior for users who do not use explicit transactions at all, and would not incur any performance penalty.

What do you think ?

[lovasoa]

We could also add a configuration option to disable the connection pooler completely, which would close the connection after each request, guaranteeing no lingering transactions. This would also be useful to people connecting to an external connection pooler.

[vtzi]

I like disabling the connection pooler - entirely and/or just for some endpoints. Besides the transaction handling this will also fix hanging temp tables and whatever other session data kept within the connection.

Issuing a ROLLBACK is a hacky thing, imo. I know Oracle is not supported atm, but it has nested transactions which makes rollback more challenging.

BTW, why don't you like recycling the connection on errors?

[bever1337]

Error (e.g. constraint violation) inside the transaction.

@vtzi I had seen these issues as tangential, but I'm glad you're bringing attention to it. If they're related issues, all the better.

I had assumed SQLPage would offer an error.sql template that functions like the 404.sql template, but exception trapping/management works great.

Besides the transaction handling this will also fix hanging temp tables and whatever other session data kept within the connection.

I think the "hanging" temporary tables are directly related to transaction handling. If developers can control transactions, then we can control table drops.

I think the best option for users is just to launch a ROLLBACK before returning the connection to the pool whenever an error occurs.

We could also add a configuration option to disable the connection pooler completely, which would close the connection after each request, guaranteeing no lingering transactions.

For me, personally, knowing the "rules" of SQLPage is sufficient. Connection pooling is good, lack of opinions is good, and I am happy hand-managing transactions as-needed. I prefer the "try and ROLLBACK" approach.

After refreshing myself with sqlx, I have come to appreciate that SQLPage is non-opinionated. "Have connection, will query" is a good baseline because it returns transaction control to the sql scripts.

[bever1337]

Bringing this from one of the related discussions. It looks like each sql implementation has its own opinions. Postgres will automatically throw away the transaction, but sqlite wants to soldier on.

https://www.postgresql.org/docs/7.3/plpgsql-errors-and-messages.html

PostgreSQL does not have a very smart exception handling model. Whenever the parser, planner/optimizer or executor decide that a statement cannot be processed any longer, the whole transaction gets aborted and the system jumps back into the main loop to get the next query from the client application.

https://www.sqlite.org/lang_transaction.html

For all of these errors, SQLite attempts to undo just the one statement it was working on and leave changes from prior statements within the same transaction intact and continue with the transaction. However, depending on the statement being evaluated and the point at which the error occurs, it might be necessary for SQLite to rollback and cancel the entire transaction.

[lovasoa]

I implemented automatic rollback of opened transaction before returning a connection to the pool. This way, you cannot get a connection with a half-finished transaction in progress when you start executing a new page.

Can you test it and confirm everything works like you would expect ?

Here is a pre-release linux binary.
sqlpage.0.32.linux.musl.bin.gz

You can also get prereleases from lovasoa/sqlpage:main on docker.

I also discovered and fixed two related bugs in the process: https://github.com/sqlpage/SQLPage/blob/main/CHANGELOG.md#0320-unreleased

[vtzi]

This is not the proper way to handle errors in the SQL scripts, imo. It kind of fixes only errors within explicit multistatement transactions.

1. The happy path in most endpoints involve mostly read-only operations - no need to penlize these with additional hidden statement (i.e. at least a network roundtrip).

2. Temp tables will hang in the connection in case of errors. E.g.

CREATE TEMP TABLE aaa(...);
...
-- SQL statement causing an error 
...
DROP TABLE aaa; 

(Not all databases have Postgresql ON COMMIT DROP)

3. Connections may keep other state as well (e.g. Postgresql session advisory locks) which will leak in case of errors.

Postgres has DISCARD ALL statement to reset the connection state, MSSQL has sp_reset_connection for similar purpose, but for portability and simplicity - recycling on errors should be fine (or config that some scripts should not use connection pooling).

That said - it's your decision. Just have in mind that error handling will bite again sooner or later.

[lovasoa]

I see what you mean...

For the first point, there is indeed a performance penalty to running rollback, but it's an order of magnitude lower than discarding the connection.

I think they are bound to transactions in sqlite too:

sqlite> BEGIN; CREATE TEMPORARY TABLE t(x); ROLLBACK;
sqlite> select * from t;
Parse error: no such table: t

and in mssql too:

BEGIN TRANSACTION; CREATE TABLE #t(x int); ROLLBACK;
SELECT * FROM #t; -- Error 208 Invalid object name '#t'.

https://sqliteonline.com/#sqltext=%23url-sqlite%3Ddb-mssql%0D%0A%23tab-name%3DMS%20SQL%0D%0ABEGIN%20TRANSACTION%3B%20%0A%0ACREATE%20TABLE%20%23t(x%20int)%3B%20%0A%0AROLLBACK%3B%0A%0A%0ASELECT%20*%20FROM%20%23t%3B

This leaves mysql, where they explicitly mention it as a limitation...

About additional session state: I think that's a problem all the time, not particularly when an error occurs.

If we close the connection after an error occurs, an attacker can cause a denial of service by just repeatedly making invalid requests that will close database connections faster than we can open new ones...

[lovasoa]

Ok, I'm reopening this :)

I changed the implementation, to run a rollback only after an error occurs, and separately added an optional sqlpage/on_reset.sql script in which you can put whatever sql you want, that will be run after each page load, and can optionnally decide to discard the current db connection.

Please let me know what you think !

[vtzi]

Looks great, thank you!

There is small error in the docs here: sqlpage/on_disconnect.sql - shouldn't it be on_reset.sql instead?

[lovasoa]

You are right, thank you! I fixed the docs.