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.

Sign up for GitHub

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

Jump to bottom

Add pkg/pg with dialects.go & txdb.go #910

Merged
merged 17 commits into from
Dec 13, 2024
Merged

Add pkg/pg with dialects.go & txdb.go #910

merged 17 commits into from
Dec 13, 2024

Conversation

reductionista
Copy link
Contributor

@reductionista reductionista commented Nov 1, 2024 •
edited
Loading

NONEVM-739

Supports

smartcontractkit/chainlink#15064
smartcontractkit/chainlink-solana#921

Description

This mostly just moves chainlink/core/internal/testutils/pgtest/txdb.go and chainlink/core/store/dialects/dialects.go into a new common package chainlink-common/pkg/pg, updating imports accordingly.
The purpose of this is so that it can be imported and used by chainlink-solana for unit testing Solana ORM''s (for example of actual use, see the linked chainlink-solana PR this supports)

Since txdb.go was a bit of a mess, it has been cleaned up a bit.

  • The global init() function invoked on package load has been replaced with RegisterTxDb() which accepts a dbUrl as a param instead of automatically reading it from the CL_DATABASE env var. Since there are only two places in the codebase which depend on our custom txdb dialect of sql being registered, this can simply be called in both of those places (inside NewSqlxDb() and NewConnection()) instead of relying on the global init().
  • Prevents calling sql.Register() more than once if RegisterTxDb() is called more than once (calling sql.Register() more than once with the same driver name will result in an error).
  • Replaces passing of context.Background to queries with passing of a new context which gets cancelled when the db is closed. (Note: db here refers to the txdb, which is just a single connection in the lower level pg db, corresponding to the dsn passed in which is a randomly generated UUID.)
  • Corrected spelling errors

This also moves some unit tests of txdb itself from chainlink into chainlink-common. Since they need an actual database, but don''t need any fixtures this is an ideal use case for an in-memory test db, something in between the empty interface added earlier and spinning up a full docker postgresql container with the schemas and tables all set up. go-duckdb has been used for this, which is now easy to add to our dependencies since we''re already at go 1.23. If the CL_CHAINLINK env var is set locally, txdb can be tested with a full ordinary postgresql database backing it... otherwise it falls back to testing it with the in-memory db backing it.

core ref: ec9e488802243585e0ae5827723f6136d6e7739b
solana ref: 6fd1891d0fbc79b0f10be353f6bd97446588209a

@reductionista reductionista mentioned this pull request Nov 1, 2024
Merged
jmank88
jmank88 reviewed Dec 13, 2024
View reviewed changes
pkg/sqlutil/pg/txdb.go
// store to use the raw DialectPostgres dialect and setup a one-use database.
// See heavyweight.FullTestDB() as a convenience function to help you do this,
// but please use sparingly because as it's name implies, it is expensive.
func RegisterTxDb(dbURL string) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
func RegisterTxDb(dbURL string) error {
func RegisterTxDB(dbURL string) error {

@reductionista reductionista merged commit edc5dee into main Dec 13, 2024
8 of 11 checks passed
@reductionista reductionista deleted the NONEVM-739/txdb branch December 13, 2024 21:05
hendoxc pushed a commit that referenced this pull request Dec 17, 2024
@reductionista @hendoxc
Add pkg/pg with dialects.go & txdb.go (#910)
0b12d46 
* Add pkg/pg with dialects.go & txdb.go

Neither of these were in the actual pg package in chainlink repo.
dialects.go came from core/store/dialects and txdb.go from
core/internal/testutils/pgtest, but neither of these seem like they
deserve their own package in chainlink-common--we can lump all the
postgres specific common utilities under pkg/pg

* Add TestTxDBDriver, NewSqlxDB, SkipShort, SkipShortDB and SkipFlakey

* Add idempotency test of RegisterTxDb

* Create ctx from testing context, instead of using context.Background

* Only abort tx's when last connection is closed

Also: convert rest of panic isn't ordinary errors

* go mod tidy

* Split abort channel into one per connection object

All txdb connections share the same underlying connection to the
postgres db. Calling NewSqlxDB() or NewConnection() with dialect=txdb
doesn't create a new pg connection, it just creates a new tx with
BEGIN. Closing the connection with db.Close() issues ROLLBACK.

Both NewSqlxDB() and NewConneciton() choose random UUID's for their
dsn string, so we shouldn't have a case where the same dsn is opened
more than once. If that did happen, then these two different txdb
"connections" would be sharing the same transaction which would
mean closing the abort channel due to a query sent over one of them
would affect the other. Hopefully that's not a problem? If it is
I think our only option will be to go back to using context.Background
for all queries.

Before this commit, there was only one abort channel for the entire
txdb driver meaning that even two entirely different connections
opened with different dsn's could interfere with each other's queries.
This should fix that case, which is presumably the only case we
care about. Since each dsn corresponds to a different call to
NewSqlxDB() and the UUID's are generated randomly, there should no
longer be a conflict. Each txdb connection will have its own abort
channel.

* Errorf -> Fatalf on failure to register txdb driver

* Add in-memory DataSource using go-duckdb

* Fall back to testing txdb with in-memory backed db if CL_DATABASE_URL is not set

This allows us to test most of it in CI, and all locally

* Fix imports & fmt.Sprintf -> t.Log

* Add concurrency test for RegisterTxDb()

* Fix race condition

This showed up in some of the unit tests in the linked PR in chainlink repo

* Remove pg.SkipDB(), add DbUrlOrInMemory()

* pkg/pg -> pkg/sqlutil/pg

* NewSqlxDB -> NewTestDB, DbUrlOrInMemory -> TestURL
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pavel-raykov pavel-raykov pavel-raykov left review comments

@samsondav samsondav samsondav left review comments

@dhaidashenko dhaidashenko dhaidashenko approved these changes

@jmank88 jmank88 jmank88 approved these changes

@chudilka1 chudilka1 Awaiting requested review from chudilka1 chudilka1 is a code owner automatically assigned from smartcontractkit/foundations

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@reductionista @jmank88 @samsondav @dhaidashenko @pavel-raykov