Issue #3200 Add grant_notes and grant_notes_revisions table migration #3209
Conversation
github-actions
bot
added
database-changes
QA Summary
Test CoverageCoverage report for `packages/client`
Coverage report for `packages/server`
|
Terraform Summary
Hint: If "Terraform Format & Style" failed, run OutputValidation OutputPlan Summary
Pusher: @thucdtran, Action: |
Knex documentation says for the unique() function, that useConstraint will use a unique constraint and is default true for postgres when there is no predicate defined. Issue #3200 specifies that we want a unique index. Documentation can be found here: https://knexjs.org/guide/schema-builder.html#unique
|
This PR looks correct to the spec, but before we land the change I wanted to better understand why this is being structured as two tables instead of one. Are there query patterns or other considerations that make this setup better than a single table? To make my question more concrete, here's my first instinct of how I would structure the new notes data, if we want to capture all the revisions:
It seems like with either setup (as spec'd in the issue, or the table I laid out here) you have to do a slightly complicated query to get what you want to display on the page: (note that I haven't tested this query... it probably could use some tweaking, but it seemed useful for illustration) with current_grant_notes as (
select
id,
user_id,
text,
row_number() over(partition by user_id order by created_at desc) as rank
from grant_notes
where grant_id = {grant_id}
-- also probably need to join in agency to filter this down to within-agency notes
)
select * from current_grant_notes
where rank = 1If we wanted that query pattern to be simpler, I think we'd need to denormalize the concept of "current note" in the database (either as a separate table or as a flag on the table I'm proposing), which would then require a bit more upkeep when writing to the table. If we don't mind the above query pattern, then I'm not yet seeing the benefit of the two tables vs the one. Sorry to hijack the PR for a design discussion, but I wanted to make sure we've nailed down the schema before committing to it. (And my apologies if this was already discussed/captured elsewhere and I missed it!) What do you think, @TylerHendrickson and @thucdtran ? |
@jeffsmohan Thanks for the thoughts here – I wrestled with this a bit also. A few things to mention:
I think it's entirely possible that as the feature evolves and we understand more about its usage, we'll either need to migrate the data in this table to a new design or even to a different storage backend entirely, like DynamoDB or a document store, which are much more suitable for storing revisions and eventually consistent behaviors that I associate with distributed commenting systems. So I'm really viewing this table design as "works for the known short-term, somewhat robust enough for fast-follow/medium-term changes, with a decent chance that it will eventually be replaced in its entirety", while remaining something that's unlikely to have performance issues as a forcing function in making those decisions. |
|
@TylerHendrickson gotcha, thanks for catching me up, and sorry I missed the background in the other ticket! I see where the design is coming from, and I agree we should be prepared to evolve the schema as these early feature requirements evolve anyway, so it's not worth holding this up any further. (In other words, PR looks good to merge!) I'll just call out that it seems the requirement to store all note revisions ends up making the schema and the query patterns significantly more complicated to write (and whether it ends up mattering or not, less efficient queries). I'm trying to remember, was the storage of past revisions just to satisfy our internal desire to understand if/when/how users are modifying their notes? If so, I wonder if there's a better path where we only store the current note per user in the DB, but spew all revisions out to datadog as logs or some other less structured storage solution that we can query ad-hoc. Just noodling... if it spurs something useful, great; if not, consider me more than sufficiently happy to continue down the current path! |
Ticket #3200
Description
Add a knex migration for issue #3200.
Screenshots / Demo Video
Testing
Checked the table schemas in postgres CLI.
Checklist