remove slonik, insert postgres.js #564

issa-tseng · 2022-08-04T22:54:29Z

No description provided.

* 1472 passing tests, 209 pending.

* porsagres does not support multiple portals at once, so we have to serialize what were previously parallel streams. * we bottleneck at the csv => zip output anyway so it doesn't matter. * using the callback interface and abusing the Promise lock to create a backpressure mechanism.

* the error gets passed all the way out now on .begin as opposed to .transaction before, so it's both a nice place to handle it as well as a necessary error to handle or the tests crash.

* see comments in tap.js for details.

matthew-white · 2022-08-04T23:26:35Z

I'm surprised it's not a larger diff! It's nice that there are a few similarities between postgres.js and Slonik.

issa-tseng · 2022-08-19T20:39:58Z

lib/external/postgres.js

+// including this file, may be copied, modified, propagated, or distributed
+// except according to the terms contained in the LICENSE file.
+
+// CRCRCR uhhhh confusing naming maybe idk


issa-tseng · 2022-08-19T20:42:56Z

lib/external/postgres.js

+// and p/postgres is happy to mix template literals across "connections".
+const sql = _postgres(undefined, options);
+
+module.exports = { postgres, sql, options };


issa-tseng · 2022-08-19T20:45:11Z

lib/util/db.js

-  return (v === null) ? null :
-    (v === undefined) ? null :
-    ((typeof v === 'object') && (v.constructor === Object)) ? JSON.stringify(v) : // eslint-disable-line indent
-    (v instanceof Date) ? v.toISOString() : // eslint-disable-line indent


should i check for tz nastiness?

issa-tseng · 2022-08-19T20:53:31Z

lib/util/db.js

 //
-// but also, they do seem a bit goofy and if anyone has a cleaner idea please give
-// it a try and a pull request.
+// CRCRCR: but actually maybe we can get rid of .map now and clean up the joiner call..


issa-tseng · 2022-08-19T20:55:33Z

lib/util/db.js

-  obj.one = (s) => db.one(s);
+  obj.q = (s) => db`${s}`;
+
+  obj.one = (s) => db`${s}`.then(([ x ]) => x); // CRCRCR: slonik used to Enforce existence, do we care?


issa-tseng · 2022-08-19T21:43:45Z

test/integration/setup.js

+const testService = (test) => () =>
+  baseContainer.transacting((container) =>
+    test(augment(request(service(container))), container).then(rollback(resolve), rollback(reject))
+  ).catch((f) => f());


call this g?

issa-tseng · 2022-08-19T21:48:27Z

test/integration/setup.js

-});
+    try {
+      return test(task._container).then(rollback(resolve), rollback(reject));
+    } catch(e) {


issa-tseng · 2022-08-19T21:58:17Z

test/util/sql.js

+const _value = (x, string, parameters, types) => {
+  if (x instanceof Builder) return x.build(string, parameters, types, options);
+  else if (x instanceof Identifier) return x.value;
+  else if (x instanceof Query) return reduceFragment(x, types, parameters).string;


unbackwards?

issa-tseng · 2022-08-19T21:59:11Z

test/util/sql.js

+  else if (x instanceof Identifier) return x.value;
+  else if (x instanceof Query) return reduceFragment(x, types, parameters).string;
+  else {
+    const value = (x?.value == null) ? x : x.value;


is this necessary?

issa-tseng · 2022-08-19T21:59:29Z

test/util/sql.js

+
+const options = { transform: { undefined: null, column: {} } };
+const inferType = (x) =>
+  //x instanceof Parameter ? x.type :


matthew-white · 2022-09-12T20:39:18Z

I just followed the repro steps in getodk/central#482 in order to see whether getodk/central#604 fixed that issue. However, I forgot to checkout getodk/central#604 first, so I ended up running the repro steps against this PR. 😅 The result was interesting though, so I thought I'd note what I saw.

In good news, I'm no longer observing a database connection leak. 🎉 After I followed the repro steps, the pg_stat_activity count did not increase.

I ran the repro steps several times. Every time that I checked the number of successful requests, 8/10 requests were successful. That's interesting to me, because with Slonik today, it's 9/10. From a quick look, it seems like postgres.js defaults to allowing up to 10 database connections, the same as Slonik, so that doesn't seem to explain the difference.

I queried pg_stat_activity during the repro steps, running select count(*) from pg_stat_activity where state <> 'idle'. From what I can tell, the count spikes to 9, quickly decreases to 8, then continues to decrease. I'm surprised not to see a count of more like 10 or 11, which is what I'd expect if postgres.js were using 10 connections.

The requests that don't succeed return with a variety of errors. Usually it's a 500 or a 504, but I've also seen some 502s. When it's a 500, Backend is returning an error response whose message property starts with "Completely unhandled exception". All the 504s and 502s I checked were HTML responses from nginx.

The specific error details also varied. For 500s, those details showed up in the error response. In other cases, they showed up in Sentry. The two most common errors were:

PostgresError: portal "" does not exist (Sentry [1][2][3])
RangeError: The value of "offset" is out of range. … (Sentry [1][2])

I also saw these 1 or 2 times:

TypeError: query.cursorFn is not a function (Sentry)
TypeError: constructors[k] is not a function (Sentry)
SyntaxError: Unexpected number in JSON at position 4 (Sentry)

(5) happened when I tried the repro steps with 20 concurrent requests instead of 10. I'm pretty sure that's true of (3) as well, and that might also be true of (4). Interestingly, when I tried 20 requests, there was also one time when I received a 404.1 Problem response ("Could not find the resource you were looking for."). There were also times when curl exited with an exit code of 6, displaying the message "Could not resolve host: test.getodk.cloud". That said, sending 20 concurrent requests resulted in more successful responses than sending 10 requests. (The count of successful requests seemed to vary. I saw 10 once.)

I have a theory about when a 500 is returned vs. a 504. Errors (1) and (2) above were thrown for stream, maybeOne, and oneFirst. I think I'm seeing a pattern that when maybeOne and oneFirst throw the error, it's a 500, while when it's stream, it's a 504.

matthew-white · 2022-09-13T02:08:10Z

@lognaturel also noted on Slack:

For what it's worth, I was disrupted at least once right when Sentry got that portal error. I was attempting to publish a form draft in one tab and had another form’s submission page open in another. The publish request seemed to hang. I got a spinner on the modal. I got impatient and killed the window. When I came back the publish had succeeded.

I think this would be expected if the publish request was sent while all available database connections were in use by the stream requests. With Slonik, a connection attempt times out in 5 seconds, so you wouldn't expect the request to hang, but with postgres.js, it looks like the timeout is 30 seconds.

matthew-white · 2022-10-10T01:59:57Z

lib/external/postgres.js

+    }
+  }
+};
+const postgres = (config) => _postgres(connectionString(config), options);


I'm not sure, but I think we'll need to do something more here to support SSL connections, which we currently support. From a quick glance, it looks like Slonik and postgres.js accept SSL options differently. For Slonik, we pass ?ssl=true as part of the connection string. However, the only mention of SSL that I see in the postgres.js readme has to do with an ssl option that's specified apart from the connection string.

#433 is about Slonik, but I think has some relevant discussion.

matthew-white · 2023-08-15T23:04:36Z

postgres.js branch that we could try to see if it fixes the issues we were seeing: https://github.com/porsager/postgres/tree/concurrent-cursors

issa-tseng · 2023-08-16T06:22:35Z

no, it doesn’t, don’t bother, it won’t. this is shared as a possible starting point for us implementing this ourselves. On Aug 15, 2023, at 16:04, Matthew White ***@***.***> wrote: postgres.js branch that we could try to see if it fixes the issues we were seeing: https://github.com/porsager/postgres/tree/concurrent-cursors —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>

lognaturel · 2024-03-15T18:17:48Z

Closing in favor of tracking issue at #1119

issa-tseng added 4 commits July 25, 2022 19:27

wip: most of porsagres port complete. only streaming problems left.

932d273

* 1472 passing tests, 209 pending.

test: fix last two tests from porsagres refactor.

1326460

* the error gets passed all the way out now on .begin as opposed to .transaction before, so it's both a nice place to handle it as well as a necessary error to handle or the tests crash.

improve: formalize tap and allow it to take prefix params.

0169e55

* see comments in tap.js for details.

issa-tseng commented Aug 19, 2022

View reviewed changes

issa-tseng requested a review from ktuite August 19, 2022 22:00

ktuite approved these changes Aug 19, 2022

View reviewed changes

issa-tseng added 2 commits August 21, 2022 23:46

Merge remote-tracking branch 'origin/master' into issa/porsagres

eedfd26

i cant

4f4a2e7

matthew-white mentioned this pull request Sep 8, 2022

10 concurrent requests for OData feed results in database connection leak #482

Closed

matthew-white mentioned this pull request Sep 15, 2022

Create streams only after Promise.all() #608

Closed

2 tasks

issa-tseng added 4 commits October 3, 2022 16:22

REVERTME: convenience commit for quick reproing.

dfcef3d

REVERTME: committing some debug logging for wrong-response error.

240ca85

rm: remove some already commented dead code.

ce225b5

deps: upgrade porsagres to 3.3.1

2c63809

This was referenced Oct 4, 2022

Immediately destroy stream created in rejected Promise.all() #634

Closed

Destroy streams created in rejected Promise.all() #609

Closed

matthew-white reviewed Oct 10, 2022

View reviewed changes

matthew-white added this to the v2023.1 milestone Nov 16, 2022

matthew-white removed this from the v2023.1 milestone Jan 24, 2023

lognaturel marked this pull request as draft February 27, 2023 17:45

lognaturel mentioned this pull request Mar 27, 2024

Get off of slonik fork #1119

Open

lognaturel closed this Mar 15, 2024

matthew-white deleted the issa/porsagres branch May 24, 2024 22:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove slonik, insert postgres.js #564

remove slonik, insert postgres.js #564

issa-tseng commented Aug 4, 2022

matthew-white commented Aug 4, 2022

issa-tseng Aug 19, 2022

issa-tseng Aug 19, 2022

issa-tseng Aug 19, 2022

issa-tseng Aug 19, 2022

issa-tseng Aug 19, 2022

issa-tseng Aug 19, 2022

issa-tseng Aug 19, 2022

issa-tseng Aug 19, 2022

issa-tseng Aug 19, 2022

issa-tseng Aug 19, 2022

matthew-white commented Sep 12, 2022 •

edited

Loading

matthew-white commented Sep 13, 2022

matthew-white Oct 10, 2022

matthew-white Oct 10, 2022

matthew-white commented Aug 15, 2023

issa-tseng commented Aug 16, 2023 via email

lognaturel commented Mar 15, 2024

remove slonik, insert postgres.js #564

remove slonik, insert postgres.js #564

Conversation

issa-tseng commented Aug 4, 2022

matthew-white commented Aug 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-white commented Sep 12, 2022 • edited Loading

matthew-white commented Sep 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-white commented Aug 15, 2023

issa-tseng commented Aug 16, 2023 via email

lognaturel commented Mar 15, 2024

matthew-white commented Sep 12, 2022 •

edited

Loading