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

add connect event (callback?) for the pool that you can use to run custom commands on a connection prior to it being made available to app code #312

Open insinfo opened 6 months ago

insinfo commented 6 months ago

add a way to run queries before making the connection available, so that it is possible to run queries like set search_path to "schemaName" and SET client_encoding = 'utf8' I'm having problems using the Pool without this feature

insinfo commented 6 months ago

in node-pg-pool there is something like this

pool.on('connect', (client) => {client.query('set search_path...');});

https://github.com/brianc/node-pg-pool

isoos commented 6 months ago

Is it fair to say that these SET parameters may go into ConnectionSettings and should be applied when we are creating the connection, regardless of pool or non-pool? Some may also go into SessionSettings?

I don't mind a callback, but the examples provided feel more like settings. What do you think?

insinfo commented 6 months ago

I think putting these options in SessionSettings would be more difficult to implement because postgresql allows a considerable amount of configuration options at runtime, having the ability to run queries with a callback would perhaps be easier to implement and would be more flexible, but perhaps Could find a middle ground between putting the most used options like 'search_path' , 'datestyle', 'TIME ZONE', 'Client Encoding' in addition to having a way to pass a query to be executed, that would be very good.

From what I saw in the npgsql driver you can pass these settings through the connection string

"ConnectionStringPostgreSqlProvider": "User ID=xxx;Password=xxx;Host=localhost;Port=5432;Database=myDb;SearchPath=idsvr4config,public;Pooling=true;",

https://www.npgsql.org/doc/connection-string-parameters.html https://www.postgresql.org/docs/current/runtime-config.html https://www.postgresql.org/docs/current/sql-set.html

isoos commented 6 months ago

I think the place of setting should reflect the expectation of the scope. I think we could make it work with run/runTx or even with individual execute calls, but it would need to run SET+RESET operations on each boundaries. It would add a non-trivial overhead, and scope nesting wouldn't work, because RESET <x> and RESET ALL won't restore the session-beginning value, rather it will restore the database-configured default(s).

I think the good balance here is to set these settings on the connection initialization (being in ConnectionSettings), and let the user override them without library support (assuming they know why and what they are doing).

This could be a simple Map<String, String> or a typed object, not yet sure which one is more preferable.

insinfo commented 6 months ago

I think the good balance here is to set these settings on the connection initialization (being in ConnectionSettings), and let the user override them without library support (assuming they know why and what they are doing).

I completely agree, I think the Map<String, String> option is good

isoos commented 6 months ago

After a bit back-and-forth, I'm backing out of my suggestion of Map-based settings, as it may become way too difficult to generalize it for e.g. cockroachdb's cluster-level variables. I also wanted to make it nice enough for named/typed variables, but that also complicates it further.

Instead, I'm suggesting to go ahead with a rather simple onOpen callback as implemented here: https://github.com/isoos/postgresql-dart/pull/313

@insinfo: what do you think?

insinfo commented 6 months ago

@isoos looks good to me, congratulations on the work