Skip to content

Build poll_host queries once instead of once per poller type - #589

Closed
somethingwithproof wants to merge 4 commits into
Cacti:developfrom
somethingwithproof:refactor/poll-host-query-scope
Closed

Build poll_host queries once instead of once per poller type#589
somethingwithproof wants to merge 4 commits into
Cacti:developfrom
somethingwithproof:refactor/poll-host-query-scope

Conversation

@somethingwithproof

@somethingwithproof somethingwithproof commented Sep 2, 2026

Copy link
Copy Markdown
Member

Closes #587. Closes #590.

poll_host() is the largest function in the tree by a factor of eight. It built the same six queries twice, once under if (set.poller_id == 0) and once in the else, 141 and 142 lines differing by 33. None of it was reachable from a test.

The copies had already drifted. The main poller adapts the poller_output upsert to the server version; the remote copy never picked that up, so a remote poller against MySQL 8 still emits VALUES(output), deprecated in 8.0.20. That is #590, and it is the argument for the refactor rather than a separate cleanup.

Three commits

1. Collapse the two branches. Both bodies compile standalone once eight inputs are declared, so the whole construction could be lifted into a generator and its output compared before and after. Across every vector it branches on, total_snmp_ports 1 and 2, dbonupdate 0 and 1, main and remote, the 72 strings are identical except four lines where query1 on a remote poller now spells its tail poller_id = N rather than poller_id=N. The other five queries already used spaces. 286 lines become 153.

2. Fix #590. After the collapse it is the removal of one guard clause. Two lines of the capture change, both posuffix on a remote poller with dbonupdate=1.

3. Extract the construction into poll_host_build_queries(). It takes host_id, regex_col and limits as arguments, reads five documented settings, and fills a poll_host_queries_t. Nothing else moves.

What that buys, concretely

poll_host() goes from 1,927 lines to 1,620. That is a first cut, not a decomposition; the three big blocks left inside it are 697, 362 and 293 lines and each needs the same treatment.

The part that matters more is that the construction is now callable, so the capture is a test that runs rather than a file with instructions attached:

[  ERROR   ] --- ERROR: query construction diverged from the golden capture at line 3
[  FAILED  ] test_build_queries_matches_the_golden_capture

That is the suite failing against a one-character edit to the fixture. make check now covers this code.

Tests

Twelve cases against the shipped poller.o, up from none:

  • the two scope helpers, on both poller types, plus a degenerate buffer and a NULL destination
  • the main poller filters by deleted and carries no poller_id anywhere
  • a remote poller filters by owner on query1, query5, query9 and query10, and the host row is filtered by deleted on both, never by owner
  • ORDER BY snmp_port appears only for multiple ports
  • dbonupdate applies on both poller types, which is Remote pollers ignore dbonupdate and emit the deprecated VALUES() upsert form #590 pinned
  • the cached lengths match the strings the result loop appends
  • every buffer is filled
  • the golden diff above

49 of 49 pass. Clean Ubuntu 24.04 rebuild matching ci.yml carries the same four warnings as develop.

Note for review

query1 was being reused near the end of poll_host() for an unrelated UPDATE host SET polling_time. It is now q.query1, so that reuse is still there and is now more visible than before. Giving it its own buffer is a one-line change I left out of this PR.

No CHANGELOG entry here; #578 is the changelog PR for this batch.

poll_host() is 1,926 lines and builds the same six queries twice, once for the
main poller and once for a remote one. The bodies differ by 33 of 141 lines,
and every difference is the same rule: the main poller reads items that are not
deleted, a remote poller reads the items assigned to it.

Nothing in that construction was reachable from a test, so a column added to
one copy and not the other would not have been caught.

Extract the rule as poller_item_scope() and poller_owner_scope(), covered by
five cases in test_linked against the shipped poller.o: the deleted filter on
the main poller, the ownership filter on a remote one, the empty fragment the
main poller needs so callers can interpolate unconditionally, and a degenerate
buffer.

No call site changes yet. Collapsing the two branches onto these helpers is the
next step and is worth its own review, because the shipped query1 spells its
tail 'poller_id=%i' while the others spell it 'poller_id = %i', so the unified
text will normalise that.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The two branches drifted: the remote copy never picked up the dbonupdate
handling the main copy grew, and nothing could have caught that because the
construction was not reachable from a test.

Captured what both branches emit across the vectors they switch on, collapsed
them onto poller_item_scope()/poller_owner_scope(), and diffed. Output is
byte-identical except query1 on a remote poller, which now spells its tail
'poller_id = N' rather than 'poller_id=N' to match the other five queries.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
set.dbonupdate is 1 on MySQL 8, which deprecated VALUES() in ON DUPLICATE KEY
UPDATE. The main poller switched to the row-alias form; the remote branch kept
the deprecated one because it had its own copy of the suffix. Closes Cacti#590.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@somethingwithproof somethingwithproof changed the title refactor: name the poller_item scoping rule that poll_host duplicates Build poll_host queries once instead of once per poller type Sep 3, 2026
The construction was 167 lines in the middle of a 1,795-line function, so
nothing could reach it. It now takes its inputs as arguments and fills a
struct, which is what lets the golden capture become a test that runs instead
of a file with instructions attached.

poll_host is 1,620 lines, down from 1,927 on develop.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@somethingwithproof

Copy link
Copy Markdown
Member Author

Consolidated into #597, which carries this branch's commits unchanged.

Every pair of these ten conflicted on tests/unit/test_linked.c because each appended to the same registration array, so whichever merged first would have forced a rebase on the other nine. #597 is one review and one approval for the same 20 commits, still one logical change each.

Nothing here is dropped. Reopen this if you would rather review it separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant