listpays: sort output payments

[nalinbhardwaj]

Fixes #4481.

[vincenzopalazzo]

I have only a small curiosity and I would like to know more about that.

I'm not able to catch what Is the reason to collect the result from the database and only after sort it, instant to ask the database the list of payment sorted.

The only motivation that I have in mind is that the databases used from c-lightning use a different dialect, but I'm not able to prove that because the ORDER BY should be a basic construct of the SQL language

[rustyrussell]

OK, this is a great start! Your code is really nice.

Some feedback:

1. This actually orders the low-level command listsendpays. Which is great, but another commit is needed for the listpays command to order that (which uses listsendpays).
2. This commit should have a line like Changelog-Changed: JSON: `listsendpays` output is now ordered by `id`.
3. As @vincenzopalazzo points out, simply adding "ORDER BY id" is a much simpler change.
4. Create a new commit, which can use your asort code to the plugin/pay.c, except order it by created_at maybe? And you'll need to marshal it into a temporary array to sort it.

Thanks!
Rusty.

[nalinbhardwaj]

Thanks for the note @vincenzopalazzo @rustyrussell. I realised that after making this PR (which is why I ended up marking it as a draft PR). I'll make both the noted changes soon.

[nalinbhardwaj]

@rustyrussell @vincenzopalazzo ready for review.

[vincenzopalazzo]

Do you think that sort by timestamp it is a good idea here?

[rustyrussell]

id is the order it was placed in the db, which is probably right here.

[vincenzopalazzo]

utack 1459ab0

With two small nit to me, but @rustyrussell can undo my comments if they are wrong.

• If you sort (ORDER by timestamp ASC | DESC) by timestamp you can avoid the sorting operation, this moves the work from lightning code to database code, and the database can (or should/may) have better performance in sorting things. In addition, this change simplifies the code in the pay plugin.
• Use the query to sort the element you can avoid using the additional array, but it is in contrast with point 4 of Rusty's comment, so in this case, I can missing some things.

PS: I would like to have an additional test in test_pay that makes sure that the array is really sorted, to avoid crazy debugging in some case in the future.

[rustyrussell]

utack 1459ab0

With two small nit to me, but @rustyrussell can undo my comments if they are wrong.

* If you sort (ORDER by timestamp ASC | DESC) by timestamp you can avoid the sorting operation, this moves the work from lightning code to database code, and the database can (or should/may) have better performance in sorting things. In addition, this change simplifies the code in the pay plugin.

Unfortunately, listpays gathers listsendpays into a hashtable, losing order. So explicit sorting is still required here.

PS: I would like to have an additional test in test_pay that makes sure that the array is really sorted, to avoid crazy debugging in some case in the future.

More tests would always be nice, of course! I will have a go at writing one, since this needs a trivial rebase anyway...

[rustyrussell]

@nalinbhardwaj Sorry for the delay. Did a trivial rebase to fix the merge conflict with db files, and added tests and docs.

What do you think?

[nalinbhardwaj]

@rustyrussell Looks great! Thanks for the rebase and the doc+tests updates. Let me know if there's anything else I missed before.

[niftynei]

ACK 0d62227