reduce frequency of `lastoriginupdate` db writes #702

e-five256 · 2024-04-11T00:37:47Z

potentially limit the amount of times we update magazine entries to only update if it has been an hour since the last update, as before it was updating every single event

nobodyatroot · 2024-04-11T00:45:51Z

Output of https://wiki.postgresql.org/wiki/Lock_Monitoring#.D0.A1ombination_of_blocked_and_blocking_activity on kbin.run before applying this patch:

BentiGorlich · 2024-04-11T07:14:52Z

What do you think about instead of updating with every announce (which includes like and is probably the biggest problem) to only update with announced create activities?

e-five256 · 2024-04-11T13:36:44Z

Even if this was moved to comments vs likes I think we still don't need every single update always as the check is >24h, it seems like we shouldn't landrush the db just like we don't for apfetchedat

BentiGorlich · 2024-04-11T13:53:02Z

apfetchedat means something totally different though. That is the date we last fetched an update from the remote server. This date means when did the remote server last send us something. And nothing is attached to the date. I would like that to be as accurate as poosible and likes do not really count, so I am willing to remove them, but not creates...

e-five256 · 2024-04-11T22:13:53Z

When your queues fall behind and you try to catch up, every single event locks the magazine row to update this. It most likely causes deadlocks as debounced has shown (though the root cause might be unrelated, it still showed just how often it's required). You just don't need this value to be accurate to the nanosecond of every event. I'm willing to bring it down to a minute if that makes you feel better, but I don't think there should be no limit whatsoever, causing this performance hit on the DB

Like, imagine you're 5k activities behind on /m/196. You don't need 5k DB writes, you need to catch up, this update should be throttled somehow

nobodyatroot

Agreed that we should not be hitting the db everytime considering what this "feature" is doing. 1 minute resolution is plenty (and probably still overkill).

e-five256 · 2024-04-12T13:10:07Z

@BentiGorlich want to make sure this seems acceptable to you after the change

BentiGorlich · 2024-04-12T13:16:11Z

I've been thinking about it and came to the conclusion that a ~3h timespan would be a good value. Atm this galue is only used to display magazine stalenes and only if no one is subscribed to the magazine... However I think we should still limit it to create activities, because even if it is just a redis hit when getting all the likes, it still involves latency. If the redis server is located on a differenf machine (who knows) that latency is super noticable... Let me know what you think.

I'd accept it the way it is now as well as it is obviously an improvement

nobodyatroot · 2024-04-12T13:20:39Z

I've been thinking about it and came to the conclusion that a ~3h timespan would be a good value. Atm this galue is only used to display magazine stalenes and only if no one is subscribed to the magazine... However I think we should still limit it to create activities, because even if it is just a redis hit when getting all the likes, it still involves latency. If the redis server is located on a differenf machine (who knows) that latency is super noticable... Let me know what you think.

I'd accept it the way it is now as well as it is obviously an improvement

i can reapprove if this is better, that is ~3h timespan @e-five256

e-five256 · 2024-04-12T14:00:51Z

I'll work on making the changes suggested

changes in process

change threshold for updates to 3 hours old

e-five256 · 2024-04-13T02:36:19Z

definitely super look over me here as this is outside my wheelhouse, kind of guessing based on grabbing payloads from past logs shared in matrix, as obviously this is difficult to test locally. If you meant announces of creates and this should go inside dewrapping announces, let me know.

BentiGorlich · 2024-04-15T09:48:49Z

src/MessageHandler/ActivityPub/Inbox/ActivityHandler.php

+        } elseif ('Create' === $payload['type']) {
+            $actor = $this->manager->findActorOrCreate($payload['actor']);
+            if ($actor instanceof Magazine && $actor->lastOriginUpdate < (new \DateTime())->modify('-3 hours')) {
+                $actor->lastOriginUpdate = new \DateTime();
+                $this->entityManager->persist($actor);
+                $this->entityManager->flush();
+            }


What actually needs checking is the announced type, so payloads like this will trigger the code:

{ "type": "Announce", "object": { "type": "Create", ... }, ... }

So I'd say move it back to where it was before and change the condition to:

if ('Announce' === $payload['type']) { if (\is_array($payload['object'])) { if ('Create' === $payload['object']['type']) {

I would've thought that means it'd only update on comments that come in from external instances, but maybe I'm misunderstanding AP here. I assumed create = comes from owner instance, announce = comes from another instance, and owner is announcing it happened. Or maybe owner instances both create and announce create their own posts?

Yeah a create from a magazine and an announced create from a magazine should refresh the field. Until now the plain create did not do that, but it should, you're right

Ok, that might actually explain some things I had seen. Looking at this one magazine, it says "This magazine is not receiving updates (last activity 29 day(s) ago). " but has posts from today, yesterday etc, continuing to get posts, but they're all posts from that instance the magazine is from

So if I understand correctly, I need both sections, what I have here which updates from the origin's creates, and then what you suggested which updates when the origin announces creates as well

Well you alsp just get the creates from the user directly which would not count towards origin uodates.

And yes both sections would be good indeed

In this conversation I forgot the point I made in my latest comment. Sorry for the confusion on my part

src/MessageHandler/ActivityPub/Inbox/ActivityHandler.php

e-five256 · 2024-04-20T15:00:34Z

I think I've handled the situations as described now where it will update the value when a create or an announced create after 3 hours

BentiGorlich · 2024-05-03T10:21:22Z

src/MessageHandler/ActivityPub/Inbox/ActivityHandler.php

@@ -113,6 +117,14 @@ private function handle(?array $payload)
                    }
                }
            }
+        } elseif ('Create' === $payload['type']) {
+            $actor = $this->manager->findActorOrCreate($payload['actor']);
+            if ($actor instanceof Magazine && $actor->lastOriginUpdate < (new \DateTime())->modify('-3 hours')) {


This will not work, because the actor of a Create activity is never a magazine, but always a user. A magazine only announces what a user creates. If you get a plain Create activity you would have to check from which instance the user is from and then get the magazine the activity is posted to and then you can adjust the origin update.
However I think I removed that part (I initially had something that did the same) because you cannot be sure that you get create activities because you are subscribed to a magazine, because you will get create activities from users you are subscribed to, regardless of whether you're subscribed to the magazine they post it to. Announce activities on the other hand are only sent to people subscribed to magazines

BentiGorlich · 2024-05-03T10:23:34Z

src/MessageHandler/ActivityPub/Inbox/ActivityHandler.php

+
+                if ('Create' === $payload['type'] && $actor instanceof Magazine && $actor->lastOriginUpdate < (new \DateTime())->modify('-3 hours')) {
+                    // announced creates update magazine last origin timestamp when after threshold
+                    $actor->lastOriginUpdate = new \DateTime();


Could you rename this variable? I was confused whether this works, because this is not the actor of the $payload variable at this position.

Suggested change

$actor->lastOriginUpdate = new \DateTime();

$announceActor->lastOriginUpdate = new \DateTime();

e-five256 · 2024-05-13T23:56:29Z

I don't think I really have the expertise to make this change, as I'm basically taking a shot in the dark each time since I'm not familiar with AP and don't have a cluster to test what real events look like.

Not sure if this could be tackled by someone else or just closed. I think assuming @nobodyatroot isn't running it anymore since that one time since he's been running the tag PR since, maybe this isn't a problem that needs to be handled and the problem at the time was the foreign key constraint violation.

I'm just very, super overly optimized where I come from (even two DB calls in the same request are against my best designs, I'm like, forcing people to use refcursors if they need multiple data sets from a DB), whereas Mbin seems to be more like "haha, postgresql connections go brrrrrrrr"

BentiGorlich · 2024-05-29T10:33:36Z

I am happy to take this on. I am more on the side of "optimize where it is doable with reasonable effort". I agree that this should be optimized and I think I have the best understanding of the AP side in our team. And as my vacation "season" is over for now I should have a lot more time

do not update last origin update if within 1 hour of last update

8ffe49d

e-five256 added the backend Backend related issues and pull requests label Apr 11, 2024

reduce throttle from one hour to one minute

a1e7a94

e-five256 changed the title ~~do not update last origin update if within 1 hour of last update~~ reduce frequency of lastoriginupdate db writes Apr 12, 2024

Merge branch 'main' into e5/lastoriginupdate-less

822b109

e-five256 mentioned this pull request Apr 12, 2024

update magazine ban and log table foreign keys #703

Merged

nobodyatroot previously approved these changes Apr 12, 2024

View reviewed changes

nobodyatroot requested a review from BentiGorlich April 12, 2024 13:11

e-five256 added 2 commits April 12, 2024 20:12

Merge remote-tracking branch 'origin' into e5/lastoriginupdate-less

98c998e

change to update last origin on create payloads

8bd3ef2

change threshold for updates to 3 hours old

nobodyatroot added this to the v1.6.0 milestone Apr 13, 2024

BentiGorlich requested changes Apr 15, 2024

View reviewed changes

e-five256 marked this pull request as draft April 15, 2024 12:14

e-five256 added 5 commits April 19, 2024 19:10

Merge remote-tracking branch 'origin' into e5/lastoriginupdate-less

94802b7

also update on announced creates

6224ea1

use actor from top level ap payload

5c83b29

fix bad copy paste

dcdd3de

Merge branch 'main' into e5/lastoriginupdate-less

3ecaaaf

e-five256 marked this pull request as ready for review April 20, 2024 14:59

e-five256 and others added 3 commits April 20, 2024 13:08

Merge branch 'main' into e5/lastoriginupdate-less

e6e9137

Merge branch 'main' into e5/lastoriginupdate-less

e04204f

Merge branch 'main' into e5/lastoriginupdate-less

cdbc05a

nobodyatroot requested a review from BentiGorlich May 1, 2024 23:06

BentiGorlich requested changes May 3, 2024

View reviewed changes

e-five256 marked this pull request as draft May 3, 2024 12:20

nobodyatroot removed this from the v1.6.0 milestone May 14, 2024

BentiGorlich self-assigned this Jun 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reduce frequency of `lastoriginupdate` db writes #702

reduce frequency of `lastoriginupdate` db writes #702

e-five256 commented Apr 11, 2024

nobodyatroot commented Apr 11, 2024

BentiGorlich commented Apr 11, 2024

e-five256 commented Apr 11, 2024

BentiGorlich commented Apr 11, 2024

e-five256 commented Apr 11, 2024 •

edited

nobodyatroot left a comment

e-five256 commented Apr 12, 2024

BentiGorlich commented Apr 12, 2024

nobodyatroot commented Apr 12, 2024

e-five256 commented Apr 12, 2024

e-five256 commented Apr 13, 2024 •

edited

BentiGorlich Apr 15, 2024

e-five256 Apr 15, 2024

BentiGorlich Apr 15, 2024

e-five256 Apr 15, 2024

BentiGorlich Apr 15, 2024

BentiGorlich May 3, 2024

e-five256 commented Apr 20, 2024

BentiGorlich May 3, 2024

BentiGorlich May 3, 2024

e-five256 commented May 13, 2024 •

edited

BentiGorlich commented May 29, 2024

	$actor->lastOriginUpdate = new \DateTime();
	$announceActor->lastOriginUpdate = new \DateTime();

reduce frequency of lastoriginupdate db writes #702

Are you sure you want to change the base?

reduce frequency of lastoriginupdate db writes #702

Conversation

e-five256 commented Apr 11, 2024

nobodyatroot commented Apr 11, 2024

BentiGorlich commented Apr 11, 2024

e-five256 commented Apr 11, 2024

BentiGorlich commented Apr 11, 2024

e-five256 commented Apr 11, 2024 • edited

nobodyatroot left a comment

Choose a reason for hiding this comment

e-five256 commented Apr 12, 2024

BentiGorlich commented Apr 12, 2024

nobodyatroot commented Apr 12, 2024

e-five256 commented Apr 12, 2024

e-five256 commented Apr 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e-five256 commented Apr 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e-five256 commented May 13, 2024 • edited

BentiGorlich commented May 29, 2024

reduce frequency of `lastoriginupdate` db writes #702

reduce frequency of `lastoriginupdate` db writes #702

e-five256 commented Apr 11, 2024 •

edited

e-five256 commented Apr 13, 2024 •

edited

e-five256 commented May 13, 2024 •

edited