Skip to content

Pick latest pgjsonb get_fun return by alter_time, not by MAX(jid)#69065

Merged
dwoz merged 2 commits into
saltstack:3006.xfrom
co-cy:pgjsonb-get-fun-order-by-alter-time
Jul 10, 2026
Merged

Pick latest pgjsonb get_fun return by alter_time, not by MAX(jid)#69065
dwoz merged 2 commits into
saltstack:3006.xfrom
co-cy:pgjsonb-get-fun-order-by-alter-time

Conversation

@co-cy

@co-cy co-cy commented May 6, 2026

Copy link
Copy Markdown
Contributor

Depends on #69063. This branch is layered on top of that PR; the diff currently shows both commits and will rebase to a single commit once #69063 is merged.

What does this PR do?

salt/returners/pgjsonb.py:get_fun chose the latest return per minion with MAX(jid). That is the lexicographic maximum of the jid column, not the time-latest return. It happens to coincide for Salt's default jid format (YYYYMMDDHHMMSSffffff) and the nano variant — both are timestamp-formatted strings of equal length, so lex order matches chronological. It silently breaks for any deployment where that assumption does not hold:

  • master_job_cache.gen_jid is overridden with a custom scheme — UUIDs, snowflake ids, hash-based identifiers — none lexicographically sortable as timestamps.
  • External publishers (salt-api, salt-ssh, orchestrate runners) call prep_jid(passed_jid="...") with a custom jid string.
  • A master spans a jid_format config change and ends up with rows of different formats in the same salt_returns.

In all those cases MAX(jid) returns a row that is not the time-latest, and get_fun reports the wrong full_ret per minion. No exception, no warning — just a silently wrong answer.

This PR replaces the lex-max algorithm with ORDER BY alter_time DESC and DISTINCT ON (id). alter_time is populated by Postgres via DEFAULT NOW(), so "latest" reflects actual insertion order regardless of jid format.

What issues does this PR fix or reference?

Fixes #69064

Previous Behavior

sql = """SELECT s.id, s.jid, s.full_ret
         FROM salt_returns s
         JOIN (SELECT MAX(jid) AS jid
               FROM salt_returns GROUP BY fun, id) max
         ON s.jid = max.jid
         WHERE s.fun = %s
         """

For two returns of the same fun on the same minion with custom jids "abc-001" and "000-002" (chronologically the second is newer), MAX(jid) picks "abc-001" — wrong.

New Behavior

sql = """SELECT DISTINCT ON (id)
                id, jid, full_ret
         FROM salt_returns
         WHERE fun = %s
         ORDER BY id, alter_time DESC
         """

alter_time DESC orders by Postgres-managed insertion timestamp; DISTINCT ON (id) keeps the first row per minion. The chronologically newest return wins regardless of jid format.

Performance

There is no index on salt_returns.alter_time in the documented schema, so this query remains a sequential scan — same as the previous MAX(jid) GROUP BY form. Adding an alter_time index, alongside several other schema observations (jids.created_at, redundant GIN indexes, salt_returns.success typing), will be tracked as a separate follow-up RFC.

Merge requirements satisfied?

  • Docs (no user-facing change; not required)
  • Changelog — changelog/69064.fixed.md
  • Tests written/updated — see tests/pytests/unit/returners/test_pgjsonb.py:
    • test_get_fun_orders_by_alter_time_desc_not_max_jid — pins the algorithm: SQL must order by alter_time DESC and must not contain MAX(jid).

Commits signed with GPG?

No.

@twangboy twangboy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please rebase once 3006.x branch tests are green. Fix conflicts if any.

@dwoz dwoz added the test:full Run the full test suite label Jun 19, 2026
@dwoz dwoz added this to the Sulphur v3006.26 milestone Jun 19, 2026
@dwoz dwoz added test:full Run the full test suite and removed test:full Run the full test suite labels Jun 22, 2026
@dwoz dwoz force-pushed the pgjsonb-get-fun-order-by-alter-time branch from 847e786 to 467c447 Compare July 4, 2026 01:03
@dwoz dwoz force-pushed the pgjsonb-get-fun-order-by-alter-time branch from 467c447 to 8e79032 Compare July 4, 2026 20:41
co-cy added 2 commits July 5, 2026 15:07
`get_fun` issued:

    SELECT s.id, s.jid, s.full_ret
    FROM salt_returns s
    JOIN (SELECT MAX(`jid`) AS jid
          FROM salt_returns GROUP BY fun, id) max
    ON s.jid = max.jid
    WHERE s.fun = %s

The backticks around `jid` are MySQL-style identifier quoting. On
PostgreSQL -- which is the only server pgjsonb talks to -- the
parser rejects this with `syntax error at or near "`"`. The query
was copy-pasted from `salt/returners/mysql.py` where the syntax is
valid; the SQL identifier quoting was not adjusted.

The bug is dormant in typical deployments: stock `master_job_cache`
operation goes through `get_load`, `get_jid`, `get_jids`, but not
`get_fun`. It surfaces only when an operator (or an extension)
explicitly calls `pgjsonb.get_fun` -- e.g. through a custom runner
or via `master_job_cache.get_fun` from external tooling.

Drop the backticks. `jid` is not a reserved word in PostgreSQL, so
no quoting is needed. While here, normalise the indentation of the
SQL string for readability; behaviour is otherwise unchanged.

Add a behavioural test that calls `get_fun` against a mocked
cursor, asserts the per-minion mapping is built correctly, and
guards against backticks creeping back into the issued SQL through
future copy-paste from the mysql returner.

Refs: saltstack#69062
`get_fun` chose the latest return per minion with:

    SELECT s.id, s.jid, s.full_ret
    FROM salt_returns s
    JOIN (SELECT MAX(jid) AS jid
          FROM salt_returns GROUP BY fun, id) max
    ON s.jid = max.jid
    WHERE s.fun = %s

`MAX(jid)` is the lexicographic max of the `jid` column, not the
time-latest return. It happens to coincide for Salt's default
`YYYYMMDDHHMMSSffffff` format and the `nano` variant because those
are timestamp-formatted strings of equal length. It silently fails
for:

* Deployments that override `master_job_cache.gen_jid` with a custom
  scheme -- random UUIDs, snowflake ids, hash-based identifiers --
  none of which sort lexicographically as timestamps.

* `prep_jid(passed_jid="...")` from external publishers (salt-api,
  salt-ssh, runners, orchestrate) that hand the master a custom jid.

* Salt-runs that span a `jid_format` config change, leaving rows of
  different formats in the same `salt_returns` table.

In all of those cases the lexicographic max is not the time-latest
and `get_fun` returns the wrong row. The function looks like it
worked, the operator gets the wrong answer.

Replace with `alter_time` ordering, which Postgres populates from
`DEFAULT NOW()` and which therefore reflects the actual insertion
order regardless of jid format:

    SELECT DISTINCT ON (id)
           id, jid, full_ret
    FROM salt_returns
    WHERE fun = %s
    ORDER BY id, alter_time DESC

`DISTINCT ON (id)` is Postgres-specific (the file already targets
Postgres). It returns one row per `id`, picking the first per
`ORDER BY` -- here, the most recent `alter_time` per minion.

Add a behavioural test that pins the algorithm: the issued SQL
must order by `alter_time DESC`, and must not contain `MAX(jid)`.

Note on performance: there is no index on `salt_returns.alter_time`
in the documented schema, so this remains a sequential scan, same
as the previous `MAX(jid) GROUP BY` form. The lack of an
`alter_time` index is a separate, larger schema discussion that
will be tracked as a follow-up RFC.

Depends on saltstack#69063 (which removed the MySQL-style backtick quoting
from this function); this PR builds on that branch and should be
merged after it.

Refs: saltstack#69064
@dwoz dwoz merged commit 47d425e into saltstack:3006.x Jul 10, 2026
428 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:full Run the full test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants