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
131 stars 37 forks source link

Cannot ALTER TABLE <table> because it is being used by active queries in this session #390

Closed davidmartos96 closed 1 day ago

davidmartos96 commented 3 days ago

Hello, I'm encountering with an issue that I'm only able to replicate in the v3 of the package. On v2 and as an script on DBeaver is working correctly.

It throws the following on version 3.4.3:

Severity.error 55006: cannot ALTER TABLE "to_alter" because it is being used by active queries in this session
#0      new _PgResultStreamSubscription (package:postgres/src/v3/connection.dart:732:29)
#1      _BoundStatement.listen (package:postgres/src/v3/connection.dart:704:12)
#2      _PreparedStatement.run (package:postgres/src/v3/connection.dart:669:43)
#3      _PgSessionBase.execute (package:postgres/src/v3/connection.dart:171:31)
...

The issue occus in a bulk upload operation but I was able to simplify it to just 2 statements inside a transaction involving 2 basic tables.

The schema is the following:

other_table {
  other_id INTEGER NOT NULL,
  PRIMARY KEY (other_id)
}

to_alter {
  a_id INTEGER NOT NULL,
  a_other_id INTEGER NOT NULL REFERENCES other_table(other_id),
  PRIMARY KEY (a_id)
}

Here are the 2 statements, and below is the reproduction code for v3 and the working version on v2.

SELECT * FROM to_alter;

ALTER TABLE to_alter
  ADD CONSTRAINT fk_other 
  FOREIGN KEY (a_other_id) REFERENCES other(other_id);

v3 code

import 'package:postgres/postgres.dart' as pg;

Future<void> main() async {
  final endpoint = pg.Endpoint(
    host: '127.0.0.1',
    database: 'my_test',
    port: 5432,
    username: 'postgres',
    password: 'changeme',
  );
  final pgConn = await pg.Connection.open(
    endpoint,
    settings: const pg.ConnectionSettings(
      applicationName: 'my-test',
      sslMode: pg.SslMode.disable,
    ),
  );

  await pgConn.execute('DROP SCHEMA IF EXISTS public CASCADE');
  await pgConn.execute('CREATE SCHEMA public');

  await createTables(pgConn);

  await run(pgConn);
  await pgConn.close();
}

const tableToAlter = 'to_alter';
const otherTable = 'other';

Future<void> run(pg.Connection pgConn) async {
  await pgConn.runTx((tx) async {
    final stmts = <String>[
      // Select from the table that will be altered
      "SELECT * FROM $tableToAlter;",
      // Add a foreign key constraint
      '''
ALTER TABLE $tableToAlter
ADD CONSTRAINT fk_other 
  FOREIGN KEY (a_other_id) 
    REFERENCES $otherTable(other_id);
'''
    ];

    for (final stmt in stmts) {
      print("Executing: $stmt");
      await tx.execute(stmt);
    }
  });
}

Future<void> createTables(pg.Session pgConn) async {
  await pgConn.execute('DROP TABLE IF EXISTS $tableToAlter');
  await pgConn.execute('DROP TABLE IF EXISTS $otherTable');

  await pgConn.execute('''
CREATE TABLE $tableToAlter (
    a_id INTEGER NOT NULL,
    a_other_id INTEGER NOT NULL,
    PRIMARY KEY (a_id)
);
''');

  await pgConn.execute('''
CREATE TABLE $otherTable (
  other_id INTEGER NOT NULL,
  PRIMARY KEY("other_id")
);
''');
}

v2 code

import 'package:postgres/postgres.dart' as pg;

Future<void> main() async {
  final pgConn = pg.PostgreSQLConnection("127.0.0.1", 5432, "my_test", username: "postgres", password: "changeme");
  await pgConn.open();

  await pgConn.execute('DROP SCHEMA IF EXISTS public CASCADE');
  await pgConn.execute('CREATE SCHEMA public');

  await createTables(pgConn);

  await run(pgConn);
  await pgConn.close();
}

const tableToAlter = 'to_alter';
const otherTable = 'other';

Future<void> run(pg.PostgreSQLConnection pgConn) async {
  await pgConn.transaction((pgConn) async {
    final stmts = <String>[
      // Select from the table that will be altered
      "SELECT * FROM $tableToAlter;",
      // Add a foreign key constraint
      '''
ALTER TABLE $tableToAlter
ADD CONSTRAINT fk_other 
  FOREIGN KEY (a_other_id) 
    REFERENCES $otherTable(other_id);
'''
    ];

    for (final stmt in stmts) {
      print("Executing: $stmt");
      await pgConn.execute(stmt);
    }
  });
}

Future<void> createTables(pg.PostgreSQLConnection pgConn) async {
  await pgConn.execute('DROP TABLE IF EXISTS $tableToAlter');
  await pgConn.execute('DROP TABLE IF EXISTS $otherTable');

  await pgConn.execute('''
CREATE TABLE $tableToAlter (
    a_id INTEGER NOT NULL,
    a_other_id INTEGER NOT NULL,
    PRIMARY KEY (a_id)
);
''');

  await pgConn.execute('''
CREATE TABLE $otherTable (
  other_id INTEGER NOT NULL,
  PRIMARY KEY("other_id")
);
''');
}
isoos commented 3 days ago

I think there is only a very little benefit to batch operations like that, and if you do, you should use the simple query protocol, e.g. set QueryMode.simple either at execute or at the SessionSettings/ConnectionSettings. IIRC the default is now extended protocol, and it may not work with batching like that. Could you please try that?

davidmartos96 commented 3 days ago

What do you mean by batching in this example? I thought batching would be when you send multiple statements into the same execute, separated by semicolon.

I'll try those settings when I get the chance

davidmartos96 commented 2 days ago

@isoos Making the SELECT use the simple query protocol works. But is this behavior expected? Is it not possible to mix SELECT and ALTER TABLE in the same transaction using the default protocol? Fortunately we don't rely on parameter binding for this SELECT, but what if we were? Our bulk update script has some internal logic, that's why it also queries the database using SELECT.

Just trying to understand why using the simple query protocol for that particular query makes it work, and if we should be aware of any gotchas mixing both modes. Thanks!

isoos commented 2 days ago

@davidmartos96: the query protocol and its limitations are on the postgres server side, a good starting point maybe here: https://www.postgresql.org/docs/current/protocol.html

The simple query protocol may accept multiple statements in a single SQL text, but doesn't work with parameter binding (e.g. everything must be serialized into the text as string). The extended protocol works with parameter binding (also accepts binary input for those parameters), and in general is more efficient both on the client and the server side. (These are true regardless of the transaction status.)

The v2 had a non-trivial mostly implicit protocol selection logic, with sometimes unexpected behavior, and in v3 we have changed it to explicit selection with the extended protocol as the default. You can select the query protocol mode on multiple level, making either of them the default at pool, connection, session/transaction level, and still override it at the statement level. So you can freely mix and match these, but need to be aware of the benefits/drawbacks.

An important note: this is semantically wrong in your code:

await pgConn.runTx((pgConn) async {
 // using pgConn inside, as it hides/overrides the pgConn variable
}

It should be instead:

await pgConn.runTx((tx) async {
  // use tx inside
  // you may use multiple tx.execute() calls inside
}

Does the above answer your questions?

davidmartos96 commented 2 days ago

@isoos Thank you for the info! Just to clarify, I'm not trying to send multiple statements in the same SQL text. The provided example is a "for loop" to a list of String, which are executed and awaited separately. Does your answer still apply to this? We are not sure what the definition of batching is in this example.

isoos commented 2 days ago

We are not sure what the definition of batching is in this example.

I thought that you are concatenating the strings and sending them all in in a single query.

Does your answer still apply to this?

I think yes, most of it still applies, also see the comment on the transaction block, "you may use multiple tx.execute() calls inside".

I am unsure though, if I was able to answer or fix your issue.

davidmartos96 commented 2 days ago

@isoos I was shadowing the pgConn name, but the issue still occurs if I use tx from the callback. I've updated the example in the description. The issue is fixed by using the simple protocol in the SELECT query. We are just not sure if that's the correct fix. What if the SELECT needed parameter binding? Is that combination of queries unsupported in postgres?

isoos commented 2 days ago

The issue is fixed by using the simple protocol in the SELECT query. We are just not sure if that's the correct fix. What if the SELECT needed parameter binding? Is that combination of queries unsupported in postgres?

This sounds a bit strange, I would have expected that a simple protocol on the ALTER query would fix this. It may be worth to turn your example into a test case and debug it that way, I may be able to spend some time on it later today.

davidmartos96 commented 2 days ago

@isoos Thanks, I'll try to do that later today.

isoos commented 2 days ago

I'm at loss here, sent a question to the postgresql mailing list: https://www.postgresql.org/message-id/CALdQGguDi%2BPkz-Vd5HMr%2BER0L1%2BDQMsAh%2BOqbM13udF1RbLzcw%40mail.gmail.com

/cc @simolus3 if there is any idea about this issue

isoos commented 2 days ago

Good news: I now know that we don't close the portal, only the statement for a prepared message. I had a hacked fix of the client where the extended protocol works as expected, but it may take a bit more time to actually write a proper code out of it.

isoos commented 1 day ago

@davidmartos96: I've published a new version with the fix, and closing this now. Thank you for the detailed case description and test case!

davidmartos96 commented 1 day ago

@isoos Great news! Thank you for the prompt investigation and fix