Re: [PATCH] Two small track_table_mutation follow-ups: doc figures and unused counters

Hi Nadav,

> While re-reading the committed track_table_mutation code I found three
> small things that are wrong in master. Two patches attached, split by
> concern.
>
> 0001 - Doc: fix memory figures and eviction description
>
> The docs understate the feature's shared memory and describe its eviction
> order incorrectly.
>
> TrackTableMutationEntry is 56 bytes, not the documented 40. It holds two
> struct timeval fields (16 bytes each on LP64), two ints, a uint32, an int
> and a bool, which pad to 56. I confirmed this by compiling the struct on
> both Linux (aarch64) and macOS; sizeof is 56 on both, with in_use at
> offset 48.
>
> So at the default track_table_mutation_table_size of 2048 the entries
> alone are ~112 KB, plus ~4 KB for the default 1024-entry bucket array,
> giving ~116 KB rather than the documented ~80 KB. The 4096-table example
> needs ~228 KB, not ~160 KB. The bucket array was not accounted for at all
> before, so the patch mentions it explicitly.
>
> Separately, the docs say that when the map is full "oldest entries are
> evicted". table_map_insert() actually scans from bucket zero and evicts
> the head of the first non-empty bucket, so eviction order follows the
> hash bucket layout rather than write time. The patch describes what the
> code does, and adds the mitigating detail that expired entries are
> reclaimed opportunistically, so a live entry is only evicted when more
> than track_table_mutation_table_size tables are written within one TTL
> window.

Good findings and the patch looks good to me.

> I only touched the English documentation. doc.ja carries the same
> figures and the same eviction wording, but I would rather leave that to
> you than guess at the phrasing.

Sure. I will take care of the Japanese doc.

> 0002 - Remove unused track_table_mutation statistics counters
>
> TrackTableMutationState carries three counters - stats_queries_checked,
> stats_forced_primary and stats_allowed_replica - that are initialized and
> incremented but never read. Nothing in the tree consumes them; there is
> no SHOW, reporting view or pcp command that exposes them, and the
> pool_track_table_mutation_get_stats() accessor that would have is one of
> the functions removed before commit.
>
> They are not free. pool_track_table_mutation_table_is_stale() releases
> the table map semaphore after the lookup and then re-acquires it only to
> bump these counters, so every staleness check on every load-balanceable
> SELECT makes a second trip through the semaphore to update state nobody
> can observe. Removing them drops that critical section and lets the
> table_oid/dboid sanity check return directly rather than jumping to a
> shared epilogue.
>
> The return value of pool_track_table_mutation_table_is_stale() is
> unchanged for every input, so there is no functional change.
>
> Both patches apply cleanly to current master, build without new warnings,
> and pass the 78-column check from the committing checklist.

Looks good to me.

I am going to commit both of your patches along with Japanese doc
update.

Regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp