-
Notifications
You must be signed in to change notification settings - Fork 5
Implement generic Table for basic CRUD operations & concurrent safe dequeuing #30
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.
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
base: master
Are you sure you want to change the base?
Conversation
6135657 to
211e63f
Compare
Co-authored-by: David Sedlacek <david.sedlacek@golang.cz>
.Save() - variadic arg instead of .Save() and .SaveAll() .List() - renamed from .GetAll() .Get() - renamed from .GetOne() .LockForUpdates() - renamed from .LockForUpdate() .LockForUpdate() - renamed from .LockOneForUpdate()
211e63f to
9df208d
Compare
david-littlefarmer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment, looks good, it would be nice to also have paginated methods.
|
Exactly, great point about the pagination 👍 I'd love to see both LIMIT/OFFSET and cursor-based pagination defined as a base data model methods. This needs some more thoughts, so I skipped it for now. |
| } | ||
|
|
||
| // Count returns the number of matching records. | ||
| func (t *Table[T, PT, IDT]) Count(ctx context.Context, where sq.Sqlizer) (uint64, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make where sq.Sqlizer a non-nil clause so that it doesn't check for all conditions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I think it's a totally valid point to worry about performance of the nil case, which effectively queries all table data:
SELECT COUNT(1) FROM tableImho, I think we should keep it as is & provide a warning about this in the comment.
And also, I think it'd be great to provide a faster alternative that returns "estimated count" via a query such as
-- Planner’s estimate of live tuples in the table. Updated by VACUUM, ANALYZE, certain DDL.
SELECT reltuples::BIGINT AS estimated_count
FROM pg_class
WHERE relname = 'table_name';
or
-- Estimated live rows tracked by the statistics collector (monitoring view) for each table.
SELECT n_live_tup
FROM pg_stat_user_tables
WHERE relname = 'table_name';
^^ need to test these first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the need for us to have a Count() method on the table..? I am nervous for offering this feature to developers, because it means they'll use it, and counting a table is one of the slowest things you can do.. can we just remove this method entirely..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imho there's nothing wrong with SELECT COUNT(1) as long as we hit the index properly, e.g.:
SELECT COUNT(1) FROM jobs WHERE status = 2;
But I hear you, this should never get "abused" for wrong purposes, such as counting number of total pages for LIMIT OFFSET pagination.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@klaidliadon @david-littlefarmer what do you use COUNT(1) or COUNT(*) for in builder and marketplace-api? I see several use of COUNT in these codebases. Are there any legit use-cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we are counting the orders (listings/offers) so we can show the numbers on top of the page on FE. How many listings are there as total count and as filtered count.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the Builder we use count using foreign keys and/or indexed values.
There are a few exceptions for tables that have a really low row count.
* save multiple * pr comments * return error instead of nil on nil record
Concurrent dequeue/update pattern
FOR UPDATE SKIP LOCKEDpatternpgkit/tests/table_test.go
Lines 193 to 200 in d081629
pgkit/tests/tables_test.go
Lines 24 to 47 in d081629
pgkit/tests/worker_test.go
Lines 24 to 64 in d081629
Data models with strongly typed
*data.Recordtype (using generics):pgkit/tests/table_test.go
Lines 24 to 45 in d081629
pgkit/tests/table_test.go
Lines 63 to 83 in d081629