fix: Delete by cq_id before insertion #266
Conversation
disq
requested review from
a team,
irmatov,
shimonp21 and
bbernays
and removed request for
a team
database/postgres/pgdatabase.go
Outdated
| if len(resources) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| if shouldCascade { | ||
| if err := deleteResourceByCQId(ctx, p.pool, resources, cascadeDeleteFilters); err != nil { |
There was a problem hiding this comment.
As an alternative we could enable ON CONFLICT UPDATE, but that checks PK collisions (which we want to keep/see/reported) and not necessarily a cq_id collision.
Or running this + the insert below in a TX might be a (costly?) option.
There was a problem hiding this comment.
I think one of the above needs to be the solution, due to the fact that when data is deleted during fetch, can cause bad user-experience, even if it is then re-inserted again anyway (@yevgenypats @roneli @bbernays).
Not sure which we should choose between TX and ON CONFLICT UPDATE. One thing idea in favor of using deleteResourceByCQId is just consistency with current behaviour in copyFrom. i.e., if we delete by cq-id in copyFrom, we should do the same kind of deletions in the fallback.
There was a problem hiding this comment.
At this point I don't think I have ever seen an issue that stems from DB performance on insert. The only incident that I am aware of is that someone had issues because they needed to vacuum their db. So overall, I am not very concerned about DB performance as our biggest bottleneck is by far and away the rate we can grab data from provider APIs...
There was a problem hiding this comment.
That being said, I like the idea and consistency of doing the deletion + insertion in a TX for each of our 3 methods of insertion (copy, insert multiple and singular insert)
#265 Continued
I think current code assumes
CopyFromtrial will delete all dupe ids, but it’s in a TX so as soon as it fails everything is reverted back to cause us more problems