Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: clarification on minimal example with full error handling #3328

Open
satire-foxfire opened this issue Oct 9, 2024 · 4 comments
Open
Labels

Comments

@satire-foxfire
Copy link

satire-foxfire commented Oct 9, 2024

The docs somewhat talk about it in pieces, but it wasn't 100% clear what a minimal code example with full error handling looks like, so I was hoping to get some clarity around that. I believe the following are both minimal examples, but I have a few questions surrounding them.

Client example:

import pg from 'pg';

const client = new pg.Client();

await client.connect();

try {
  await client.query('SELECT $1::text as message', ['Hello world!']);
} finally {
  await client.end();
}

Pool example:

import pg from 'pg';

const pool = new pg.Pool();

try {
  await pool.query('SELECT $1::text as message', ['Hello world!']);
} finally {
  await pool.end();
}

My couple questions:

  1. Does either example need an actual event listener for error, or because I'm calling connect -> query -> end directly after each other, is it not possible for an error event to actually be emitted? I know in normal case errors that happen during those are rejected with the promise, but I wasn't sure if it was actually possible for an error event to be emitted between say the connect and query calls, etc.
  2. Is there any advantage or disadvantage to using Pool even if it's only a single call and will then be torn down? The only advantage I can see is it requires one less line of code since you don't need to explicitly call connect, but I didn't know if there was other overhead or considered an antipattern*?

* I know instantiating a bunch of pools is an antipattern, but think of a usecase more akin to lambda or something, where it's not possible to keep a pool active, so I'm basically just spinning up some code, running a single query, then spinning everything down. Does using Pool have any negatives for that use case, or does defaulting to Pool over Client make sense?

Thanks for any info/help!

@charmander
Copy link
Collaborator

Does either example need an actual event listener for error[…]? […] I wasn't sure if it was actually possible for an error event to be emitted between say the connect and query calls, etc.

Yes, in the current version of pg, both examples need error listeners (even if they’re just no-ops) to avoid crashing on connection-level errors that happen after the initial connection. You also probably want to put the connect call inside the try.

Is there any advantage or disadvantage to using Pool even if it's only a single call and will then be torn down?

It’s perfectly fine to use Pool for short-lived single-client applications, and I’d recommend it in many situations, especially if you want to recover from connection-level errors at all. The main downside (again, in the current version – improvements possible?) is that it’s back to being as complicated as Client and then some if you ever have to use pool.connect() instead of pool.query() – an error listener has to be attached on acquire and detached on release, or attached persistently on the pool’s connect event, and of course you need to ensure release is always called (with a truthy argument as appropriate when a client can’t be reused because of an error).

@satire-foxfire
Copy link
Author

@charmander Thanks for the response, just a quick follow-up / double confirmation. The docs say:

When the client is in the process of connecting, dispatching a query, or disconnecting it will catch and forward errors from the PostgreSQL server to the respective client.connect client.query or client.end promise ...

Are you saying the following is possible?

await client.connect();

// an error could be emitted here on the client that would NOT be forwarded to either `connect` or `query`?

try {
  await client.query('SELECT $1::text as message', ['Hello world!']);

  // or an error could be emitted here after query but before end is called?
} finally {
  await client.end();
}

I assumed since we're calling connect, query, and then end sequentially, it wouldn't be possible for an error event to not be forwarded to one of those three calls, but it appears you're saying it is possible to have something not forwarded but actually emitted between those 3 calls?

You also probably want to put the connect call inside the try.

Are you saying it's possible for the connect call to fail, while still needing to call end? Assume this entire block is in a try/catch for errors in general, I just left that out, but the try/finally I included was specifically to ensure the client is always closed properly. Are we supposed to call end even if connect fails?

@charmander
Copy link
Collaborator

Your “here” and “between” might not be pedantically correct (it’s more like “during” the await), but essentially, yes. Also, even when a query is in progress, the decision to emit an error event separately from the rejection for some types of errors is intentional.

Are you saying it's possible for the connect call to fail, while still needing to call end?

No. Catching errors for the entire block is fine.

@satire-foxfire
Copy link
Author

satire-foxfire commented Oct 10, 2024

Yeah, I was oversimplifying with the before/after and here/between :D but the key thing was not realizing it was possible for some things to be emitted no matter what, I assumed there was a possible way to write the code that guaranteed they would come through as rejected promises, but that isn't the case, so an error listener is always required no matter what.

And 👍 for the second part.

That was everything, feel free to close this out if you want. If I get some spare time, I might try to send a tiny PR just to make that a little more explicit it in the docs if you're open to it, otherwise that clears everything up for me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants