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

Support for Postgres Pooling #363

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

starrematte
Copy link

@starrematte starrematte commented Oct 11, 2022

Added stable support for Pooling in Postgres.
A few changes have been made in order to achieve a consistent code.

  • deno.json and import_map.json have been added for support to some node libraries;

  • in the Connector interface, for code design issues I had to put _client and _connected as optional due to inchoerence with the pool existence;

  • PostgresPoolOptions has been added and relies on the original options of Postgres Poolclass;

  • PostgresConnector has been modified with substantial edits in the various exposed methods;

  • Database has been modified with the support of the underlying connector implementation of _pool with the method getPool?

Copy link
Owner

@eveningkid eveningkid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good evening, Matteo!

First off, thank you so much for contributing to this project–and for emailing me :)

I am usually swamped with work from all sides, it's hard for me to take time on open-source

Regarding this PR, looks solid. My comments are all to keep harmony in the codebase and should be easy to address

If not, feel free to raise any question/seek help in the comments and I'll be happy to assist :)

Have a great rest of your day!

Comment on lines +1 to +7
{
"imports": {
"https://dev.jspm.io/npm:@jspm/core@1/nodelibs/timers.js": "https://deno.land/[email protected]/node/timers.ts",
"https://dev.jspm.io/npm:@jspm/core@1/nodelibs/url.js": "https://deno.land/[email protected]/node/url.ts",
"https://dev.jspm.io/npm:@jspm/core@1/nodelibs/events.js": "https://deno.land/[email protected]/node/events.ts"
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sure this is a larger problem, but do you mind sharing what was causing this issue?

What parent lib?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, i saw that some of these libraries relies on "dev.jspm.io" and are no longer available. But anyway, the parent lib is the main postgres lib. The error that gives is the following:

Module not found :
"https://dev.jspm.io/npm:@jspm/core@1/nodelibs/url.js" at https://dev.jspm.io/[email protected]:3:8

Copy link
Contributor

@Aiko-Suzuki Aiko-Suzuki Oct 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one of the borken import for me is

export { default as SQLQueryBuilder } from "https://raw.githubusercontent.com/Zhomart/dex/930253915093e1e08d48ec0409b4aee800d8bd0c/mod-dyn.ts";
we can fix it by pointing to the branch instead of a commit.

https://github.com/Zhomart/dex/blob/update-deps/mod-dyn.ts

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep sure, thanks!

lib/connectors/connector.ts Outdated Show resolved Hide resolved
lib/connectors/connector.ts Outdated Show resolved Hide resolved
Comment on lines 32 to 35
_connected?: boolean | undefined;

/** Is the optional pool for making connections to an external instance. */
_pool?: ConnectionPool | undefined;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here, | undefined should be omitted :)


/** Test connection. */
ping(): Promise<boolean>;

/** Connect to an external database instance. */
_makeConnection(): void;
_makeConnection(): void | ConnectorClient | Promise<void> | Promise<ConnectorClient>;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should keep this a void method

It makes a connection and shouldn't return anything

Copy link
Author

@starrematte starrematte Oct 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the pool, we need it because of the underlying implementation. Should we keep it so?

Comment on lines 89 to 98
if (this._client) {
if (this._connected) {
return;
}

await this._client.connect();
this._connected = true;
return this._client;
} else {
return await this._pool!.connect();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's create two new methods:

_isPoolConnector() {
  return "size" in this._options;
}

_getClientOrPool() {
  return this._isPoolConnector() ? this.getPool() : this.getClient();
}

_makeConnection() {
  if (this._connected) {
    return;
  }

  await this._getClientOrPool().connect();
  this._connected = true;
}

Comment on lines 103 to 114
async ping() {
await this._makeConnection();
return await this.tryConnection(await this._makeConnection());
}

async tryConnection(client?: PostgresClient) {
try {
const [result] = (
await this._client.queryObject("SELECT 1 + 1 as result")
).rows;
await client!.queryArray("SELECT 1 + 1 as result")
).rows[0];
return result === 2;
} catch {
return false;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we keep all the code inside ping()?

Now we'll need to call this._getClientOrPool() every time we need this._client or ._pool

Copy link
Author

@starrematte starrematte Oct 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, as we stick to the this._getClientOrPool() we can keep everything inside ping()

Comment on lines 140 to 147
if (this._client) {
if (!this._connected) {
return;
}
await this._client.end();
} else {
await this._pool?.end()!
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here, using this._getClientOrPool().end() would be great :)

@@ -189,6 +189,11 @@ export class Database {
return this.getConnector()._client;
}

/* Get the database pool if existent. */
getPool?() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we drop the ? here?

Suggested change
getPool?() {
getPool() {

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this, actually is up to the implementation to give a "pooling" experience on the connector. Afaik, not every connectors have a pool implementation out of the box, but prove me wrong

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, no problem!

Comment on lines 42 to 60
* @example
* //Direct connection usage:
* const connection = new PostgresConnector({
* host: '...',
* username: 'user',
* password: 'password',
* database: 'airlines',
* });
* //Pool connection usage:
* const connection = new PostgresConnector({
* connection_params: {
* host: '...',
* username: 'user',
* password: 'password',
* database: 'airlines',
* },
* size: 5,
* lazy: false
* });
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this comes from a good intention but I believe types are good hints to use the connector

Types are always be up-to-date, we don't need to manually edit them each time

So let's keep your life easy here haha :D

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you are right!

starrematte and others added 3 commits October 14, 2022 17:59
@starrematte
Copy link
Author

new commit: 3aeaec8

I fixed some stuff that I made and added yours. I did not test the other components involved with the Connector
changes.

If something seems strange, I forgot something or everything else, let's chat about it :)

Aiko-Suzuki added a commit to Aiko-Suzuki/denodb that referenced this pull request Oct 23, 2022
* Added pool feature for the postgres connector

* added stable support for postgres pool, added tests

* Update lib/connectors/connector.ts

* fixed stuff as previously mentioned in eveningkid#363

* making getClient & _getClientOrPool optional

* update test

Co-authored-by: Matteo Stellato <[email protected]>
Co-authored-by: Arnaud <[email protected]>
Copy link
Owner

@eveningkid eveningkid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another series of comments, thanks a lot for keeping up with them!

Also, please always run deno fmt at the root of your project before committing your files

It will keep all the syntax bits coherent with the rest of the codebase :)

lib/connectors/postgres-connector.ts Outdated Show resolved Hide resolved
Comment on lines 69 to +81
async _makeConnection() {
if (this._connected) {
return;
if (!this._isPoolConnector()) {
if (this._connected) {
return this._client!;
}
await this._client!.connect();
return this._client!;
} else if (this._pool?.available || !this._pool?.available) {
return await this.getPool()?.connect()
} else {
throw new Error("no connections available")
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could makeConnection not return anything?

It was created to work as:

querySomething() {
  await makeConnection() // << make sure that this.client next line won't be undefined
  this.client.query(something)
}

Only connect, don't return a client :)

}
await this._client!.connect();
return this._client!;
} else if (this._pool?.available || !this._pool?.available) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if you intended to write this?

if (a || !a)

I think this whole method could be rewritten as:

async _makeConnection() {
  if (this._connected) {
    return;
  }

  if (this._isPoolConnector()) {
    await this._getPool().connect();
  } else {
    await this._getClient().connect();
  }
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem here, like the previous comment is that if you do this:

await this._getClient().connect() // -> connects the client

this connects the actual client already instanciated before.
While doing this:

await this._getPool().connect() //-> gives a client back on which connection should be established

So that's why I make it return a client.

If we write the code like so:

async _makeConnection() {
    if (this._connected) {
    return;
  }

  if (this._isPoolConnector()) {
    await this._getPool().connect();
  } else {
    await this._getClient().connect();
  }
}

we're going to get nothing.

We need something that makes us doing async client operations, and maybe could be like having an array of clients:

_clients?: Array<PostgresClient>;

where the first element is always instanciated if the connector is in "client" mode, while if in "pool" mode, the clients field grows until the max pool size choosen in the options.
Of course this has a bit of impact on the general design of the connector itself.

Another way could be to build a completely custom Pool class that every connector implement on their own. So the idea already shown with the clients could be possible without completely twsting the main design.

In a last option we can stick with _makeConnection() returning the client.
I do not see any better solutions at the moment, but something possibly will get in my mind soon.

@eveningkid what do you think?

const [result] = (
await this._client.queryObject("SELECT 1 + 1 as result")
).rows;
const connection = await this._makeConnection();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So now we can keep this outside of the try, as it was before:

async ping() {
  await this._makeConnection()

  try {
    const [result] = await this._getClientOrPool().queryArray(...)

And also, keep no console.log in your changes, thanks! :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot those while testing out😄

const query = this._translator.translateToQuery(queryDescription);
const response = await this._client.queryObject(query);
const response = await client!.queryObject(query);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that every call to client should now call this._getClientOrPool()

Comment on lines +128 to 133
if (this._client) {
if (!this._connected) {
return;
}
await this._getClientOrPool().end();
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep things simple here:

async close() {
  if (!this.connected) {
    return;
  }

  await this._getClientOrPool().end();
  this._connected = false;
}

@@ -189,6 +189,11 @@ export class Database {
return this.getConnector()._client;
}

/* Get the database pool if existent. */
getPool?() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, no problem!

Comment on lines +42 to +46
/** Gets the client connected to the database */
getClient(): any

/** Gets the pool connected to the database */
getPool?(): any
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** Gets the client connected to the database */
getClient(): any
/** Gets the pool connected to the database */
getPool?(): any
/** Gets the client connected to the database */
getClient?(): ConnectorClient;
/** Gets the pool connected to the database */
getPool?(): ConnectorPool;


/** Options to connect to an external instance. */
_options: ConnectorOptions;

/** Is the client connected to an external instance. */
_connected: boolean;
_connected?: boolean
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is always defined:

Suggested change
_connected?: boolean
_connected: boolean

@@ -16,19 +22,31 @@ export interface Connector {
_translator: Translator;

/** Client that maintains an external database connection. */
_client: ConnectorClient;
_client?: ConnectorClient;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this cause type errors in other connectors?

Since it wasn't optional before, I assume that any code like:

this.client.doSomething()

Would not work anymore, complaining that this.client could be not defined

We could keep _client as always defined in this file and override this interface just for Postgres

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this will have an impact in other files, so we can override it only in Postgres.

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.

3 participants