Skip to content

adapter: make statement arrival logging check local#37629

Closed
def- wants to merge 1 commit into
MaterializeInc:mainfrom
def-:pr-statement-log2
Closed

adapter: make statement arrival logging check local#37629
def- wants to merge 1 commit into
MaterializeInc:mainfrom
def-:pr-statement-log2

Conversation

@def-

@def- def- commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Keep the switch synchronized with the system variable through a callback. This avoids an async catalog snapshot for every pgwire message and HTTP request when emergency logging is disabled.

Follow-up to #37567

This seems to have caused a perf. regression, seen in https://buildkite.com/materialize/nightly/builds/17267:

QUERY                                    | STAT    |     THIS     |    OTHER     |  CHANGE   | THRESHOLD | REGRESSION?
----------------------------------------------------------------------------------------------------------------------
SELECT 1 (standalone)                    | avg     |        13.61 |        11.26 |    20.84% |       20% |   !!YES!!
SELECT 1 (standalone)                    | max     |        54.85 |        74.97 |   -26.84% |           |
SELECT 1 (standalone)                    | min     |         5.23 |         3.54 |    47.71% |           |
SELECT 1 (standalone)                    | p50     |        13.40 |        10.94 |    22.43% |       20% |   !!YES!!
SELECT 1 (standalone)                    | p95     |        18.72 |        16.39 |    14.20% |       30% |      no
SELECT 1 (standalone)                    | p99     |        22.44 |        19.86 |    12.99% |           |
SELECT 1 (standalone)                    | qps     |       734.60 |       887.70 |   -17.25% |       20% |   !!YES!!

Performance nightly run: https://buildkite.com/materialize/nightly/builds/17310

@def- def- requested a review from ggevay July 14, 2026 10:05
@def- def- requested a review from a team as a code owner July 14, 2026 10:05
Keep the switch synchronized with the system variable through a callback.
This avoids an async catalog snapshot for every pgwire message and HTTP
request when logging is disabled.

Follow-up to MaterializeInc#37567

This seems to have caused a perf. regression, seen in https://buildkite.com/materialize/nightly/builds/17267:

QUERY                                    | STAT    |     THIS     |    OTHER     |  CHANGE   | THRESHOLD | REGRESSION?
----------------------------------------------------------------------------------------------------------------------
SELECT 1 (standalone)                    | avg     |        13.61 |        11.26 |    20.84% |       20% |   !!YES!!
SELECT 1 (standalone)                    | max     |        54.85 |        74.97 |   -26.84% |           |
SELECT 1 (standalone)                    | min     |         5.23 |         3.54 |    47.71% |           |
SELECT 1 (standalone)                    | p50     |        13.40 |        10.94 |    22.43% |       20% |   !!YES!!
SELECT 1 (standalone)                    | p95     |        18.72 |        16.39 |    14.20% |       30% |      no
SELECT 1 (standalone)                    | p99     |        22.44 |        19.86 |    12.99% |           |
SELECT 1 (standalone)                    | qps     |       734.60 |       887.70 |   -17.25% |       20% |   !!YES!!
@def- def- force-pushed the pr-statement-log2 branch from 3fb746b to e40cf14 Compare July 14, 2026 11:41
@ggevay

ggevay commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Looking at this hard now, because if this is true, then we'd have to consider a new RC, since #37567 was just included in an RC yesterday evening.

However, my current theory is that only a very small part of the regression was caused by #37567. This is because if #37567 were the cause, then the benchmark runs on this PR should be reversing the effect, i.e., we should be seeing a 15-20% speedup (it's comparing to main), but actually we are only seeing ~2%. (I kicked off several retries to be statistically significant.)

Discussed also with @def-. Another somewhat plausible candidate is 6c590b0, which is being reverted here #37631. (Note that every connection establishment involves a table write in mz_sessions.) I'm also trying to verify this independently by running the benchmark on the commits just before and just after the PR (and comparing their absolute numbers between these two runs):

Edit: Confirmed in the above before/after runs.

@def- def- closed this Jul 14, 2026
@def-

def- commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

This was not the root cause of the issue, instead #37631 fixes the SELECT 1 slowdown.

@ggevay

ggevay commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

(As discussed on slack, an additional problem with the PR is that Claude found multiple pre-existing glitches in the register_callback mechanism (which is relied upon by the PR). So, the risk of basing the statement arrival logging on register_callback is, I'd say, not worth the ~2% perf impact. And note that for real, non-constant queries, it would be more like 1%. And even this seems to happen only in benchmarks that establish a connection per query.)

(I'll open issues for the register_callback glitches.)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants