API v2 proposal: remove PinResults.count

[lidel]

Prompted by ipfs/ipfs-webui#1900 – this is BREAKING CHANGE for v1 API, so marking this as something we can discuss if we ever want to create v2.

PinResults.count is used for two things:

(1) Reading pin count per status (used in ipfs pin remote service ls --stat):

GET /pins?limit=1&status=queued
GET /pins?limit=1&status=pinning
GET /pins?limit=1&status=failed
GET /pins?limit=1&status=pinned

(2) pagination (where it acts as an equivalent of more: true:

If the value in PinResults.count is bigger than the length of PinResults.results, the client can infer there are more results that can be queried.

Problem

• Calculating PinResults.count is expensive, especially when running expensive filters like cid, name or status
  • ipfs-desktop/webui is checking CID status via GET /pins?cid=Qm1.Qm2...Qm10 which

Proposed change

• remove PinResults.count
• add PinResults.more (true/false bool)
• add /service/pins/healthcheck that allows inexpensive health check
• add /service/pins/stats that returns total pin counts for each status
• switch ecosystem to new fields

This would be a breaking change that requires v2.x.x of this spec and coordinated effort across the stack:
go-ipfs, js-ipfs, ipfs-webui, ipfs-desktop and Pinata.

Is it really breaking? Needs analysis.

I've been thinking a bit about a way to do this in backward compatible way and we could have PinResults.count as an optional field, and always return 1 so the pagination and health checks in old clients still works.

👉 Before we consider doing this, the main question is how old client in go-ipfs will react to an unexpected PinResults.more – if it is ignored and nothing crashes, then we have a better path forward.

If not, we would break our users, and need to coordinate go-ipfs/ipfs-webui/ipfs-desktop and Pinata to make the change around the same time to limit the damage. TBD is this is acceptable, considering the performanc benfits in the long run.

cc @obo20 @aschmahmann

[aschmahmann]

If not, we would break our users, and need to coordinate go-ipfs/ipfs-webui/ipfs-desktop and Pinata to make the change around the same time to limit the damage. TBD is this is acceptable, considering the performanc benfits in the long run.

Can we not just add support for a v2 (and maybe a versions) endpoint and avoid breaking changes here? IIRC when it was previously discussed that we wouldn't use libp2p and leverage multistream for protocol negotiation, but instead use HTTP the claim was that having version negotiation would be similarly easy.

[lidel]

Yes, if we every do this, then we would introduce this breaking change as v2 + versioning scheme.
I've updated the title of this issue + created topic/v2 for changes like this one.

So far this proposal got no feedback from any pinning service – let's park this until there is interest and/or we gather more items under topic/v2

[guseggert]

Besides being expensive, looking at the count while paginating has problematic edge cases, e.g. pins being added/deleted while paginating.

If we're going to make a breaking change, we should consider using an opaque pagination token instead of the created timestamp, which makes less assumptions about the capabilities of the underlying DB and can include arbitrary info to help the DB paginate better.

[obo20]

@lidel just now seeing this and definitely supporting this change. Count is super expensive for larger pinsets and can cause a lot of issues at scale.

I'm in favor of moving towards a created timestamp paradigm here. We had to switch our normal pinList endpoint to using this paradigm as the previous method which required counting was becoming extremely expensive for larger users.

[obo20]

@lidel I would also add here that getting rid of count would allow us to raise our rate limits, which is something I know the PL team / ipfs-desktop users have been asking for on this specific endpoint