Repository navigation
Keep the client name a query was made with - #3302
Merged
Merged
Conversation
DL6ER
reviewed
Oct 25, 2026
DL6ER
left a comment
DL6ER
left a comment
Member
There was a problem hiding this comment.
One change before I can approve: please take the held reply out of the test, the upstream in Python, the release file and the dig in the background. It only covers a query that is stored again after a rename, which the per-second pass makes a rare case. With the pass gone in step 4 it becomes the normal case, and a plain sequence can check it there. The rest of the test covers what step 4 will rewrite, the names through export and import, so that part is worth its two restarts.
Your three questions:
- No shortcut for now. +3 ms per 100k queries is fine, the code it would add matters more. It can still come as its own change later.
- The cap is fine. A log line the first time it is reached would make it easy to spot.
- No, queries made before a client was resolved keep no name. That follows the rule and matches the rows on disk.
#3054: noted, whichever of the two is merged second gets the rename_client() call in flush_network_table().
010011110
added a commit
to 010011110/FTL
that referenced
this pull request
Oct 25, 2026
A tool for moving the query log from the in-memory database to shared memory in pi-hole#3275, not meant to be merged: it is closed once step 4 is done. Every request goes to the in-memory query log and, with `disk=true`, to the on-disk database. FTL imports a fixture at startup, so both hold the same rows and have to give the same answer. The disk path, SQL on `pihole-FTL.db`, is the reference. `test/query_log_diff.py` fills a database FTL has created with 10050 rows of one domain and client, more than the 10000-row cap, and 760 varied ones: every status, reply type and DNSSEC state, mapped and unmapped types including 100 (qtype 0), a range of EDE codes, rows without `list_id` and `ede`, negative list IDs, several clients and upstreams, the hidden client and CNAME target of the privacy levels, domains with `_`, `%`, a backslash and mixed case, CNAME statuses, timestamps with microseconds, slightly out of order, tied and two hours off for some rows, ID gaps, and a client whose name changes. From the data it builds about 320 requests with the parameters of the API specification and the web interface: every filter alone and in pairs, wildcards, the column searches with `_`, `%`, `\_` and upper case, every sort column in both directions, a cursor below the newest ID with sorts and filters, first, middle and last pages and other start values, the cap, time ranges among the rows that are off, and invalid parameters, which have to give the same error. Every other request has to succeed on disk. Each request is paged through on both paths and compared as a whole: the page lengths and keys, `draw`, `cursor`, `recordsTotal` and `recordsFiltered`, the sort keys in the same order and the rows, where rows with equal keys may come in any order. A second run adds exclusion regexes that also match client names and other case, and has to leave rows out. `query_log_diff.py paging` checks the order of equal keys in memory, by ID in the direction of the sort, also where a page in the back half is read from the other end: it pages through the results sorted by status and by reply type with 10, 100, 1000, N/2 and 7000 rows per page, and takes single pages sorted by time, domain and list ID, with and without a filter and with older cursors, at the end, past it, in the middle and where the result is read from the other end. Each page has to be exactly its slice of the order by key and ID, with the counts of the whole result. Three more FTL starts check what the fixture cannot show: - `time-order` on a fixture with every timestamp in ID order, where the time order is read straight off the queries array. - `live` at privacy levels 1 and 2: queries blocked by the test gravity database, answered locally, over TCP, in flight to an upstream that never answers, and blocked by a CNAME this upstream answers with, each of those with a name of its own, as one identical to a query in flight would get the answer as a duplicate. Their rows are read from memory once FTL has answered all but those in flight, most of them before they are stored, and have to hold each of these kinds. `stored` compares them with the database after the next start, as FTL stores everything when it exits. Not compared are `took` and the values of `earliest_timestamp` and `earliest_timestamp_disk`, which the garbage collection moves. With the name history of 3a (pi-hole#3302) memory and disk show the name each query was made with, so `test_final.bats` compares names with `--strict-names`. Without it, the requests that depend on names are left out and counted, and a name is only compared where the in-memory log shows one. A client without a name has to show null in memory, not an empty name. `test/query_log_diff.sh` runs a given `pihole-FTL` on ports of its own. It starts FTL without the caller's `FTLCONF_` variables and with `misc.readOnly`, so the test's settings stay out of `pihole.toml`, and with DHCP and `/etc/dnsmasq.d` off. It refuses to run next to a `pihole.toml` that is neither its own nor the suite's, and outside the suite also next to the `setupVars.conf`, `pihole-FTL.conf` or `gravity.db` of an installation: one of v5 would not be migrated once `pihole.toml` exists. It waits for each FTL it stops to exit, and stops the one it started last on any exit, also on Ctrl-C and while that FTL is still starting. The two files also run standalone against any build in a fresh container. `test_final.bats` runs them without `--strict-names` at the end of the suite, in about 45 s, 60 s with the suite's debug flags, and skips them on riscv64 like the API tests. Signed-off-by: 010011110 <duckenheim@posteo.de>
5 tasks done
Contributor
Author
|
thanks, done in a commit on top: the held reply is out of the test, the re-store after a rename gets a plain test in step 4, and the history logs one warning the first time it is full |
010011110
added a commit
to 010011110/FTL
that referenced
this pull request
Oct 25, 2026
A tool for moving the query log from the in-memory database to shared memory in pi-hole#3275, not meant to be merged: it is closed once step 4 is done. Every request goes to the in-memory query log and, with `disk=true`, to the on-disk database. FTL imports a fixture at startup, so both hold the same rows and have to give the same answer. The disk path, SQL on `pihole-FTL.db`, is the reference. `test/query_log_diff.py` fills a database FTL has created with 10050 rows of one domain and client, more than the 10000-row cap, and 760 varied ones: every status, reply type and DNSSEC state, mapped and unmapped types including 100 (qtype 0), a range of EDE codes, rows without `list_id` and `ede`, negative list IDs, several clients and upstreams, the hidden client and CNAME target of the privacy levels, domains with `_`, `%`, a backslash and mixed case, CNAME statuses, timestamps with microseconds, slightly out of order, tied and two hours off for some rows, ID gaps, and a client whose name changes. From the data it builds about 320 requests with the parameters of the API specification and the web interface: every filter alone and in pairs, wildcards, the column searches with `_`, `%`, `\_` and upper case, every sort column in both directions, a cursor below the newest ID with sorts and filters, first, middle and last pages and other start values, the cap, time ranges among the rows that are off, and invalid parameters, which have to give the same error. Every other request has to succeed on disk. Each request is paged through on both paths and compared as a whole: the page lengths and keys, `draw`, `cursor`, `recordsTotal` and `recordsFiltered`, the sort keys in the same order and the rows, where rows with equal keys may come in any order. A second run adds exclusion regexes that also match client names and other case, and has to leave rows out. `query_log_diff.py paging` checks the order of equal keys in memory, by ID in the direction of the sort, also where a page in the back half is read from the other end: it pages through the results sorted by status and by reply type with 10, 100, 1000, N/2 and 7000 rows per page, and takes single pages sorted by time, domain and list ID, with and without a filter and with older cursors, at the end, past it, in the middle and where the result is read from the other end. Each page has to be exactly its slice of the order by key and ID, with the counts of the whole result. Three more FTL starts check what the fixture cannot show: - `time-order` on a fixture with every timestamp in ID order, where the time order is read straight off the queries array. - `live` at privacy levels 1 and 2: queries blocked by the test gravity database, answered locally, over TCP, in flight to an upstream that never answers, and blocked by a CNAME this upstream answers with, each of those with a name of its own, as one identical to a query in flight would get the answer as a duplicate. Their rows are read from memory once FTL has answered all but those in flight, most of them before they are stored, and have to hold each of these kinds. `stored` compares them with the database after the next start, as FTL stores everything when it exits. Not compared are `took` and the values of `earliest_timestamp` and `earliest_timestamp_disk`, which the garbage collection moves. With the name history of 3a (pi-hole#3302) memory and disk show the name each query was made with, so `test_final.bats` compares names with `--strict-names`. Without it, the requests that depend on names are left out and counted, and a name is only compared where the in-memory log shows one. A client without a name has to show null in memory, not an empty name. `test/query_log_diff.sh` runs a given `pihole-FTL` on ports of its own. It starts FTL without the caller's `FTLCONF_` variables and with `misc.readOnly`, so the test's settings stay out of `pihole.toml`, and with DHCP and `/etc/dnsmasq.d` off. It refuses to run next to a `pihole.toml` that is neither its own nor the suite's, and outside the suite also next to the `setupVars.conf`, `pihole-FTL.conf` or `gravity.db` of an installation: one of v5 would not be migrated once `pihole.toml` exists. It waits for each FTL it stops to exit, and stops the one it started last on any exit, also on Ctrl-C and while that FTL is still starting. The two files also run standalone against any build in a fresh container. `test_final.bats` runs them without `--strict-names` at the end of the suite, in about 45 s, 60 s with the suite's debug flags, and skips them on riscv64 like the API tests. Signed-off-by: 010011110 <duckenheim@posteo.de>
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Shared memory holds one name per client, the one the resolver found last. The query log from shared memory showed it for every query, while the database has the name a row was stored with, which the export took from the client at the time of the export, also for queries made before a rename. Now every query keeps the name its client had when the query was made, in shared memory and on disk alike. When the resolver renames a client, `rename_client()` records the old name in an entry: the client's queries with an ID below `until_id` had the name at `namepos`, the next query ID marks the boundary. Entries are sorted by client and ID. `client_name_at()` finds the name of a query with a binary search over them and falls back to the current name. The query log (`query_client_name()`) and the export (`getClientNameString()`) use it, so filters, sorts, the column search, the exclusion regexes and the rows on disk all see the same name. The query log's filters look a name up only when they use it, a filter by client IP alone does not. A query made before a rename takes the `client_by_id` row of its own name rather than the one cached for the client. The import reads the name each query was stored with, in the order of the IDs, and keeps every run of queries with the same name as an entry. The `queries` view has no row ID of the client, so the import selects its columns with the joins of the on-disk view itself, plus the name. Imported queries thus show their stored names right away instead of the client's current one. `runGC()` drops the entries below the oldest query in memory. An entry takes 16 bytes and the history holds at most 65536 of them (1 MiB), renames beyond that are not remembered. Only the main process uses the history (resolver, import, export, query log and GC), always with the SHM lock held, so it lives in its heap and the shared memory layout stays as it is. Signed-off-by: 010011110 <duckenheim@posteo.de>
The new test in test_final.bats lets FTL resolve 127.0.0.20 through dns.hosts: the client gets a name after its first query and another one after its second. Its query of held.names.ftl goes to an upstream that answers only after the rename, so the export stores that query again then, with the name it was made with, while the next query of the client takes the row of the new name. In shared memory before a restart, and both in shared memory and on disk after it, the output, the filter by name, the column search, the sort by name and the exclusion regexes go by the name each query was made with. The test configuration's exclusion regex is cleared first, as the query log looks names up for it, so a filter by client IP alone runs without them. Two rows of another client with consecutive IDs and different names check the boundary between imported names. The first instance does not import the queries of the earlier tests and the second one resolves no names, so the resolver has only the clients of this test to look up. Signed-off-by: 010011110 <duckenheim@posteo.de>
The history keeps at most 65536 entries. When it is full, FTL now logs a warning the first time, so a history that stops remembering renames is easy to spot. Nothing changes for the lookups. The test no longer holds back the answer to one query until after the rename. That case, a query stored again after its client was renamed, is rare while queries are stored every second and becomes the normal one once the in-memory database is gone, where a plain sequence can check it. The test keeps the names through the export and the import. Signed-off-by: 010011110 <duckenheim@posteo.de>
The import takes its columns with the joins of the on-disk `queries` view of database version 22, which uses `COALESCE()`: an ID without a row in its linking table reads as the ID itself. The view of the in-memory database that the import read before uses `CASE typeof()` and reads such an ID as NULL, so the import skipped the row. With the joins a row whose domain or client row is missing was imported with the number as its domain or client, as the test of the skipped rows in test_final.bats shows. The import now takes `CASE typeof()` with the joins, as the view of the in-memory database does, and skips these rows again. Signed-off-by: 010011110 <duckenheim@posteo.de>
010011110
force-pushed
the
client-name-history-3a
branch
from
October 26, 2026 05:30
9ad9241 to
11f6593
Compare
010011110
added a commit
to 010011110/FTL
that referenced
this pull request
Oct 26, 2026
A tool for moving the query log from the in-memory database to shared memory in pi-hole#3275, not meant to be merged: it is closed once step 4 is done. Every request goes to the in-memory query log and, with `disk=true`, to the on-disk database. FTL imports a fixture at startup, so both hold the same rows and have to give the same answer. The disk path, SQL on `pihole-FTL.db`, is the reference. `test/query_log_diff.py` fills a database FTL has created with 10050 rows of one domain and client, more than the 10000-row cap, and 760 varied ones: every status, reply type and DNSSEC state, mapped and unmapped types including 100 (qtype 0), a range of EDE codes, rows without `list_id` and `ede`, negative list IDs, several clients and upstreams, the hidden client and CNAME target of the privacy levels, domains with `_`, `%`, a backslash and mixed case, CNAME statuses, timestamps with microseconds, slightly out of order, tied and two hours off for some rows, ID gaps, and a client whose name changes. From the data it builds about 320 requests with the parameters of the API specification and the web interface: every filter alone and in pairs, wildcards, the column searches with `_`, `%`, `\_` and upper case, every sort column in both directions, a cursor below the newest ID with sorts and filters, first, middle and last pages and other start values, the cap, time ranges among the rows that are off, and invalid parameters, which have to give the same error. Every other request has to succeed on disk. Each request is paged through on both paths and compared as a whole: the page lengths and keys, `draw`, `cursor`, `recordsTotal` and `recordsFiltered`, the sort keys in the same order and the rows, where rows with equal keys may come in any order. A second run adds exclusion regexes that also match client names and other case, and has to leave rows out. `query_log_diff.py paging` checks the order of equal keys in memory, by ID in the direction of the sort, also where a page in the back half is read from the other end: it pages through the results sorted by status and by reply type with 10, 100, 1000, N/2 and 7000 rows per page, and takes single pages sorted by time, domain and list ID, with and without a filter and with older cursors, at the end, past it, in the middle and where the result is read from the other end. Each page has to be exactly its slice of the order by key and ID, with the counts of the whole result. Three more FTL starts check what the fixture cannot show: - `time-order` on a fixture with every timestamp in ID order, where the time order is read straight off the queries array. - `live` at privacy levels 1 and 2: queries blocked by the test gravity database, answered locally, over TCP, in flight to an upstream that never answers, and blocked by a CNAME this upstream answers with, each of those with a name of its own, as one identical to a query in flight would get the answer as a duplicate. Their rows are read from memory once FTL has answered all but those in flight, most of them before they are stored, and have to hold each of these kinds. `stored` compares them with the database after the next start, as FTL stores everything when it exits. Not compared are `took` and the values of `earliest_timestamp` and `earliest_timestamp_disk`, which the garbage collection moves. With the name history of 3a (pi-hole#3302) memory and disk show the name each query was made with, so `test_final.bats` compares names with `--strict-names`. Without it, the requests that depend on names are left out and counted, and a name is only compared where the in-memory log shows one. A client without a name has to show null in memory, not an empty name. `test/query_log_diff.sh` runs a given `pihole-FTL` on ports of its own. It starts FTL without the caller's `FTLCONF_` variables and with `misc.readOnly`, so the test's settings stay out of `pihole.toml`, and with DHCP and `/etc/dnsmasq.d` off. It refuses to run next to a `pihole.toml` that is neither its own nor the suite's, and outside the suite also next to the `setupVars.conf`, `pihole-FTL.conf` or `gravity.db` of an installation: one of v5 would not be migrated once `pihole.toml` exists. It waits for each FTL it stops to exit, and stops the one it started last on any exit, also on Ctrl-C and while that FTL is still starting. The two files also run standalone against any build in a fresh container. `test_final.bats` runs them without `--strict-names` at the end of the suite, in about 45 s, 60 s with the suite's debug flags, and skips them on riscv64 like the API tests. Signed-off-by: 010011110 <duckenheim@posteo.de>
Contributor
Author
|
rebased onto development for the conflict with #3300, both tests are kept. its test of the skipped rows then caught something in here: the import took its joins from the version 22 view, whose COALESCE reads an ID without a row in its linking table as the ID itself, while the in-memory view it replaced reads it as NULL and so skips the row. fixed in a commit on top, the import uses CASE typeof() with the joins now |
|
Conflicts have been resolved. |
5 tasks done
010011110
added a commit
to 010011110/FTL
that referenced
this pull request
Oct 26, 2026
A tool for moving the query log from the in-memory database to shared memory in pi-hole#3275, not meant to be merged: it is closed once step 4 is done. Every request goes to the in-memory query log and, with `disk=true`, to the on-disk database. FTL imports a fixture at startup, so both hold the same rows and have to give the same answer. The disk path, SQL on `pihole-FTL.db`, is the reference. `test/query_log_diff.py` fills a database FTL has created with 10050 rows of one domain and client, more than the 10000-row cap, and 760 varied ones: every status, reply type and DNSSEC state, mapped and unmapped types including 100 (qtype 0), a range of EDE codes, rows without `list_id` and `ede`, negative list IDs, several clients and upstreams, the hidden client and CNAME target of the privacy levels, domains with `_`, `%`, a backslash and mixed case, CNAME statuses, timestamps with microseconds, slightly out of order, tied and two hours off for some rows, ID gaps, and a client whose name changes. From the data it builds about 320 requests with the parameters of the API specification and the web interface: every filter alone and in pairs, wildcards, the column searches with `_`, `%`, `\_` and upper case, every sort column in both directions, a cursor below the newest ID with sorts and filters, first, middle and last pages and other start values, the cap, time ranges among the rows that are off, and invalid parameters, which have to give the same error. Every other request has to succeed on disk. Each request is paged through on both paths and compared as a whole: the page lengths and keys, `draw`, `cursor`, `recordsTotal` and `recordsFiltered`, the sort keys in the same order and the rows, where rows with equal keys may come in any order. A second run adds exclusion regexes that also match client names and other case, and has to leave rows out. `query_log_diff.py paging` checks the order of equal keys in memory, by ID in the direction of the sort, also where a page in the back half is read from the other end: it pages through the results sorted by status and by reply type with 10, 100, 1000, N/2 and 7000 rows per page, and takes single pages sorted by time, domain and list ID, with and without a filter and with older cursors, at the end, past it, in the middle and where the result is read from the other end. Each page has to be exactly its slice of the order by key and ID, with the counts of the whole result. Three more FTL starts check what the fixture cannot show: - `time-order` on a fixture with every timestamp in ID order, where the time order is read straight off the queries array. - `live` at privacy levels 1 and 2: queries blocked by the test gravity database, answered locally, over TCP, in flight to an upstream that never answers, and blocked by a CNAME this upstream answers with, each of those with a name of its own, as one identical to a query in flight would get the answer as a duplicate. Their rows are read from memory once FTL has answered all but those in flight, most of them before they are stored, and have to hold each of these kinds. `stored` compares them with the database after the next start, as FTL stores everything when it exits. Not compared are `took` and the values of `earliest_timestamp` and `earliest_timestamp_disk`, which the garbage collection moves. With the name history of 3a (pi-hole#3302) memory and disk show the name each query was made with, so `test_final.bats` compares names with `--strict-names`. Without it, the requests that depend on names are left out and counted, and a name is only compared where the in-memory log shows one. A client without a name has to show null in memory, not an empty name. `test/query_log_diff.sh` runs a given `pihole-FTL` on ports of its own. It starts FTL without the caller's `FTLCONF_` variables and with `misc.readOnly`, so the test's settings stay out of `pihole.toml`, and with DHCP and `/etc/dnsmasq.d` off. It refuses to run next to a `pihole.toml` that is neither its own nor the suite's, and outside the suite also next to the `setupVars.conf`, `pihole-FTL.conf` or `gravity.db` of an installation: one of v5 would not be migrated once `pihole.toml` exists. It waits for each FTL it stops to exit, and stops the one it started last on any exit, also on Ctrl-C and while that FTL is still starting. The two files also run standalone against any build in a fresh container. `test_final.bats` runs them without `--strict-names` at the end of the suite, in about 45 s, 60 s with the suite's debug flags, and skips them on riscv64 like the API tests. Signed-off-by: 010011110 <duckenheim@posteo.de>
010011110
added a commit
to 010011110/FTL
that referenced
this pull request
Oct 26, 2026
A tool for moving the query log from the in-memory database to shared memory in pi-hole#3275, not meant to be merged: it is closed once step 4 is done. Every request goes to the in-memory query log and, with `disk=true`, to the on-disk database. FTL imports a fixture at startup, so both hold the same rows and have to give the same answer. The disk path, SQL on `pihole-FTL.db`, is the reference. `test/query_log_diff.py` fills a database FTL has created with 10050 rows of one domain and client, more than the 10000-row cap, and 760 varied ones: every status, reply type and DNSSEC state, mapped and unmapped types including 100 (qtype 0), a range of EDE codes, rows without `list_id` and `ede`, negative list IDs, several clients and upstreams, the hidden client and CNAME target of the privacy levels, domains with `_`, `%`, a backslash and mixed case, CNAME statuses, timestamps with microseconds, slightly out of order, tied and two hours off for some rows, ID gaps, and a client whose name changes. From the data it builds about 320 requests with the parameters of the API specification and the web interface: every filter alone and in pairs, wildcards, the column searches with `_`, `%`, `\_` and upper case, every sort column in both directions, a cursor below the newest ID with sorts and filters, first, middle and last pages and other start values, the cap, time ranges among the rows that are off, and invalid parameters, which have to give the same error. Every other request has to succeed on disk. Each request is paged through on both paths and compared as a whole: the page lengths and keys, `draw`, `cursor`, `recordsTotal` and `recordsFiltered`, the sort keys in the same order and the rows, where rows with equal keys may come in any order. A second run adds exclusion regexes that also match client names and other case, and has to leave rows out. `query_log_diff.py paging` checks the order of equal keys in memory, by ID in the direction of the sort, also where a page in the back half is read from the other end: it pages through the results sorted by status and by reply type with 10, 100, 1000, N/2 and 7000 rows per page, and takes single pages sorted by time, domain and list ID, with and without a filter and with older cursors, at the end, past it, in the middle and where the result is read from the other end. Each page has to be exactly its slice of the order by key and ID, with the counts of the whole result. Three more FTL starts check what the fixture cannot show: - `time-order` on a fixture with every timestamp in ID order, where the time order is read straight off the queries array. - `live` at privacy levels 1 and 2: queries blocked by the test gravity database, answered locally, over TCP, in flight to an upstream that never answers, and blocked by a CNAME this upstream answers with, each of those with a name of its own, as one identical to a query in flight would get the answer as a duplicate. Their rows are read from memory once FTL has answered all but those in flight, most of them before they are stored, and have to hold each of these kinds. `stored` compares them with the database after the next start, as FTL stores everything when it exits. Not compared are `took` and the values of `earliest_timestamp` and `earliest_timestamp_disk`, which the garbage collection moves. With the name history of 3a (pi-hole#3302) memory and disk show the name each query was made with, so `test_final.bats` compares names with `--strict-names`. Without it, the requests that depend on names are left out and counted, and a name is only compared where the in-memory log shows one. A client without a name has to show null in memory, not an empty name. `test/query_log_diff.sh` runs a given `pihole-FTL` on ports of its own. It starts FTL without the caller's `FTLCONF_` variables and with `misc.readOnly`, so the test's settings stay out of `pihole.toml`, and with DHCP and `/etc/dnsmasq.d` off. It refuses to run next to a `pihole.toml` that is neither its own nor the suite's, and outside the suite also next to the `setupVars.conf`, `pihole-FTL.conf` or `gravity.db` of an installation: one of v5 would not be migrated once `pihole.toml` exists. It waits for each FTL it stops to exit, and stops the one it started last on any exit, also on Ctrl-C and while that FTL is still starting. The two files also run standalone against any build in a fresh container. `test_final.bats` runs them without `--strict-names` at the end of the suite, in about 45 s, 60 s with the suite's debug flags, and skips them on riscv64 like the API tests. Signed-off-by: 010011110 <duckenheim@posteo.de>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this implement/fix?
step 3a of the plan in #3275, after #3294: a query keeps the client name its client had when the query was made, in the query log from shared memory and in its row on disk alike, also after a restart
shared memory holds one name per client, so the main process keeps a small history next to it. when the resolver renames a client that has queries, rename_client() adds an entry: the queries of that client below the next query ID had the old name. client_name_at() finds the name of a query with a binary search over the entries, sorted by client and ID, and gives the current name when no entry is newer than the query. the engine of #3294 takes that name for the output, the filter by client name, the sort, the client part of the column search and the exclusion regexes, its string blocks copy it like any other pool string, and a filter by client IP alone does not look it up, unless webserver.api.excludeClients is set, whose regexes need the name of every row. queries_to_database() stores the same name, so a query made before a rename gets the client_by_id row of the old name, also when it is stored again after the rename
the import reads the stored name with every row and turns each run of rows with the same name into an entry, so imported queries show the names they were stored with. a new client's first queries, made before the resolver found its name, keep no name, on disk too. for an IPv4 client that is about a second of queries, an IPv6 client is resolved only after at least 2 x database.DBinterval and then at the next pass for new clients or the hourly refresh, so up to an hour of its queries stay without a name, in memory and on disk. with #3294 alone the query log in memory showed them with the name once it was known. the GC drops the entries older than the oldest query in memory. an entry takes 16 bytes and there are at most 65536 of them (1 MiB)
only the main process uses the history and always under the SHM lock, so it lives in its heap and the shared memory layout stays as it is, no SHARED_MEMORY_VERSION change here
without comments and blank lines the code adds 118 lines and removes 21, 77 of them are the new src/name-history.c
measured as in #3294, FTL's own time (took), median of 10 warm runs, three runs each, on a busier machine than in #3294. 100k queries of synthetic data as in #3294, 24 clients named through the resolver as on a running Pi-hole, #3294 -> this branch: first page sorted by client name 2.4-2.6 -> 5.2-5.6 ms, filter by client name 2.2-2.3 -> 4.6-4.8 ms, the client column search 2.3-2.4 -> 4.7-4.9 ms, with 277 renames in the window 2.4-2.5 -> 5.6-5.7, 2.2-2.4 -> 5.1-5.4 and 2.4-2.5 -> 5.2-5.6 ms. a filter by client IP 2.3-2.6 -> 1.9-2.0 ms, the default page 0.1 ms on both. the DNS path does not change, a rename is an insert into the array in the resolver thread, and on #3294 before your review fixes the p99 of 1000 DNS queries/s while 8 clients paged through the client name sort at 2M stayed under 1.4 ms with and without this
three things for you to decide:
a NULL name in client_by_id, which FTL does not write, counts as no name in memory, so client_name=* matches such rows in memory but not on disk
#3054 clears the names of all clients in flush_network_table(), merged with this that loop should call rename_client() rather than set namepos, so the queries before the flush keep their names
How to test the change during review
rename a client between two of its queries, e.g. with a dns.hosts entry for its address and SIGRTMIN+4, and compare its queries in /api/queries with and without disk=true, before and after a restart. the new test in test_final.bats does that
Additional information
Related issue or feature (if applicable): #3275
Pull request in docs with documentation (if applicable): N/A
By submitting this pull request, I confirm the following:
git rebase)src/dnsmasq/. That tree is a verbatim copy of upstream dnsmasq and we do not carry anything in it that deviates from upstream. Fixes have to go through thednsmasq-discussmailing list first, we merge them once they are in dnsmasq master.Checklist:
developmentbranch.