Caught errors within transactions still thrown as uncaught

[issa-tseng]

maybe it's just me, but i find this surprising:

sql.begin((tsql) =>
  tsql`select * from nonexistent`
    .then(console.log)
    .catch((ex) => Promise.resolve('no no it's fine everything is okay'))
).catch((ex) => console.log('got:', ex));
// got: PostgresError: relation "nonexistent" does not exist

i feel like this should not result in an error at all. i handled it, right?

[ghost]

Check this out: https://www.postgresql.org/docs/14/tutorial-transactions.html

Transactions are all or nothing. If you catch error from individual SQL statement inside the sql.begin() callback function it doesn't mean you have somehow saved the transaction from failing. The transaction is rolled back, and failed as a whole regardless of what you do next.

You may look at savepoints if you want to avoid this.

[porsager]

Hmm.. In that case what would you suggest the sql.begin promise should resolve to? I suppose it could be the resolved value from your inner query catch but even so I think it goes a bit against how you use transactions in raw sql.

You should use savepoints to recover from specific statement (groups) in your transactions instead.

eg. try something like this (not tested)

sql.begin(sql =>
  sql.savepoint(sql => sql`select * from nonexistent`)
    .then(console.log)
    .catch((ex) => Promise.resolve('no no it's fine everything is okay'))
).catch((ex) => console.log('got:', ex));

[issa-tseng]

there is no intent to make further queries within the transaction after the error. it feels like that ought to be a separate concern from what the return value of the .begin() call should be.

certainly in my scenario the query failure does not mean the user operation failed, and indeed the successful result i need to return once i .catch the query failure gets clobbered by the original postgres error.

i understand the correlation you are trying to draw @Megous and i guess if you assume the promise language is meant in this scenario to model and follow a sql transaction's semantics, then it is correct to say that a statement failure is a statement failure is a statement failure because the transaction is done, and therefore the return value of .begin() must be a failure.

i'd argue instead that promises come with their own semantics beyond just the signature of .then(), and to me given a function begin(f: T => Promise): Promise i am quite surprised that the value i'm returning from f is silently ignored in favor of what from my perspective seems to be an intermediate result in my promise chain that i dealt with.

so i guess yes, i would expect that the result would be the result of my promise chain. i would expect that if i tried to make further database requests some kind of error would occur or be thrown.

[issa-tseng]

i guess to word a different way, it's very confusing to me that the promise i return from inside begin is respected in terms of flow control but not the result value in the event of a failure. whereas on success both flow control and data are followed. it feels strange that my promises are being awaited on but not always listened to.

[charsleysa]

@issa-tseng the .begin() call stores any error of any query performed inside the transaction. When your resolved promise is returned to .begin() it will rethrow the stored error and rollback the transaction. This causes the weird functionality that you're experiencing.

I've opened PR #444 to fix this so that .begin() is more straightforward in that if the returned promise is resolved then the transaction is committed otherwise if the returned promise is rejected the transaction is rolled back.

As a side note, either way unless you are doing some custom transaction handling you probably don't want to be committing the transaction when there's an error. With PR #444 merged you could do:

sql.begin((tsql) => tsql`select * from nonexistent`)
    .then(console.log)
    .catch((ex) => Promise.resolve(`no no it's fine everything is okay`));

or

sql.begin((tsql) =>
    tsql`select * from nonexistent`.catch((ex) => Promise.reject(new MyCustomError(ex)))
)
    .then(console.log)
    .catch((ex) => {
        if (ex instanceof MyCustomError) {
            return Promise.resolve(`no no it's fine everything is okay`);
        } else {
            return Promise.reject(ex);
        }
    });

[issa-tseng]

hey, sorry, it's been a while since i had time to sit at a computer. i think your solution sounds reasonable, thank you!

[porsager]

@issa-tseng sorry I haven't had time to look closer at this, but rereading now, I think your points makes sense. Unfortunately it's a breaking change to alter the behaviour, but I think it's worth looking into for v4.

Do you have any input reading the rest of the thread @Megous ?

[issa-tseng]

hey @porsager, thanks for the note! i'm guessing this is a separate issue, but is this related at all to the multi portal work you tried out?