isoos / postgresql-dart

Dart PostgreSQL driver: supports extended query format, binary protocol and statement reuse.
https://pub.dev/packages/postgres
BSD 3-Clause "New" or "Revised" License
128 stars 34 forks source link

Timeout is not applied to the running query when transaction is involved. #344

Open JuanLuisNL opened 1 month ago

JuanLuisNL commented 1 month ago

If I run this from a computer:

execute(‘BEGIN’) String cSQL = ‘SELECT * FROM customers WHERE id = 1635 FOR UPDATE’ execute(cSQL, queryMode: QueryMode.extended, timeout: const Duration(seconds: 5)) .....

and from another order I execute the same code to check the lock, although I have a timeout of 5 seconds, the error takes about 1 minute. SERVER: statement_timeout = 0 What could be happening?

isoos commented 1 month ago

@JuanLuisNL: Thanks for reporting this! Is there any chance that you can provide a minimal working example, using the dockerized test setup like this: https://github.com/isoos/postgresql-dart/blob/master/test/decode_test.dart#L328-L343 ?

JuanLuisNL commented 1 month ago

Sorry for my ignorance, I don't understand what you mean). I have this function (I have removed the unimportant parts), first I execute it from a computer, when I execute this same code from another computer (to prevent the modification of the row), the program waits 1 minute instead of 5 seconds. If I commit in the first computer, automatically in the second one the process continues. I don't know if I'm making myself clear

Future lockRow(String cTabla, int id) async { try { String cSQL = "SELECT * FROM $cTabla WHERE id = $id FOR UPDATE"; await execute("BEGIN"); await _oCn.execute(cSQL, queryMode: QueryMode.extended, timeout: timeOut); return true; } catch (e, stack) { Utils.printError(e, stack); return false; } }

JuanLuisNL commented 1 month ago

Can I provide more information to solve the problem?

isoos commented 1 month ago

@JuanLuisNL: I was mostly offline for the past two weeks, slowly getting back to these issues.

The most helpful thing would be a minimal test in our test suite that fails (as a PR or a comment here), along the lines of:

  withPostgresServer('sql null', (server) {
    test('decode output', () async {
      final c1 = await server.newConnection();
      final c2 = await server.newConnection();

      // ... demonstrates the issue
    });
  });

The test suite will create a local postgres instance inside a docker image, and we can run it repeatedly in CI after fixing the underlying issue.

JuanLuisNL commented 1 month ago

Sorry, I'm not familiar with that kind of testing and I don't really know what to do (

What I do to test is from "pgadmin" run this : BEGIN ; SELECT * FROM emp0001.customers WHERE id = 1635 FOR UPDATE;

Then from my code I run: await execute("BEGIN"); String cSQL = "SELECT * FROM emp0001.customers WHERE id = 1635 FOR UPDATE"; Result result = await execute(cSQL, timeOut: const Duration(seconds: 5));

The program stops for about 1 minute, not 5 seconds.. But if from "pgadmin" I do COMMIT the program continues immediately.

I'm sorry I can't do more to explain myself ...

isoos commented 1 month ago

@JuanLuisNL: Yeah, that's a helpful description. I may need some time to look into it, but now I understand it better.

JuanLuisNL commented 1 month ago

Thank you very much

isoos commented 1 day ago

@JuanLuisNL: I was able to create a small reproduction of the issue as a regular test: #362

Unfortunately I don't see a quick fix for it, as it will require the cancellation of the currently running query with the coordination of a client-side timer. If somebody wants to dig into it, I imagine it something like:

isoos commented 1 day ago

We should also consider to use the statement_timeout variable on each session (or per-query + reset if needed).

JuanLuisNL commented 18 hours ago

Thank you very much István

I did the ‘statement_timeout’ (per-query + reset if needed) and it works perfectly. As before I check if there is a lock set, that command will be executed very few times, so it's not a problem to do it that way.

I appreciate your help

isoos commented 14 hours ago

As before I check if there is a lock set

Could you please elaborate if this is a DB lock or something in your app?

I am starting to think that statement_timeout would be the correct way to address this. E.g. on each connection start we set it to whatever the resolved value is, and we keep track of the latest value we have set it to. Whenever we send in a new statement, the client may check the currently resolved timeout parameter and if it differs, we send a new value. /cc @simolus3 - is there any similar feature in the sqlite client package?

simolus3 commented 8 hours ago

There isn't, as sqlite3 doesn't have an obvious way to set timeouts (it's possible because you can inject a callback that it invokes once in a while, but nothing official like with postgres).

and we keep track of the latest value we have set it to. Whenever we send in a new statement, the client may check the currently resolved timeout parameter and if it differs, we send a new value

I think this is a reasonable approach :+1: We might want to suggest setting a statement timeout on the connection primarily, only altering it for rare queries that really need different values. Otherwise we'd end up sending a lot of variable changes as the effective timeout for every query jumps around. (or maybe there should just be a Future<void> setQueryTimeout(Duration) method instead of an explicit option for statements?)

isoos commented 8 hours ago

We might want to suggest setting a statement timeout on the connection primarily, only altering it for rare queries that really need different values.

We already have SessionSettings.queryTimeout (which should be migrated to / renamed to statementTimeout long term?) with a default of 5 minutes. Users using the defaults would just have it set at connection time, and whenever you override it for a query or session, it would be updated. Certainly not for every query, unless you do set a different value for the queries.

isoos commented 5 hours ago

@JuanLuisNL: Please give me more details on how you made the transaction + the session_timeout work. I have a pending work in #363 that is not really working yet, but I may be overlooking something.