Skip to content
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

Pagination error in api/blocks/{id}/transactions endpoint #2676

Closed
Moustikitos opened this issue Jun 8, 2019 · 7 comments

Comments

Projects
None yet
4 participants
@Moustikitos
Copy link

commented Jun 8, 2019

Considered block
Below is a block on ark mainnet containing 150 transactions :

{
  "forged": {
    "reward": 200000000,
    "fee": 1500000000,
    "total": 1700000000,
    "amount": 122934968010
  },
  "generator": {
    "username": "ravelou",
    "address": "AXvBF7JoNyM6ztMrgr45KrqQf7LA7RgZhf",
    "publicKey": "021f277f1e7a48c88f9c02988f06ca63d6f1781471f78dba49d58bab85eb3964c6"
  },
  "height": 8558258,
  "id": "bfd6c6264271b982a491d3895b8fe14fe98dd01204a39ccef687bb8342cef405",
  "payload": {
    "hash": "683cfc16afedacb143edc745a8db75852be228bd630d20adaad0dfb34ee0f2df",
    "length": 4800
  },
  "previous": "cc58117e7c991db134bcf3e3e8673f3cde28758e51a66d8e6c233d53f1342af2",
  "signature": "304402203cb02e0686f163509f984f86cad9da3bf62a54d70c4bb1974b2f9c81f7a5343a0220380ea2d022bc5cb6c2f4fd08edb5b5bbeda90d2547372cab5e636340de015902",
  "timestamp": {
    "epoch": 69550576,
    "unix": 1559651776,
    "human": "2019-06-04T12:36:16.000Z"
  },
  "transactions": 150,
  "version": 0
}

Expected Behavior
[peer]/api/blocks/bfd6c6264271b982a491d3895b8fe14fe98dd01204a39ccef687bb8342cef405/transactions

{
  "meta": {
    "count": 100,
    "pageCount": 2,
    "totalCount": 150,
    "next": "/api/v2/blocks/bfd6c6264271b982a491d3895b8fe14fe98dd01204a39ccef687bb8342cef405/transactions?page=2&limit=100",
    "previous": null,
    "self": "/api/v2/blocks/bfd6c6264271b982a491d3895b8fe14fe98dd01204a39ccef687bb8342cef405/transactions?page=1&limit=100",
    "first": "/api/v2/blocks/bfd6c6264271b982a491d3895b8fe14fe98dd01204a39ccef687bb8342cef405/transactions?page=1&limit=100",
    "last": "/api/v2/blocks/bfd6c6264271b982a491d3895b8fe14fe98dd01204a39ccef687bb8342cef405/transactions?page=2&limit=100"
  },
  "data": [...]
}

Current Behavior

{
  "meta": {
    "count": 100,
    "pageCount": 1,
    "totalCount": 100,
    "next": null,
    "previous": null,
    "self": "/api/v2/blocks/bfd6c6264271b982a491d3895b8fe14fe98dd01204a39ccef687bb8342cef405/transactions?page=1&limit=100",
    "first": "/api/v2/blocks/bfd6c6264271b982a491d3895b8fe14fe98dd01204a39ccef687bb8342cef405/transactions?page=1&limit=100",
    "last": "/api/v2/blocks/bfd6c6264271b982a491d3895b8fe14fe98dd01204a39ccef687bb8342cef405/transactions?page=1&limit=100"
  },
  "data": [...]
}

Because the considered block has more than 100 transactions, a second page should pe proposed in the meta.next field of JSON response.

@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

Thanks for opening this issue! A maintainer will review this in the next few days and explicitly select labels so you know what's going on.

If no reviewer appears after a week, a reminder will be sent out.

@Moustikitos Moustikitos changed the title Pagination error in `api/blocks/{id}/transactions` endpoint Pagination error in api/blocks/{id}/transactions endpoint Jun 8, 2019

@vasild

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

This boils down to a wrong estimate from postgres. For example:

ark_devnet=# SELECT COUNT(*) FROM transactions WHERE block_id = '4486277915989266225';
 count 
-------
   150
(1 row)

ark_devnet=# EXPLAIN SELECT * FROM transactions WHERE block_id = '4486277915989266225';
                                     QUERY PLAN                                      
-------------------------------------------------------------------------------------
 Bitmap Heap Scan on transactions  (cost=4.56..76.50 rows=19 width=512)
   Recheck Cond: ((block_id)::text = '4486277915989266225'::text)
   ->  Bitmap Index Scan on transactions_block_id  (cost=0.00..4.56 rows=19 width=0)
         Index Cond: ((block_id)::text = '4486277915989266225'::text)
(4 rows)

The estimate is rows=19 instead of 150.

@Moustikitos

This comment has been minimized.

Copy link
Author

commented Jun 20, 2019

Ok.

COUNT(*) returns the right value... So it should be technically possible to manage the meta field of json response... right ?

@vasild

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

There is a specific reason we don't use COUNT(*) and it is because it is horribly slow (in general). However if we have WHERE indexed_column = ... then it will be fast.

@vasild

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

A complication to use COUNT(*) only in some cases (if we know it is going to be fast) is that the code in question:
https://github.com/ArkEcosystem/core/blob/master/packages/core-database-postgres/src/repositories/repository.ts#L76
is servicing all possible queries so it is generic and does not know the details of the query.

One possibility is to try to parse the query from inside findManyWithCount() and assess whether it is ok to use COUNT(*), another possibility is to pass down this knowledge from the code that crafts the query (it knows whether it is doing WHERE indexed_column = ...). A third possibility is to ditch the postgres database and use something more appropriate.

vasild added a commit that referenced this issue Jul 4, 2019

feat(core-api): make it configurable whether to use estimates
Introduce a new boolean config option totalCountIsEstimate and use
database estimates for the total number of rows if it is true (fast)
or use the precise COUNT(*) if the option is false (slow).

So, it is up to the node operator to configure their node for accuracy
vs speed. Add a new property in the response to indicate which one is
being used.

Resolves #2676
Pagination error in api/blocks/{id}/transactions endpoint

supaiku0 added a commit that referenced this issue Jul 5, 2019

feat(core-api): make it configurable whether to use estimates (#2772)
* feat(core-api): make it configurable whether to use estimates

Introduce a new boolean config option totalCountIsEstimate and use
database estimates for the total number of rows if it is true (fast)
or use the precise COUNT(*) if the option is false (slow).

So, it is up to the node operator to configure their node for accuracy
vs speed. Add a new property in the response to indicate which one is
being used.

Resolves #2676
Pagination error in api/blocks/{id}/transactions endpoint

* fix(core-api): only try to return totalCount[IsEstimate] if it is set

* refactor(core-api): move totalCountIsEstimate property to meta

* chore: rename config variable totalCountIsEstimate to estimateTotalCount

* chore: make it possible to set estimate/ornot via the environment

* chore: rename short-lived variable q to query

* fix: missing recipient_id

* test: fix
@vasild

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

Fixed in #2772.
Now, if estimateTotalCount is set to false (it is true by default), then the precise and slow COUNT(*) will be used.

@vasild vasild closed this Jul 8, 2019

@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

This issue has been closed. If you wish to re-open it please provide additional information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.