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.

I have pushed 0001 and 002 as the separate committs.
(001 includes the Japanese doc update).

https://git.postgresql.org/gitweb/?p=pgpool2.git;a=commit;h=17352edb15faf64752a22fd4f1491cf3f268cd19
https://git.postgresql.org/gitweb/?p=pgpool2.git;a=commit;h=91c85223dce3718cc321b582802e9c39f8b0a8b9

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