Conversation
* Got rid of pggen because of its limitations * [WIP] Adding mtree update functions
jason-lynch
left a comment
There was a problem hiding this comment.
The Merkle tree changes are a bit out of my expertise, but the new database interface looks good to me! I left two small comments for possible improvements. They could be a separate PR if you wanted to implement them.
| } | ||
|
|
||
| func (m *MerkleTreeTask) insertBlockRanges(pool *pgxpool.Pool, ranges []BlockRange) error { | ||
| // func (m *MerkleTreeTask) buildWhereClause(block types.BlockRange) (string, error) { |
There was a problem hiding this comment.
Do we need to keep this commented-out function?
There was a problem hiding this comment.
I had it there in case what I was trying didn't work out. Removed in #27
| return RenderSQL(SQLTemplates.GetPkeyOffsets, data) | ||
| } | ||
|
|
||
| func CreateXORFunction(ctx context.Context, db *pgxpool.Pool) error { |
There was a problem hiding this comment.
One thing that could make this interface a little friendlier is to make a struct that has a pgxpool.Pool property and make these functions methods on that struct, like:
type Queries struct {
DB *pgxpool.Pool
}
func (q *Queries) CreateXORFunction(ctx context.Context) error {
// ...
}That way, you don't need to pass the pool as an argument to every function. I'd still keep the context.Context argument to every method since that's a Go convention.
There was a problem hiding this comment.
Makes sense. Will refactor in a new PR.
| db := queries.NewQuerier(pool) | ||
|
|
||
| schemaExists, err := db.CheckSchemaExists(context.Background(), pgtype.Name{String: c.SchemaName, Status: pgtype.Present}) | ||
| schemaExists, err := queries.CheckSchemaExists(context.Background(), pool, c.SchemaName) |
There was a problem hiding this comment.
Rather than instantiating a new context for each query, I'd really recommend making the context an argument to RunChecks and passing the same context instance through to each of these query functions. I'd say the same for the other places you're using these queries.
In most cases, I like to initialize a context somewhere near the beginning of my programs, like in the main function. Then I make new sub-contexts from it to support different cancellation conditions. For example, I'll typically use signal.NotifyContext (also typically in my main function) to make it so that an incoming interrupt or terminate signal (like from docker stop or someone hitting ctrl+c) will cancel all in-progress tasks. Another example: if I want to apply a timeout to particular operation, I'll use context.WithTimeout to create a sub-context with a timeout condition. Since that timeout context derives from my root context, it can still be cancelled by OS signals. It provides a nice user experience and gives you an opportunity to call shutdown logic if needed.
This PR removes dependence on
pggenfor SQL code generation and implements a custom interface for interactions with the database. Whilepggenwas very good at what it did, it could not handle cases when the table name (or a similar identifier) itself was unknown -- something that's very commonly found in ACE queries since they execute against user tables during run time. Instead of having one set of queries handled bypggenand the other set handled differently, the custom interface implemented here standardises all interactions with the Postgres database instances.