Remove `stream_positions` table #17047

realtyem · 2024-04-04T09:35:00Z

The stream_positions table is used to track what sequence number(as a stream id) has been last persisted into the database, and only for Postgres. Updating this table seems to be done often, but is only read from during startup. As all the data that is in that table can be discovered in each stream's actual tables, just use that at startup and don't bother with this extra table.

After start-up, this data is maintained in memory the same way it is today.

The motivator for this change:

2024-02-22 16:24:01.454 CST [127] LOG:  duration: 1836.864 ms  plan:
        Query Text:
                    INSERT INTO stream_positions (stream_name, instance_name, stream_id)
                    VALUES ('presence_stream', 'presence1', (87199721))
                    ON CONFLICT (stream_name, instance_name)
                    DO UPDATE SET
                        stream_id = EXCLUDED.stream_id
                    WHERE stream_positions.stream_id < EXCLUDED.stream_id

        Insert on stream_positions  (cost=0.00..0.01 rows=0 width=0) (actual time=1836.861..1836.862 rows=0 loops=1)
          Conflict Resolution: UPDATE
          Conflict Arbiter Indexes: stream_positions_idx
          Conflict Filter: (stream_positions.stream_id < excluded.stream_id)
          Tuples Inserted: 0
          Conflicting Tuples: 1
          ->  Result  (cost=0.00..0.01 rows=1 width=72) (actual time=0.002..0.003 rows=1 loops=1)
2024-02-22 16:24:01.538 CST [148] LOG:  process 148 still waiting for ExclusiveLock on tuple (0,110) of relation 20495 of database 19796 after 1000.080 ms
2024-02-22 16:24:01.538 CST [148] DETAIL:  Process holding the lock: 149. Wait queue: 151, 97, 150, 148, 124.
2024-02-22 16:24:01.538 CST [148] STATEMENT:
                    INSERT INTO stream_positions (stream_name, instance_name, stream_id)
                    VALUES ('presence_stream', 'presence1', (87199723))
                    ON CONFLICT (stream_name, instance_name)
                    DO UPDATE SET
                        stream_id = EXCLUDED.stream_id
                    WHERE stream_positions.stream_id < EXCLUDED.stream_id

2024-02-22 16:24:01.883 CST [127] LOG:  duration: 2265.752 ms  statement:
                    INSERT INTO stream_positions (stream_name, instance_name, stream_id)
                    VALUES ('presence_stream', 'presence1', (87199721))
                    ON CONFLICT (stream_name, instance_name)
                    DO UPDATE SET
                        stream_id = EXCLUDED.stream_id
                    WHERE stream_positions.stream_id < EXCLUDED.stream_id

Let's just...not do this 🤷‍♂️

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file.
Code style is correct
(run the linters)

…t's near the beginning of the init process. Just set it to the 1 as the default.

…th new SQL looking up the data directly in the associated tables for a given stream writer

…sts will fail at this point

…add some explanation around the rationale

…ike lines on my scrollbar in pycharm)

…xisting_stream() Without a `stream_positions` table to lean on, this only needed a few small tweaks to wording and actual representative positions

realtyem · 2024-04-04T10:18:45Z

synapse/storage/util/id_generators.py

@@ -451,7 +443,8 @@ def __init__(
            self._current_positions.values(), default=1
        )

-        # For the case where `stream_positions` is not up to date,
+        # TODO: this needs updating or verifying


RE: TODO comment
Actually, I believe this block can be removed. _max_seen_allocated_stream_id is already set just above and will already equal what _persisted_upto_position is because of what happens inside of _load_current_ids().

Either it already is set to the max()(see new line 519)
or it is already set to the max() through iteration of _add_persisted_positions()(see new line 561)

(I see my ability to show the spot correctly has not improved with time)

realtyem · 2024-04-04T10:26:44Z

synapse/storage/util/id_generators.py

+            # to be persisted or will default to 1.
+            # TODO: is this next comment still valid?
+            # (This can be a problem for e.g. backfill streams where the server has
+            # never backfilled).


RE: TODO comment
I believe this is resolved by the setting of max_stream_id to 1 at the top of the function. If I'm following the juggling of negatives and positives, this one is always positive?

realtyem · 2024-04-04T11:03:48Z

tests/storage/test_id_generators.py

-        # If we add back the old "first" then we shouldn't see the persisted up
-        # to position revert back to 3.
+        # If we add back the old "first" then we should see the persisted up
+        # to position revert back to 3, as this writer hasn't written anything since.


I postulate that it actually should be a 3 and not a 6, as "first" hasn't actually written anything since it's initial 3 reflecting it's minimum persisted value.

I assert that the first instance created in this test is proof, since it's
instance name is neither of the writers named and is therefore a 'reader' and not a 'writer', just as this 5th instance is.

erikjohnston · 2024-04-04T11:23:07Z

This table got added in matrix-org/synapse#8374, does the rationale no longer make sense?

realtyem · 2024-04-04T12:06:38Z

This table got added in matrix-org/synapse#8374, does the rationale no longer make sense?

Thank you for pointing at this, I was not aware of the 'why' behind it. I will have to process what all of that means for a while, I think. Your question is worth a good response.

It does appear that the majority of the tables do not have indexes on instance_name, so if my Postgres is up to par means that a table scan would end up being required.

However, I believe eliminating the excess of dead tuples in Postgres(not to mention the wear on hard drives) in place of a table scan on startup may be worth the effort. I don't suppose you recall how long it was taking for the table scan, as whatever evidence my server has would not be particularly useful next to a larger instance such as matrix.org?(Just as a curiosity)

erikjohnston · 2024-04-04T12:26:26Z

I agree that it would be good to find a way of reducing the DB load due to it, just not quite sure how atm 🙂

realtyem · 2024-04-05T08:37:44Z

I think I have a different angle to try on this, so I will close it

realtyem added 11 commits March 30, 2024 22:57

Toss out almost all references to 'stream_positions'

de5fb2c

_current_positions will have no values at this point in startup, as i…

62e053e

…t's near the beginning of the init process. Just set it to the 1 as the default.

Replace the SQL loading positions from stream_positions at start wi…

ab5484c

…th new SQL looking up the data directly in the associated tables for a given stream writer

Update unit tests to remove references to stream_positions. Some te…

2e3c09b

…sts will fail at this point

Remove old generation of min_stream_id as it is already handled, and …

aff92f0

…add some explanation around the rationale

This can all be removed as it was handled above

cec51f6

Drive-by small comment typo/grammer/spelling fixups(because I don't l…

644fe9b

…ike lines on my scrollbar in pycharm)

Update unit test MultiTableMultiWriterIdGeneratorTestCase.test_load_e…

0c8ded9

…xisting_stream() Without a `stream_positions` table to lean on, this only needed a few small tweaks to wording and actual representative positions

Update unit test MultiWriterIdGenerator.test_writer_config_change()

12e8977

save a note

547569d

changelog

e98b03b

realtyem marked this pull request as ready for review April 4, 2024 09:56

realtyem requested a review from a team as a code owner April 4, 2024 09:56

realtyem commented Apr 4, 2024

View reviewed changes

Remove a TODO comment from test

8bc9595

realtyem commented Apr 4, 2024

View reviewed changes

erikjohnston marked this pull request as draft April 4, 2024 12:26

erikjohnston removed the request for review from a team April 4, 2024 12:26

realtyem closed this Apr 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `stream_positions` table #17047

Remove `stream_positions` table #17047

realtyem commented Apr 4, 2024 •

edited

Loading

realtyem Apr 4, 2024 •

edited

Loading

realtyem Apr 4, 2024

realtyem Apr 4, 2024 •

edited

Loading

realtyem Apr 4, 2024

erikjohnston commented Apr 4, 2024

realtyem commented Apr 4, 2024

erikjohnston commented Apr 4, 2024 •

edited

Loading

realtyem commented Apr 5, 2024

Remove stream_positions table #17047

Remove stream_positions table #17047

Conversation

realtyem commented Apr 4, 2024 • edited Loading

Pull Request Checklist

realtyem Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

realtyem Apr 4, 2024

Choose a reason for hiding this comment

realtyem Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

realtyem Apr 4, 2024

Choose a reason for hiding this comment

erikjohnston commented Apr 4, 2024

realtyem commented Apr 4, 2024

erikjohnston commented Apr 4, 2024 • edited Loading

realtyem commented Apr 5, 2024

Remove `stream_positions` table #17047

Remove `stream_positions` table #17047

realtyem commented Apr 4, 2024 •

edited

Loading

realtyem Apr 4, 2024 •

edited

Loading

realtyem Apr 4, 2024 •

edited

Loading

erikjohnston commented Apr 4, 2024 •

edited

Loading