Skip to content

auth lmdb: correctly update timestamps in non-split domain table mode#17516

Open
miodvallat wants to merge 3 commits into
PowerDNS:masterfrom
miodvallat:brainless
Open

auth lmdb: correctly update timestamps in non-split domain table mode#17516
miodvallat wants to merge 3 commits into
PowerDNS:masterfrom
miodvallat:brainless

Conversation

@miodvallat
Copy link
Copy Markdown
Contributor

Short description

This is an embarassing bug which is tarnishing the 5.1 release. As noticed by @mind04, an lmdb database configured with the defaults (write notification update timestamps immediately, and domain table not split) will fail to actually write these timestamps.

Possible workaround are to enable split domain table (but will not be interoperable with older versions lacking the feature), or disable writes and perform regular flushes with pdns_control.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • read and accepted the Developer Certificate of Origin document, including the AI Policy, and added a "Signed-off-by" to my commits
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

Signed-off-by: Miod Vallat <miod.vallat@powerdns.com>
Copy link
Copy Markdown
Contributor

@pieterlexis pieterlexis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A (unit?)test to prevent regressions would be good

@coveralls
Copy link
Copy Markdown

coveralls commented Jun 4, 2026

Coverage Report for CI Build 26952964725

Warning

Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes.
Quick fix: rebase this PR. Learn more →

Coverage increased (+0.03%) to 71.115%

Details

  • Coverage increased (+0.03%) from the base build.
  • Patch coverage: 5 uncovered changes across 1 file (11 of 16 lines covered, 68.75%).
  • 107 coverage regressions across 14 files.

Uncovered Changes

File Changed Covered %
modules/lmdbbackend/lmdbbackend.cc 16 11 68.75%

Coverage Regressions

107 previously-covered lines in 14 files lost coverage.

Top 10 Files by Coverage Loss Lines Losing Coverage Coverage
pdns/dnsdistdist/dnsdist-lua-inspection.cc 45 41.83%
pdns/recursordist/test-syncres_cc1.cc 13 79.9%
pdns/dnsdistdist/dnsdist-console-completion.cc 10 42.73%
pdns/recursordist/syncres.cc 8 81.52%
pdns/rcpgenerator.cc 5 89.14%
pdns/auth-secondarycommunicator.cc 4 57.94%
pdns/iputils.hh 4 76.03%
pdns/packethandler.cc 4 69.12%
modules/gpgsqlbackend/spgsql.cc 3 63.78%
pdns/iputils.cc 3 60.33%

Coverage Stats

Coverage Status
Relevant Lines: 170718
Covered Lines: 133006
Line Coverage: 77.91%
Relevant Branches: 81542
Covered Branches: 46388
Branch Coverage: 56.89%
Branches in Coverage %: Yes
Coverage Strength: 7254114.69 hits per line

💛 - Coveralls

@miodvallat
Copy link
Copy Markdown
Contributor Author

A (unit?)test to prevent regressions would be good

Indeed. The hardest part was finding where to put it; hopefully this won't break CI.

@miodvallat miodvallat force-pushed the brainless branch 3 times, most recently from 760eb69 to 2e75702 Compare June 4, 2026 10:10
@miodvallat
Copy link
Copy Markdown
Contributor Author

Can't get the CI to pass ATM, removing the test, will do it in a separate PR

Signed-off-by: Miod Vallat <miod.vallat@powerdns.com>
Signed-off-by: Miod Vallat <miod.vallat@powerdns.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants