brianc / node-postgres

PostgreSQL client for node.js.
https://node-postgres.com
MIT License
12.17k stars 1.21k forks source link

Unexpected implicit transactions in `query` with `;` #2933

Open skeet70 opened 1 year ago

skeet70 commented 1 year ago

I noticed this while trying to use node-postgres with some integration tests. For the tests I needed to load in fixtures (that we normally loaded with psql in k8s) in test code. I read the .sql file in and passed it to client.query, but had confusing errors about running CREATE DATABASE in a transaction. I read the documentation and saw that the driver requires explicit transaction blocks to get transactions. It took me a pretty long time to realize that there were implicit transactions being added to the mix because I had multiple statements in one query. I worked around it by lightly parsing the file and splitting each statement up to be fed into .query, but I would've expected this behaviour to be documented on the transactions doc page (regardless of if it's pg or postgres itself causing this to happen).

I made a minimal reproduction of the weird behaviour here.

See also #1763 and #2100

aspic-fish commented 1 year ago

postgres doc

When a simple Query message contains more than one SQL statement (separated by semicolons), those statements are executed as a single transaction, unless explicit transaction control commands are included to force a different behavior.

This lib is just a driver. It sends queries to postgres and shows responses. It doesn't care how postgres handles them.

skeet70 commented 1 year ago

As I said in the original comment,

It took me a pretty long time to realize that there were implicit transactions being added to the mix because I had multiple statements in one query

I did figure out that it was coming from postgres eventually.

Recommendations in other issues for importing SQL files were to load the file and pass the string to query. That recommendation isn't made in the documentation, but it can fail because of this behaviour. It's fine that this is a just driver, and that strings are passed as a single simple query. What happened here was unexpected to me after reading the documentation. I don't this is a bug in the library, but it is a gap the documentation could cover. Maybe a page on recommendations for importing SQL files using the driver, since it seems to be repeating question?

aspic-fish commented 1 year ago

The main section of the docs should not be bloated. This lib is pretty low level. So it assumes you know what your queries are doing. Otherwise it'd require to duplicate postgres docs much. Well, some sort of a FAQ page is actually a good idea. If it's what you want I suggest you to change the title to something like: docs: create FAQ page, docs[FAQ]: add reading queries from .sql file section

But as for the question itself it's actually not that simple.

There should be even more nuances as I'm not a db guy in the first place.

At this point the problem requires a separate library. But the actual demand for it seems quite low. So it probably won't be created anytime soon.

This instruction can be used as a base for the future FAQ section and updated as needed if it'd be created ofk.

pigoz commented 1 year ago

Oh my god, I spent so much time on this!