Skip to content

Conversation

YuppY
Copy link

@YuppY YuppY commented Oct 27, 2020

This fix allows to use native client with multi-host configuration:

const { native: { Client } } = require('pg');

const client = new Client({
  user: 'user',
  host: 'foo,bar,baz',
  database: 'db',
});
client.connect().then(() => {
  client.query('SELECT 1', console.log);
});

This is workaround for #1470

I have a couple of questions though:

  1. Do we need this dns resolution at all? In which cases libpq cannot resolve host by itself?
  2. To make better tests of dns errors I need to patch dns.resolve. Can I just add jest to dependecies? Or node-postgres unit tests have to be written without mocking?

@brianc
Copy link
Owner

brianc commented Nov 2, 2020

Hey thanks for the pull request!

1. Do we need this dns resolution at all? In which cases libpq cannot resolve host by itself?

AFAIK libpq doesn't offer a non-blocking way to do dns resolution, so letting libpq do the dns resolution results in the node event loop blocking. There are ways to run blocking C/C++ operations on background threads with node bindings to get around this, but at the time that was over my head so I just did the resolving in node.

2\. Or `node-postgres` unit tests have to be written without mocking?

Yeah just monkey-patch dns.resolve in the test to a mock or stub function and then revert it to the original implementation at the end of the test if you need it to go back. Each file runs in its own node process so you don't have to do a ton of cleanup. Adding jest is a no-go...it would require way too much reworking of everything right now!

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

Successfully merging this pull request may close these issues.

2 participants