Adding support for `pg-native` #4327

machuga · 2021-03-02T01:44:26Z

👋 Hello!

Thanks for all the work on this project, it has been a great help to our team.

Based on feedback from @tgriesser and #4062, I am submitting a small PR to allow for pg-native to be used.

Problem

Due to problems with the non-native driver holding open connections longer than necessary, and those in this thread, my team and I need to adopt the pg-native library. In production the difference is very noticeable, where our pool gets drained much quicker than it should and starts causing issues at scale (connections timing out, locking issues, session issues in AWS RDS).

Description

Based on feedback, I've created a new pgnative dialect that descends from the postgresql dialect, but loads in the native bindings and will use the updated callback signature for client.connect that is compatible with both the node-postgres and pg-native bindings.

Installing pg-native module in a project, then selecting this dialect will allow this strategy to work. It has the same configuration object as the normal postgresql dialect.

Example Configuration

const pg = require('knex')({
  client: 'pgnative',
  connection: process.env.PG_CONNECTION_STRING,
  searchPath: ['knex', 'public'],
});

Old Description Before Feedback - Discard, kept for posterity.

This PR allows for the pg-native library to be utilized in place of the standard pg bindings if either the NODE_PG_FORCE_NATIVE env var is set, or if the useNative config is set to true when passed into the strategy.

Example Configuration

const pg = require('knex')({
  client: 'pg',
  connection: process.env.PG_CONNECTION_STRING,
  searchPath: ['knex', 'public'],
  useNative: true
});

Note

This is my first PR to this code base so I am assuming not everything is ready to go. I've added an integration test configuration in a way that may not be ideal - I'm happy to tweak it to fit the situation better.

machuga · 2021-03-02T02:26:40Z

Noting Node 10 integration build failed due to pull request rate limit being hit. Hopefully it should pass after rate limits let off

elhigu · 2021-03-02T09:10:19Z

I would rather see this implemented as a separate npm module (in contributing.md there are instructions how to make dialects that are not in knex core), which contains also better tests checking that it works.

Otherwise this will start burdening knex more with bug reports, when people start using it and thinks that everything will work the same between pg / pg-native variants. That repository then will have some other people maintaining it and will be able to fix pg-native related issues of their side.

Also I haven't seen the actual problem anywhere stated what is your use case and exact problem where would you need to use pg-native instead of pg. That difference between parsing @brianc has been really active in removing performance differences between pg-native and pg versions and most probably this will end up deprecated pretty soon.

If need for pg-native does get lots of momentum and pg performance issues are impossible to fix, I would consider again if it is a good idea to have this after all in knex core and support it officially and include complete integrations test suites to be ran with it from knex core side.

machuga · 2021-03-02T11:38:58Z

Thanks for the quick response!

If we profile the standard node-postgres bindings, we find that we have extended ClientRead sections in our monitoring. It's similar to here: brianc/node-postgres#1774. Our nodes are already in the same data center, so the one comment in the thread doesn't apply. @brianc acknowledges it is a bug that he hasn't been able to track down yet, and many report success with moving to pg-native bindings.

When compared to our older service using a different stack to the same database (we are migrating away from this stack to the new stack with knex), the queries do not exhibit the same behavior.

When high quantities of writes happen concurrently that is when we see this issue crop up most notably.

elhigu · 2021-03-02T11:56:42Z

I do acknowledge the original issue and see why you like to use the pg-native. Though a clear testcase which demonstrates the problem (I know that brianc was able to somewhat demonstrate it by himself according to the linked issue) would probably be really useful. Even some docker based stress test example.

Though all the above is mostly irrelevant since the main point of my last comment was that:

Is there any reason why wouldn't you do this in a separate npm module and provide maintenance for it? It can be even published under knex organization in github if you wish.

This will start burdening knex maintainers in case if it is added to core and I don't believe that need for this is going to remain forever, which would also make it more suitable to reside in a separate npm package with own maintainers.

kibertoad · 2021-03-02T12:01:21Z

To be fair, we are not getting additional burden due to supporting mysql and mysql2, I can't remember a single case when we had to fix something specific to just one of them, and didn't have an issue that affected both in shared code.

If pg-native can be supported in a similar way as a completely drop-in replacement for pg that would pass all same tests, I don't see a reason not to include it.

machuga · 2021-03-02T12:13:28Z

Thanks for the guidance!

Yep, I can totally understand and appreciate the maintenance burden. A module would likely be sufficient - we've been running from a fork. I can look into it more today. If the decision goes the direction of favoring not having this in core we can go that direction.

I think some of the changes may want to be considered regardless if I have read the node-postgres docs correctly. The callback that I modified in https://github.com/knex/knex/pull/4327/files#diff-1fc6054df84862e1c3a2f9469a7a36e03d8b4afc522df61dc1ba48153b613598R82 looks to currently be defined in node-postgres as:

client.connect(callback: (err: Error) => void) => void

and this is also the same in pg-native.

Since the default postgresql strategy in knex uses node-postgres, the current use in trunk of:

client.connect(function (err, connection)

looks to be on a deprecated callback signature. Removing connection and favoring the client created with

const clientConnection = new client.driver.Client(client.connectionSettings);

seems like it would match their newest guidelines. I could 100% be reading some of this wrong, but that's what it seemed like to me on my initial digging.

From there I figured it was a small leap to load in the native driver since the major blocker (connection syntax) was removed.

elhigu · 2021-03-02T12:45:12Z

To be fair, we are not getting additional burden due to supporting mysql and mysql2, I can't remember a single case when we had to fix something specific to just one of them, and didn't have an issue that affected both in shared code.

I actually have resolved for example pooling issues, where mysql2 was a bit unstable not recognizing broken connections and probably with pg most of the open issues will be about streaming support and I don't think that mysql2 version should be supported be us either :)

When things are fixed in pg diealect, they still would be automatically fixed to external pg-native module, because it would be inherited from knex's pg dialect where all the fixes are.

Biggest thing with adding this to knex will be probably to add it to knex's integration tests to make sure that current cases (except for streaming) works as expected (no need to check query sql generation).

machuga · 2021-03-02T15:40:51Z

@elhigu Would it be acceptable for me to reduce my PR down to just adjusting the callback signature + remove the usePgNative flag? This would make the function signature compatible between the pg-native / pg versions while also aligning it to the documentation. This has the benefit of not explicitly calling out support for pg-native in knex which should prevent maintenance burden from growing. The PR would simply not prohibit the use of pg-native via the environment variable being included.

This also adds the benefit of thinking through first-class support at a different time.

Demonstration for same behavior: https://github.com/brianc/node-postgres/blob/25f658f227a1bcbe759423678a7ab4ba8e067994/packages/pg/lib/client.js#L279

elhigu · 2021-03-03T00:45:30Z

To me either way is the same. Both will add more features to knex core, when I would like to push knex to have more modular design with less features in core package and more pushed to additional modules.

If the community wants it in anyways, I suppose having the configure flag for it or implementing it as a completely separate dialect like mysql2 is better.

machuga · 2021-03-04T21:33:19Z

@elhigu Got it. So what are next steps? Should this PR be closed? Should we wait for additional community input?

kibertoad · 2021-03-04T21:35:20Z

@machuga I would say that if you can rework the PR in a way that follows the mysql/mysql2 style and is covered by CI, this PR can carry on. It's not even a new dialect per se, just exposing more of the driver we already use. And we've been relying on community contributions for the majority of fixes lately anyway.

machuga · 2021-03-04T21:50:08Z

Thanks @kibertoad! I'll get started down that path soon.

machuga · 2021-03-05T14:02:49Z

Alright the code has been updated to use a separate driver. I'm still looking around a bit to see if I missed any other places this code should be referenced, but I think it is fairly complete.

kibertoad · 2021-03-06T17:58:29Z

@machuga https://github.com/knex/knex/blob/master/.github/workflows/coverage.yml also needs to be updated.
Also we have a problem here, as https://github.com/knex/knex/blob/master/.github/workflows/integration-tests.yml does not execute pgnative currently; considering that we match db name to docker entry there, might be a bit of a problem; probably easiest solution would be to simply duplicate pg docker-compose entry as pgnative in https://github.com/knex/knex/blob/master/scripts/docker-compose.yml

kibertoad · 2021-03-06T17:59:10Z

Also documentation at https://github.com/knex/documentation needs to be updated

kibertoad · 2021-03-06T18:00:23Z

Also there is a bunch of if (knex.client.driverName === 'pg') still left in the code. Could you replace them with isPostgreSQL check?

machuga · 2021-03-08T01:31:34Z

Thanks for the guidance! You've already seen it, but for others: I have separated this out to #4356 to keep the extra helpers clean up separate from the pg-native work.

machuga · 2021-03-08T14:46:26Z

@machuga https://github.com/knex/knex/blob/master/.github/workflows/coverage.yml also needs to be updated.
Also we have a problem here, as https://github.com/knex/knex/blob/master/.github/workflows/integration-tests.yml does not execute pgnative currently; considering that we match db name to docker entry there, might be a bit of a problem; probably easiest solution would be to simply duplicate pg docker-compose entry as pgnative in https://github.com/knex/knex/blob/master/scripts/docker-compose.yml

@kibertoad Got it! I'll get these updated. I noticed that mysql2 can be passed in via DB, but the integration-tests.yml file doesn't specify loading an independent DB. Is that because mysql2 runs primarily under the integration2 suite? I was considering loading it up how mysql/mysql2 would load for consistency sake.
However, your way of creating a separate docker-compose entry may make the most sense if the tests would use the same container instance so we could avoid conflicting rows/flaky tests should they be run in parallel. If they use independent instances or will never run in parallel then we can ignore that situation.

machuga · 2021-03-20T12:34:17Z

Should be able to come back to this this weekend. Thanks for your patience!

machuga · 2021-04-11T13:48:21Z

Going back to some of the earlier comments, the streaming may not work, but I am looking into it.

kibertoad · 2021-07-12T20:55:26Z

@machuga Can I help you with finishing this?

machuga · 2021-07-14T01:59:01Z

Hi @kibertoad! Sure. I think I'll have a few questions regarding which feature I should disallow and how to reject them according to conventions. Thanks for reminding me to get back on it.

machuga · 2021-07-17T11:54:20Z

@kibertoad I had a design question pop up. The pg library returns error messages in the format error: TestError - syntax error at or near "TestError", while pg-native returns them as Error: TestError - syntax error at or near "TestError". In the tests I see it pop up, but I don't believe it'll interfere with user-based usage. Do you have any opinions on whether I need to be concerned about anything here? I can alter the test to accommodate case-insensitivity or I can work toward normalizing if this error bleeds out into user space consumption often and name parsing is expected.

Example: https://github.com/knex/knex/pull/4327/files#diff-ef380392f7c99984959538380cd3a01631d49da7eb768b02fd71695bfb2a3febR1123-R1129

The name here differs between the previously mentioned error or Error

lib/dialects/pgnative/index.js

kibertoad · 2021-07-17T12:02:24Z

@machuga I don't think we should be tinkering with driver output here, generally we prefer to stay out of driver's way. Case-insensitivity in tests sounds like a better approach.

machuga · 2021-07-17T16:18:40Z

lib/dialects/pgnative/index.js

+
+class Client_PgNative extends Client_PG {
+  _stream() {
+    console.log('HERE WE GO');


This will get rebased out after debugging complete.

@kibertoad since pg-native is incompatible with pg-query-stream I'll need to either:

Disable support for streaming as something like the below line where an exception gets thrown at invocation (though this method isn't the correct place from what I can tell, perhaps something in the execution lib)

Attempt to implement a stream that has the same feeling, but will not have the same response characteristics.

machuga · 2021-07-17T16:19:51Z

lib/dialects/pgnative/index.js

+  _escapeBinding2 = makeEscape({
+    escapeArray(val, esc) {
+      return esc(arrayString(val, esc));
+    },
+    escapeString(str) {
+      let hasBackslash = false;
+      let escaped = "'";
+      for (let i = 0; i < str.length; i++) {
+        const c = str[i];
+        if (c === "'") {
+          escaped += c + c;
+        } else if (c === '\\') {
+          escaped += c + c;
+          hasBackslash = true;
+        } else {
+          escaped += c;
+        }
+      }
+      escaped += "'";
+      if (hasBackslash === true) {
+        escaped = 'E' + escaped;
+      }
+      return escaped;
+    },
+    escapeObject(val, prepareValue, timezone, seen = []) {
+      if (val && typeof val.toPostgres === 'function') {
+        seen = seen || [];
+        if (seen.indexOf(val) !== -1) {
+          throw new Error(
+            `circular reference detected while preparing "${val}" for query`
+          );
+        }
+        seen.push(val);
+        return prepareValue(val.toPostgres(prepareValue), seen);
+      }
+      return JSON.stringify(val);
+    },
+  });
+}
+
+Object.assign(Client_PgNative.prototype, {
+  driverName: 'pgnative',
+  canCancelQuery: true,
+  _escapeBinding: makeEscape({
+    escapeArray(val, esc) {
+      return esc(arrayString(val, esc));
+    },
+    escapeString(str) {
+      let hasBackslash = false;
+      let escaped = "'";
+      for (let i = 0; i < str.length; i++) {
+        const c = str[i];
+        if (c === "'") {
+          escaped += c + c;
+        } else if (c === '\\') {
+          escaped += c + c;
+          hasBackslash = true;
+        } else {
+          escaped += c;
+        }
+      }
+      escaped += "'";
+      if (hasBackslash === true) {
+        escaped = 'E' + escaped;
+      }
+      return escaped;
+    },
+    escapeObject(val, prepareValue, timezone, seen = []) {
+      if (val && typeof val.toPostgres === 'function') {
+        seen = seen || [];
+        if (seen.indexOf(val) !== -1) {
+          throw new Error(
+            `circular reference detected while preparing "${val}" for query`
+          );
+        }
+        seen.push(val);
+        return prepareValue(val.toPostgres(prepareValue), seen);
+      }
+      return JSON.stringify(val);
+    },
+  }),
+});
+
+function arrayString(arr, esc) {
+  let result = '{';
+  for (let i = 0; i < arr.length; i++) {
+    if (i > 0) result += ',';
+    const val = arr[i];
+    if (val === null || typeof val === 'undefined') {
+      result += 'NULL';
+    } else if (Array.isArray(val)) {
+      result += arrayString(val, esc);
+    } else if (typeof val === 'number') {
+      result += val;
+    } else {
+      result += JSON.stringify(typeof val === 'string' ? val : esc(val));
+    }
+  }
+  return result + '}';
+}


Will rebase out and deduplicate between the two strategies next commit. Put here during experimentation.

machuga · 2021-08-22T12:51:47Z

@kibertoad I've opted for a faked-out stream (it will stream the results after they're fetched in order to keep the same interface as other drivers.

I have four timeout failures happening right now with the pg-native driver that I need to debug. I'm not sure the timeout support is available in pg-native yet so I need to verify.

kibertoad · 2021-08-22T13:07:14Z

Awesome, thank you for the update!

machuga · 2021-08-29T19:58:14Z

Query cancellation in pgnative must be handled via the cancel function on the pgnative/pg wrapper; processID odes not exist on queries returned from pgnative. This behavior needs to step into the pgnative implementation directly, skipping the wrapper, because the wrapper will not expose errors.

On clean runs this normally works, but there a few issues:

Somewhere, in the additional tests file there is a resource (or multiple) not being cleaned up.
The forShare() test in selects tests file hangs due to the timeout implementation. Sometimes it completes, sometimes it doesn't

Trying to work around it and properly debug

This enables pgnative to run in CI as well.

Using the newer syntax in the default postgres strategy and inheriting it into the pg-native. Also extracting connection creation in postgres dialect The `pg` library, both standard and `native` clients, offers the option to return a promise instead of callback when calling `connect`. The unit tests for `pg` were mocking out the `connect` call, but that relied on the deprecated call signature in `pg` standard client to work. The `_acquireOnlyConnection` call will now acquire the connection and connect to it without any of the additional configuration of the rest of `acquireRawConnection` to keep the encapsulation clean and make it easily mockable if necessary.

The stream is not a legit stream-as-you-go solution via following a cursor due to limitations of pg-native. Instead, to keep the interface consistent, this streams the results of the response after they're fetched.

lib/dialects/pgnative/index.js

kibertoad · 2021-09-05T15:19:05Z

lib/dialects/postgres/index.js

@@ -62,34 +62,40 @@ class Client_PG extends Client {
    return `"${value.replace(/"/g, '""')}"${arrayAccessor}`;
  }

+  _acquireOnlyConnection() {


Why was introduction of this method and modification of acquireRawConnection necessary if pgnative doesn't override either?

This goes back to connect using the deprecated (at least according to docs) callback signature where the same connection is returned: https://github.com/brianc/node-postgres/blob/master/packages/pg/lib/client.js#L279

The native client doesn't do this. The mocking was dependent on this behavior, which is now undocumented, so I moved it toward being compliant with the current node-postgres documentation + compatible with pg-native

https://github.com/brianc/node-postgres/blob/master/packages/pg/lib/native/client.js#L119-L134 native version for comparison.

lib/dialects/pgnative/index.js

kibertoad · 2021-09-05T18:31:06Z

Our notoriously flaky test is failing, any chance you could take a look at it?

machuga · 2021-09-05T19:25:56Z

Our notoriously flaky test is failing, any chance you could take a look at it?

Able to reproduce on my machine if I run it in isolation.

 Integration Tests
    postgresql | pgnative
NOTICE:  table "migration_test_1" does not exist, skipping
NOTICE:  table "migration_test_2" does not exist, skipping
NOTICE:  table "migration_test_2_1" does not exist, skipping
      knex.migrate.latest in parallel
Error: create table "knex_migrations" ("id" serial primary key, "name" varchar(255), "batch" integer, "migration_time" timestamptz) - duplicate key value violates unique constraint "pg_type_typname_nsp_index"
    at module.exports.Client._emitResult (./src/knex/node_modules/pg-native/index.js:173:26)
    at module.exports.Client._read (./src/knex/node_modules/pg-native/index.js:215:31)
    at PQ.emit (node:events:365:28) {
  severity: 'ERROR',
  code: '23505',
  messageDetail: 'Key (typname, typnamespace)=(knex_migrations_id_seq, 2200) already exists.',
  schema: 'pg_catalog',
  table: 'pg_type',
  constraint: 'pg_type_typname_nsp_index',
  file: 'nbtinsert.c',
  line: '649',
  routine: '_bt_check_unique'
}
[
  1,
  [ '20131019235242_migration_1.js', '20131019235306_migration_2.js' ]
]

This is what I get from the promise results in this area. Digging in a bit further.

Moving to constructor assignment for driverName nad cancelability

machuga · 2021-09-05T21:13:03Z

Okay the error that came up was not the same, looked like it was something triggered by running in isolation only. Tests passing now 👍

kibertoad · 2021-09-06T09:57:11Z

scripts/stress-test/docker-compose.yml

@@ -31,6 +33,14 @@ services:
      - POSTGRES_PASSWORD=postgresrootpassword
      - POSTGRES_USER=postgres

+  pgnative:
+    image: mdillon/postgis


what's the advantage of using this image over vanilla pg?

I'm not too sure if there are technical advantages, but the postgresql strategy uses it above on line 27. I assumed there was a good reason for it. For benefit of pg-native development, it makes debugging easier by ensuring there aren't any odd bugs because of configuration issues across two different images.

kibertoad · 2021-09-06T13:04:39Z

Thank you! It was an epic journey.

machuga · 2021-09-06T13:16:11Z

Thanks for your continuous help and patience! It's been fun. 🙌

Pyrolistical · 2023-09-15T22:29:03Z

Why is the dialect pgnative and not pg-native? This violates the principle of least surprise.

Also, pgnative does not appear in documentation. I've created an issue for that #5997

timedtext · 2023-09-16T02:20:20Z

Does this driver support CockroachDB?

kibertoad added missing documentation PostgreSQL labels Mar 6, 2021

machuga mentioned this pull request Mar 7, 2021

Unify db test helpers #4356

Merged

kibertoad added the needs rebasing label Mar 13, 2021

kibertoad mentioned this pull request May 12, 2021

CockroachDB Support #2002

Closed

kibertoad reviewed Jul 17, 2021

View reviewed changes

lib/dialects/pgnative/index.js Outdated Show resolved Hide resolved

machuga commented Jul 17, 2021

View reviewed changes

machuga added 5 commits September 5, 2021 09:18

Adding support for pgnative

6ae53f0

This enables pgnative to run in CI as well.

Updating tests to work for pg-native

0504559

Adding pseudo-streaming support for pgnative

9b9ff0c

The stream is not a legit stream-as-you-go solution via following a cursor due to limitations of pg-native. Instead, to keep the interface consistent, this streams the results of the response after they're fetched.

Allowing pgnative query cancellation and timeouts

1980934

kibertoad reviewed Sep 5, 2021

View reviewed changes

lib/dialects/pgnative/index.js Outdated Show resolved Hide resolved

kibertoad reviewed Sep 5, 2021

View reviewed changes

lib/dialects/pgnative/index.js Show resolved Hide resolved

Removing object assignment in pgnative

6d27cbb

Moving to constructor assignment for driverName nad cancelability

machuga mentioned this pull request Sep 5, 2021

Adding notes about pg-native knex/documentation#336

Merged

kibertoad removed missing documentation needs rebasing labels Sep 6, 2021

kibertoad reviewed Sep 6, 2021

View reviewed changes

kibertoad approved these changes Sep 6, 2021

View reviewed changes

kibertoad merged commit 01cfa98 into knex:master Sep 6, 2021

OlivierCavadenti pushed a commit to AbeonaPascha/knex that referenced this pull request Nov 4, 2021

Adding support for pg-native (knex#4327)

ea8e0a1

stephenh mentioned this pull request Nov 22, 2021

Investigate knex pg-driver support joist-orm/joist-orm#185

Closed

Pyrolistical mentioned this pull request Sep 15, 2023

Missing reference to client: 'pgnative' #5997

Open

Adding support for pg-native #4327

Adding support for pg-native #4327

Conversation

machuga commented Mar 2, 2021 • edited

Problem

Description

Example Configuration

Old Description Before Feedback - Discard, kept for posterity.

Example Configuration

Note

machuga commented Mar 2, 2021

elhigu commented Mar 2, 2021 • edited

machuga commented Mar 2, 2021

elhigu commented Mar 2, 2021

kibertoad commented Mar 2, 2021

machuga commented Mar 2, 2021 • edited

elhigu commented Mar 2, 2021 • edited

machuga commented Mar 2, 2021 • edited

elhigu commented Mar 3, 2021 • edited

machuga commented Mar 4, 2021

kibertoad commented Mar 4, 2021

machuga commented Mar 4, 2021

machuga commented Mar 5, 2021

kibertoad commented Mar 6, 2021 • edited

kibertoad commented Mar 6, 2021

kibertoad commented Mar 6, 2021 • edited

machuga commented Mar 8, 2021

machuga commented Mar 8, 2021

machuga commented Mar 20, 2021

machuga commented Apr 11, 2021

kibertoad commented Jul 12, 2021

machuga commented Jul 14, 2021

machuga commented Jul 17, 2021 • edited

kibertoad commented Jul 17, 2021 • edited

machuga Jul 17, 2021

Choose a reason for hiding this comment

machuga Jul 17, 2021

Choose a reason for hiding this comment

machuga commented Aug 22, 2021

kibertoad commented Aug 22, 2021

machuga commented Aug 29, 2021 • edited

kibertoad Sep 5, 2021

Choose a reason for hiding this comment

machuga Sep 5, 2021

Choose a reason for hiding this comment

machuga Sep 5, 2021

Choose a reason for hiding this comment

kibertoad commented Sep 5, 2021

machuga commented Sep 5, 2021

machuga commented Sep 5, 2021

kibertoad Sep 6, 2021

Choose a reason for hiding this comment

machuga Sep 6, 2021

Choose a reason for hiding this comment

kibertoad commented Sep 6, 2021

machuga commented Sep 6, 2021

Pyrolistical commented Sep 15, 2023

timedtext commented Sep 16, 2023

Adding support for `pg-native` #4327

Adding support for `pg-native` #4327

machuga commented Mar 2, 2021 •

edited

elhigu commented Mar 2, 2021 •

edited

machuga commented Mar 2, 2021 •

edited

elhigu commented Mar 2, 2021 •

edited

machuga commented Mar 2, 2021 •

edited

elhigu commented Mar 3, 2021 •

edited

kibertoad commented Mar 6, 2021 •

edited

kibertoad commented Mar 6, 2021 •

edited

machuga commented Jul 17, 2021 •

edited

kibertoad commented Jul 17, 2021 •

edited

machuga commented Aug 29, 2021 •

edited