Helper functions

[jwalton]

Hey, I use these two helper functions in pretty much every project I use node-postgres in. I was just wondering if you'd be open to a PR to add these to the project proper?

export async function withClient(pool: Pool, fn: (client: PoolClient) => Promise<void>) {
    const client = await pool.connect();
    try {
        await fn(client);
    } finally {
        client.release();
    }
}

export async function withTransaction(pool: Pool, fn: (client: PoolClient) => Promise<void>) {
    await withClient(async client => {
        try {
            await client.query('BEGIN');
            await fn(client);
            await client.query('COMMIT');
        } catch (e) {
            await client.query('ROLLBACK');
            throw e;
        }
    });
}

The basic idea here is I can do:

await withTransaction(pool, async client => {
    client.query(...);
});

and then there's no chance of me messing up and forgetting the ROLLBACK or forgetting to release the client from the pool; it all gets handled for me. If you're interested, I'll do up a PR and write some docs.

[vitaly-t]

This shouldn't be in the driver. Such generic functions should only be considered when they are truly generic, but the ones shown are absolutely not, they are missing a lot of things. Here's just the top ones:

• Connect/disconnect overrides + events reporting.
• Custom begin and nested calls for transactions.

Non-customizable solutions will only bloat the library.

[sehrope]

+1

I must have written this same function a hundred times too. I used to think anything like this should be external but it's such a common use case I've changed my mind on it. It'd be helpful for anyone new coming to use pg and useful to reference in issues / questions as well.

Slight improvement to allow for an arbitrary return type:

export async function withClient<ResultT = void>(pool: Pool, fn: (client: PoolClient) => Promise<ResultT>): Promise<ResultT> {
    const client = await pool.connect();
    try {
        const result = await fn(client);
        client.release();
        return result;
    } catch (err) {
        client.release(err);
        throw err;
    }
}

export async function withTransaction<ResultT = void>(pool: Pool, fn: (client: PoolClient) => Promise<ResultT>): Promise<ResultT> {
    await withClient(async client => {
        try {
            await client.query('BEGIN');
            const result = await fn(client);
            await client.query('COMMIT');
            return result;
        } catch (e) {
            await client.query('ROLLBACK');
            throw e;
        }
    });
}

That let's you do stuff like:

const result = await withTransaction(pool, async (client) => {
    await client.query('INSERT INTO foo ...');
    await client.query('INSERT INTO bar ...');
    const result = await client.query('INSERT INTO baz ... RETURNING id');
    return result.rows[0].id;
});

If we do add something like this, it's tied closely enough to the Pool that it could be added as a member of the pool itself, i.e. await pool.withTransaction(async (client) => {... })).

A more direct approach for transaction handling could also deal with errors a bit smarter by not evicting from the pool if the ROLLBACK is successful:

export async function withTransaction<ResultT = void>(pool: Pool, fn: (client: PoolClient) => Promise<ResultT>): Promise<ResultT> {
    const client = await pool.connect();
    try {
        await client.query('BEGIN');
        const result = await fn(client);
        await client.query('COMMIT');
        // Successful transaction and COMMIT so return client to pool:
        client.release();
        return result;
    } catch (e) {
        try {
            // Try to issue a ROLLBACK
            await client.query('ROLLBACK');
            // Successful ROLLBACK so assume client is valid for reuse
            client.release();
        } catch (rollbackErr) {
            // Errant ROLLBACK  so assume client is permanently broken
            client.release(rollbackErr);
        }
        // Throw the original error so the user sees that rather than any rollback issues
        throw e;
    }
}

Updated: Fixed not returning broken clients to the pool in withClient(...). The original version would blindly add them back even if the task had an error. I think that's another example of why it'd be nice to have a tested / validated / working thing like this within the library. Also, added a more elaborate withTransaction(...) function that tries to recover errant connections by issuing a ROLLBACK.

[ianstormtaylor]

+1 as well. It seems like having these built-in could handle 90% of use cases, and people can always write their own more custom transaction processing utils if they need more complex pieces.

---

I also think that pg could be offering slightly nicer helpers for these things by default, to avoid people running into common traps of forgetting to release clients, rollback transactions, etc. These defaults seem pretty unopinionated still to me, but would make using pg out of the box a lot less error prone.

For example, getting access to a single client right now:

// Currently...
const client = await pool.connect()

try {
  ...
} finally {
  client.release()
}

// Could instead be...
await pool.client(async (client) => {
  ...
})

Or similarly for transactions:

// Currently...
const client = await pool.connect()

try {
  await client.query('BEGIN')
  ...
  await client.query('COMMIT')
} catch (e) {
  await client.query('ROLLBACK')
  throw e
} finally {
  client.release()
}

// Could instead be...
await pool.transaction(async (client) => {
  ...
})

// Which is essentially the same (and available) as...
await client.transaction(async (client) => {
  ...
})

[brianc]

I'm down to merge something like this if it's sufficiently tested! I think ideally you could create another project in the monorepo like pg-fns or something & add it there. Then you could write it with sweet, sweet typescript. Currently converting the core pg module to typescript is something I've fiddled with locally a few times but isn't quite in scope yet, and would 100% be a semver major just because typescript is gonna fiddle w/ the internals of things enough to likely break a bunch of integrations and stuff (things like datadog and sentry sometimes poke deeply into the module's code). That would also keep anyone who wants the main package "clean" to have it their way, and I could long term own the maintenance and documentation of pg-fns (with help from the community of course!) This would hopefully allow on-boarding into the ecosystem to be easier, reduce having to write the slightly finicky withTransaction function yourself in every project, might be a cool place for new contributors to contribute to, etc.

The name of the module can be whatever you want - I just picked pg-fns because it's available on npm. Apparently a lot of pg-* modules are taken already 🙃

[brianc]

I also think that pg could be offering slightly nicer helpers for these things by default, to avoid people running into common traps of forgetting to release clients, rollback transactions, etc.

I agree with you - most of those traps are due to me designing a...maybe not the best...api a long time ago and trying hard to limit backwards incompatible breaks. query queuing, emitting row events, manually releasing clients, being really opinionated with timezones and type coercion, etc...are all things I could have done a better job of a long time ago. Now it's a careful dance of weighing the cost of forcing thousands of folks to change their apps versus maintaining things which aren't very nice or intuitive. I've focused this year on improving performance (we're up something like 30-40% from 7.x branch) and will likely start the work of deprecating and slowly sweeping away some of those bad decisions in 2021. I planned on more of that in 2020 but you know 2020 wasn't the very best year for focus/productivity while the world has been on fire. ❤️ 😬

I will say if you feel the pain I feel it 10x.

[ianstormtaylor]

@brianc not criticizing you for the decisions at all! I love pg and it's inspiring how well you've maintained it over the years, probably one of the most solid packages in the Node ecosystem.

I was more just thinking of what a more intuitive API could look like now that we've got async/await and all. I think things like pool.client and pool.transaction could be layered into the existing API as methods very easily without needing to have a breaking change—and they'd just compose the existing lower level methods anyways, so there's no real need to have a breaking change. (And of course you could always do a major release to drop old debt in the future, which sounds great.)

---

FWIW, one thing I was also thinking about was the current package/repo separation...

I think there's an argument that pg-pool shouldn't actually be a separate package. It seems like the best practice is to use pools for most real-world use cases, and that pg just re-exports pg-pool. I'd argue that the separate package isn't really buying much, and causing a slightly confusing amount of redirection (I noticed there was another issue open about dependency locking). It also seems that pg-pool is very pg-package specific and even requires pg under the covers for the default use case.

My suggestion might be for Pool to live in pg itself. And to layer the helpers discussed in this issue into Pool and Client as methods, instead of adding another pg-fns package that will also likely be tightly coupled.

Even if you don't merge pg-pool, I still think these helpers make more sense as methods instead of an alternative pg-fns. Just thinking about the "recommended" case—if these helpers are truly helpful for the 90% case and will prevent common footguns it doesn't really make sense to add a hurdle for people to getting the better experience.

Anyways, just food for thought. I could very likely be not taking into account some important considerations!

[abenhamdine]

Just to add that indeed I think nearly everyone needs to do some transactions in its code and will eventually implement its own helpers...

But in another hand, I understand the reason to keep low the burden of maintenance of such a popular package as pg.

I think a separate package is a very acceptable solution.

FWIW, we have added following helpers to pg in our app :

await pg.getClient(name) // return a client and attach it an associated label to track internaly blocked clients
await client.transaction(fn) // same as already discussed in this issue : perform the query in a transaction
await client.begin() // do the same thing as client.query('BEGIN;')
await client.commit() // do the same thing as client.query('COMMIT';)
await client.rollback() // do the same thing as client.query('ROLLACK;)

/**
 * This module handle safegard transaction by avoiding the client to be in a blocked state.
 * Every failed statement will be rollbacked to the last previous savepoint
 */
export async function safeguardTransaction<T = Error>(params: SafeguardParams<T>): Promise<void> {

    const { client, functionToSafeguard, onErrorCallback } = params

    // * we need unique ids
    const savepointId = getSavepointId()

    try {
        await client.query(`SAVEPOINT ${savepointId};`)
        await functionToSafeguard()

    } catch (err) {
        await client.query(`ROLLBACK TO SAVEPOINT ${savepointId};`)

        if (onErrorCallback) {
            await onErrorCallback(err)
        }

    } finally {
        await client.query(`RELEASE SAVEPOINT ${savepointId};`)
    }

}

function getSavepointId(): string {
    let result = ''
    const stringLength = 30
    const characters = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz_'
    const charactersLength = characters.length
    for (let i = 0; i < stringLength; i++) {
        result += characters.charAt(Math.floor(Math.random() * charactersLength))
    }
    return result
}