Filter out auth chain queries that don't exist

[realtyem]

Short version:

It is kind of a logic fix in that you're doing less work by disregarding the sentinel value of 0. This has two improvements:

1. Avoiding expanding the chains dictionary unnecessarily.
2. Avoiding passing more arguments through the SQL engine (and to the database) which we know will result in no matches.

Long version:

In _get_auth_chain_ids_using_cover_index_txn():

synapse/synapse/storage/databases/main/event_federation.py

Lines 300 to 304 in c14a7de

	# Add the initial set of chains, excluding the sequence corresponding to
	# initial event.
	for chain_id, seq_no in event_chains.items():
	chains[chain_id] = max(seq_no - 1, chains.get(chain_id, 0))

Seems to have a small logic problem, where the max() can evaluate to 0. The chains dict is passed into the following SQL block:

synapse/synapse/storage/databases/main/event_federation.py

Lines 314 to 325 in c14a7de

	# We can use `execute_values` to efficiently fetch the gaps when
	# using postgres.
	sql = """
	SELECT event_id
	FROM event_auth_chains AS c, (VALUES ?) AS l(chain_id, max_seq)
	WHERE
	c.chain_id = l.chain_id
	AND sequence_number <= max_seq
	"""
	rows = txn.execute_values(sql, chains.items())
	results.update(r for r, in rows)

where it inhabits the max_seq variable. Inside the event_auth_chains database table, that column can never be 0.

Filter out that 0 before it gets to the SQL.

The temporary metric attached produces results that appear like this(over less than 24 hours):

The metric commit isn't particularly efficient and has no long term relevance, so it will not be included in the final render.

Pull Request Checklist

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

Signed-off-by: Jason Little realtyem@gmail.com

[DMRobertson]

I agree that chain IDs are never 0. They come from a DB-backed sequence:

synapse/synapse/storage/databases/main/events_worker.py

Lines 293 to 300 in 12ca87f

	self.event_chain_id_gen = build_sequence_generator(
	db_conn,
	database.engine,
	get_chain_id_txn,
	"event_auth_chain_id",
	table="event_auth_chains",
	id_column="chain_id",
	)

synapse/synapse/storage/schema/main/delta/59/04_event_auth_chains.sql.postgres

Line 16 in 25f43fa

CREATE SEQUENCE IF NOT EXISTS event_auth_chain_id;

which per https://www.postgresql.org/docs/16/sql-createsequence.html#id-1.9.3.81.6 should start at 1 and increase by 1.

Therefore querying for events in chain 0 will never return anything.

[DMRobertson]

Oh, your point is that the sequence_number in the chains is never 0?

[DMRobertson]

It looks like these are also seeded at 1 and grow by 1:

synapse/synapse/storage/databases/main/events.py

Lines 941 to 950 in e9069c9

	# We found a chain ID/sequence number candidate, check its
	# not already taken.
	proposed_new_id = existing_chain_id[0]
	proposed_new_seq = existing_chain_id[1] + 1
	if chain_to_max_seq_no[proposed_new_id] < proposed_new_seq:
	new_chain_tuple = (
	proposed_new_id,
	proposed_new_seq,
	)

[clokep]

I wonder if comparing to != 0 would be clearer?

Suggested change

	if max_sequence_result > 0:
	if max_sequence_result != 0:

[DMRobertson]

Do we understand when max_sequence_result is 0?

I think I've convinced myself that sequence numbers are strictly positive, and chains is a map to sequence numbers. Therefore we would need to have seq_no == 1 and chain_id not in chains. Is there any particular meaning to this situation?

[DMRobertson]

I guess this corresponds to a brand-new chain that isn't the target of some other chain. Which sounds like the kind of thing that only happens at the start of a room? (cc @erikjohnston: is any of this sane?)

[erikjohnston]

This is basically handling the case where an initial event is the first thing in the chain (and nothing else references it). This is actually pretty common, as we start a new chain whenever we see a new type / state key pair.

[DMRobertson]

With your change, is it possible that chains can be empty after this loop? If so, will the query below fall over if we pass in an empty chains.items() to execute_values?

[DMRobertson]

I think this is probably fine, but I was careful to double-check this since event auth chain logic is important to keep watertight. I would like @erikjohnston to double-check though.

[erikjohnston]

I'd think I'd replace this change with something like:

        for chain_id, seq_no in event_chains.items():
            # Check if the initial event is the first item in the chain. If so, then
            # there is nothing new to add from this chain.
            if seq_no == 1:
                continue
                
            chains[chain_id] = max(seq_no - 1, chains.get(chain_id, 0))

Maybe?

[erikjohnston]

Not sure if that is clearer?

[realtyem]

Works for me

[realtyem]

Replaced logic in 94e22c2

Would you like me to lose that comment above the condition?

[erikjohnston]

Thanks! Yeah, I think that comment is now redundant

[realtyem]

Removed in 3eaeaff

[erikjohnston]

❤️