fix: use rpc for pins upsert #1088
Conversation
packages/db/postgres/functions.sql
Outdated
| BEGIN | ||
| FOREACH pin_data IN array json_arr_to_json_element_array(data -> 'pins') | ||
| LOOP | ||
| -- Add to pin_location table if new |
There was a problem hiding this comment.
Could we be calling upsert_pin here instead?
We'd avoid some duplication. There's an inconsistency with the data inputs, in one function we use content_cid and in the other we usecid, but I think that can be easily changed? And we'd benefit from a bit more consistency?
There was a problem hiding this comment.
Corrected to re-use upsert_pin and to return an array of pin Ids.
Also changed to use content_cid consistenly.
packages/db/index.js
Outdated
|
|
||
| if (error) { | ||
| throw new DBError(error) | ||
| } | ||
|
|
||
| return data |
There was a problem hiding this comment.
If I'm not mistaken the rpc call is currently returning void, so there's no actual useful data to return here.
It'd be probably nice to return an array of the inserted pin ids, to be consistent with upsertPin. If not we get rid of the return statement here.
There was a problem hiding this comment.
Yeah, I think we could be consistent and return Pin Ids
There was a problem hiding this comment.
Amended to return pinIds. Also amended upsert_pin to return pin_id not pin_location_id.
There was a problem hiding this comment.
Thanks for putting this together @GaryHomewood Overall looking good, let's wait for #1093 to get in to have ipfsPeerId column in location. And please look into @flea89's good suggestions
packages/db/index.js
Outdated
| location: { | ||
| peer_id: pin.location.peerId, | ||
| peer_name: pin.location.peerName, | ||
| region: pin.location.region |
There was a problem hiding this comment.
Be aware this will change soon (today) to have one more thing: https://github.com/web3-storage/web3.storage/pull/1093/files
So let's wait on that PR to get in and update this
There was a problem hiding this comment.
Merged latest to include ipfs peer id.
packages/db/postgres/migrations/004-add-upsert-pins-function.sql
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,59 @@ | |||
| DROP FUNCTION IF EXISTS upsert_pin; | |||
There was a problem hiding this comment.
This files is not needed anymore right? We already have a migration 004
There was a problem hiding this comment.
That's true, have removed.
| insert into pin (content_cid, status, pin_location_id, updated_at) | ||
| values (data ->> 'content_cid', | ||
| (data -> 'pin' ->> 'status')::pin_status_type, | ||
| pin_location_result_id, |
There was a problem hiding this comment.
Why this change? It is not in functions.sql, so I guess we don't need it?
With that in mind, we are not changing upsert_pin and only upsert_pins should be in the migration?
There was a problem hiding this comment.
Also true, have removed. Migration is just the new function.
|
The cron job has changed in the PR and it now relies on a function that currently does not exist in the production DB. So for 2 days now the pins job has been failing because it can't update pins. |
|
@alanshaw good catch. TL;DR However, this raises some questions for me. How can we avoid this in the future? I wonder if we should have a release step for cron code as well? |
|
Extracting from the convo here: https://filecoinproject.slack.com/archives/C02BZPRS9HP/p1647599946603429
@flea89 as we both suggested, adding it to release please might help. I think it's unexpected for changes to cron to immediately be in production when that is not the case for the rest of the packages in the repo. Any chance you can open an issue so we can track? 🙏🙏🙏 |
|
Sure, here it is! |
Revert to using a DB function to upsert pins so that we have the location id.
fixes #1082