sqldb+kvdb: unify SQL error mapping and transaction retry for between the kvdb and sqldb packages
#8611
Conversation
|
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the Note Reviews PausedUse the following commands to manage reviews:
WalkthroughThis update focuses on enhancing error handling across various components by adopting Changes
Possibly related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
bhandras
force-pushed
the
sql-tx-retry
branch
3 times, most recently
from
944f6dc to
726c127
Compare
bhandras
changed the title
sqldb and kvdbsqldb and kvdb
bhandras
changed the title
sqldb and kvdbkvdb and sqldb packages
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| @@ -334,6 +334,8 @@ func (t *TransactionExecutor[Q]) ExecTx(ctx context.Context, | |||
| if !ok { | |||
| return fmt.Errorf("expected *sql.Tx, got %T", tx) | |||
| } | |||
|
|
|||
| reset() | |||
There was a problem hiding this comment.
I wonder if missing this partially exacerbated the issues w/ the async payment benchmark?
There was a problem hiding this comment.
I think it's possible, although from my experience most of the issues came from PaymentControl's InitPayment method.
|
Can be rebased. And the module |
bhandras
force-pushed
the
sql-tx-retry
branch
4 times, most recently
from
c16dc0c to
1e86c2b
Compare
Can't fully use the tag yet as this PR also changes both |
|
@coderabbitai pause |
Some sub-systems like btcwallet will return an error from the database, but they won't properly wrap it. As a result, we were unable to actually catch the serialization errors in the first place. To work around this, we'll now attempt to parse the error string directly.
With the new postgres concurrency control, an error may come from a bucket function that's actually a postgres error. In this case, we need to return early so we can retry the txn. Otherwise, we'll be working with an aborted tx, and never actually return the error so we don't auto retry.
In this commit, we fix a bug that would cause the entire db to shutdown if hit a panic (since db operations in the main buckets exit with a panic) while executing a txn call back. This might be a postgres error we need to check, so we don't want to bail out, and instead want to pass up the error to the caller so we can retry if needed.
For SQL transactions, we often accumulate results in variables declared outside the closure's scope. To eliminate the need for manually clearing these containers, we introduce a reset function to ExecTx, mirroring the approach already adopted in kvdb.
Previously if the `reverse` named arg was unset (value of NULL), then
SQL would order by NULL instead of ID causing undifined ordering of the
returned rows. To fix that we check for NULL and also make sure to set
the `reverse` arg in the code explicitly as it in the generated code it
is an `interface{}` instead of `bool`.
Post merge module clean up after lightningnetwork#8611.
Previously we had very similar error mapping and retry functions in the affected modules diverging over time. To avoid bugs resulting from individual updates this PR consolidates the retry and error mapping to live in the newly carved out
sqldbpackage.Based on: #8603
Added commits from : #8529