Add pg-query-stream module #2035

brianc · 2019-12-20T18:33:59Z

This uses git subtree as awesomely recommended by @charmander to pull in the history of the pg-query-stream into this module. As was done with #2030 I haven't used lerna to manage sub-module dependencies between this & pg-cursor yet. I'll do that in a follow on PR.

important: need to use merge commit button here to merge this, not squash.

Consider a system where one component is scheduling tasks that yield streams, and passing them to (unknown) clients for consumption. It would be useful for the scheduler to know that the query underlying the stream is completed (so it can continue on to it's next task) without having to wait for the consumer to finish reading all results.

closes #2

Emit 'close' events when query completes

pg-cursor no longer returns the empty array 'done' signal to the callback until the cursor recieves a readyForQuery message. This means pg-query-stream will not emit 'close' or 'end' events until the server is __truly__ ready for the next query. This fixes some race-conditions where some queries are triggered off of the `end` event of the query-stream closes #3

- appears that timestamp queries emit a lot of `rows` with length == 0 - `self.once('end')` is added each of these times - assertion on listener count shows that more than 10 listeners are applied

- moves 'end' event listener to constructor, only listen once - ensures all existing tests still green

maxListeners on timestamp queries

This includes fixes in [email protected]. I've relaxed semver a touch so I don't have to release a new version here just for patch changes to pg-cursor.

Bumps [eslint](https://github.com/eslint/eslint) from 4.4.0 to 4.18.2. - [Release notes](https://github.com/eslint/eslint/releases) - [Changelog](https://github.com/eslint/eslint/blob/master/CHANGELOG.md) - [Commits](eslint/eslint@v4.4.0...v4.18.2) Signed-off-by: dependabot[bot] <[email protected]>

* Bump version of pg-cursor This includes fixes in [email protected]. I've relaxed semver a touch so I don't have to release a new version here just for patch changes to pg-cursor. * Pass options to pg-cursor fixes #55

…d87bfc9035e487e8' git-subtree-dir: packages/pg-query-stream git-subtree-mainline: cccf84e git-subtree-split: 9ced05e

packages/pg-packet-stream/dist/BufferReader.js

Delete accidental addition

brianc · 2019-12-20T20:10:47Z

looks like there's a flaky test on node 8 I'll need to take a look at

brianc · 2019-12-20T21:05:09Z

oh wasn't a flaky test it was just running tests from all packages in parallel mode isn't gonna work since they use a postgres instance and some of the tests in the main package cancel all running queries and the like. I was wondering why it was saying "command terminated due to administrative command." Now I know...

brianc · 2019-12-23T16:49:05Z

Thanks for the review!

matthieusieben · 2019-12-25T15:14:02Z

packages/pg-query-stream/test/async-iterator.es6

+    const query = client.query(stream)
+    const iteratorRows = []
+    for await (const row of query) {
+      iteratorRows.push(row)


You should make:

a test that breaks in the first iteration

a test that triggers two breaking loops

a test that await new Promise(r => setTimeout(r)) in the loop body

These tests will fail due to the poor implementation of pg-query

Refer to brianc/node-pg-query-stream#52 (comment) for an alternative implementation

matthieusieben · 2019-12-28T10:59:45Z

You really shouldn't have just copied the code from brianc/node-pg-query-stream as it is an invalid implementation of the Readable interface. Using the current implementation breaks the pg connection when used with async iterables.

brianc · 2019-12-28T16:49:53Z

You really shouldn't have just copied the code from brianc/node-pg-query-stream as it is an invalid implementation of the Readable interface. Using the current implementation breaks the pg connection when used with async iterables.

Ohhh yeah I wanted to keep the operations discrete: first merge the old repo as is just to keep it contained in a single step, then implement your changes, add tests, release a new version, then convert to typescript, etc....versus doing it all in 1 big bang as it's harder to follow that way for me. Thanks for reminding me though, I'll get to fixing the stream right now!

what do you mean by

a test that triggers two breaking loops?

There were some subtle behaviors with the stream being implemented incorrectly & not working as expected with async iteration. I've modified the code based on #2050 and comments in #2035 to have better test coverage of async iterables and update the internals significantly to more closely match the readable stream interface. Note: this is a __breaking__ (semver major) change to this package as the close event behavior is changed slightly, and `highWaterMark` is no longer supported. It shouldn't impact most usage, but breaking regardless.

matthieusieben · 2019-12-28T17:29:31Z

for await (const r of getNewStream()) break
for await (const r of getNewStream()) break

With each stream running a query that results in more than batchSize entries.

(I am on mobile, sorry for the poor example)

brianc · 2019-12-28T17:33:10Z

no worries! appreciate the help! its my wifes birthday today so prolly wont have time to finish this until tomorrow but i put up a PR and i think i covered that case with a test!

…

On Sat, Dec 28, 2019 at 11:29 AM Matthieu Sieben ***@***.***> wrote: for await(const r of new stream) breakfor await (const r of new stream) break With each stream running a query that results in more than batchSize entries. (I am on mobile, sorry for the poor example) — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#2035?email_source=notifications&email_token=AAAMHIOHHR2FA2XPRW6OFETQ26EHZA5CNFSM4J6BJF6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHYOQLQ#issuecomment-569436206>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAMHIM5R74DOR4KU6KDPLDQ26EHZANCNFSM4J6BJF6A> .

* Fix pg-query-stream There were some subtle behaviors with the stream being implemented incorrectly & not working as expected with async iteration. I've modified the code based on #2050 and comments in #2035 to have better test coverage of async iterables and update the internals significantly to more closely match the readable stream interface. Note: this is a __breaking__ (semver major) change to this package as the close event behavior is changed slightly, and `highWaterMark` is no longer supported. It shouldn't impact most usage, but breaking regardless. * Remove a bunch of additional code * Add test for destroy + error propagation * Add failing test for destroying unsubmitted stream * Do not throw an uncatchable error when closing an unused cursor

brianc and others added 30 commits October 21, 2013 23:57

Initial commit

722296f

Remove bad code

5400dfe

Bump version

cc20d98

Fix relative load path for pg.js

48687fc

Bump version

aec85ce

Add in proper error handling

1b249e9

Bump version

5888135

Update npm test command

278c5ce

Add ability to configure highWaterMark and batchSize

33be525

Bump version

0ebd4c3

Add cleanup to the example

0df516c

closes #2

Merge pull request #1 from grncdr/patch-1

0ed940c

Emit 'close' events when query completes

Port tests to use mocha

2909062

Remove Makefile

f8f2a92

Add test for stream close & satisfy stream contract

b66be5e

Bump version

122bcfb

Rebase code on top of pg-cursor

37de9c2

Bump version

0a7da37

Bump version

0f13c80

adds failing test

87b52f9

- appears that timestamp queries emit a lot of `rows` with length == 0 - `self.once('end')` is added each of these times - assertion on listener count shows that more than 10 listeners are applied

passes stream-tester-timestamp

cab956b

- moves 'end' event listener to constructor, only listen once - ensures all existing tests still green

Merge pull request #4 from tbuchok/master

d82386e

maxListeners on timestamp queries

Bump version

7593a44

fix up tests

41b7d7d

clean ups

fec0909

Update version of pg-cursor

e242b94

Bump version

99fe666

merge it

6763e09

brianc and others added 6 commits October 30, 2019 13:03

Bump version of pg-cursor (#64)

3e59f28

This includes fixes in [email protected]. I've relaxed semver a touch so I don't have to release a new version here just for patch changes to pg-cursor.

Pass options to cursor (#65)

08072a9

* Bump version of pg-cursor This includes fixes in [email protected]. I've relaxed semver a touch so I don't have to release a new version here just for patch changes to pg-cursor. * Pass options to pg-cursor fixes #55

Bump version

9ced05e

Add 'packages/pg-query-stream/' from commit '9ced05e8aab65f3fdf1a67ad…

db1b95e

…d87bfc9035e487e8' git-subtree-dir: packages/pg-query-stream git-subtree-mainline: cccf84e git-subtree-split: 9ced05e

Run gitsubtree merge

8607302

brianc assigned brianc and charmander and unassigned brianc and charmander Dec 20, 2019

brianc requested a review from charmander December 20, 2019 19:28

charmander requested changes Dec 20, 2019

View reviewed changes

packages/pg-packet-stream/dist/BufferReader.js Outdated Show resolved Hide resolved

Unify lint

ef2f2d2

Delete accidental addition

brianc force-pushed the bmc/add-pg-query-stream branch from 084b1a2 to ef2f2d2 Compare December 20, 2019 19:47

Do not run tests in parallel

6b7b8d1

Delete files which are no longer needed as they exist in the monorepo

fdae851

charmander approved these changes Dec 21, 2019

View reviewed changes

brianc merged commit bd3efaa into master Dec 23, 2019

brianc deleted the bmc/add-pg-query-stream branch December 23, 2019 16:49

matthieusieben reviewed Dec 28, 2019

View reviewed changes

brianc mentioned this pull request Dec 28, 2019

Fix pg-query-stream implementation #2051

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pg-query-stream module #2035

Add pg-query-stream module #2035

brianc commented Dec 20, 2019

brianc commented Dec 20, 2019

brianc commented Dec 20, 2019

brianc commented Dec 23, 2019

matthieusieben Dec 25, 2019

matthieusieben Dec 28, 2019 •

edited

Loading

matthieusieben commented Dec 28, 2019 •

edited

Loading

brianc commented Dec 28, 2019

matthieusieben commented Dec 28, 2019 •

edited

Loading

brianc commented Dec 28, 2019 via email

Add pg-query-stream module #2035

Add pg-query-stream module #2035

Conversation

brianc commented Dec 20, 2019

brianc commented Dec 20, 2019

brianc commented Dec 20, 2019

brianc commented Dec 23, 2019

matthieusieben Dec 25, 2019

Choose a reason for hiding this comment

matthieusieben Dec 28, 2019 • edited Loading

Choose a reason for hiding this comment

matthieusieben commented Dec 28, 2019 • edited Loading

brianc commented Dec 28, 2019

matthieusieben commented Dec 28, 2019 • edited Loading

brianc commented Dec 28, 2019 via email

matthieusieben Dec 28, 2019 •

edited

Loading

matthieusieben commented Dec 28, 2019 •

edited

Loading

matthieusieben commented Dec 28, 2019 •

edited

Loading