[TECH DEBT] pgjsonb returner schema and index revamp — RFC for upstream discussion

Description of the tech debt to be addressed, include links and screenshots

This is an RFC, not a code-bearing PR. The pgjsonb returner schema and indexes — documented in the module docstring as the canonical setup — have several issues that surface on busy production clusters. Some are slow leaks, some are write-throughput tax that stays invisible until the cluster is large enough, one is a silent data-quality bug. None can be fixed without either a schema migration or a write-side detection layer, which is why this is a discussion before any individual PR.

The intent of this issue is to gather opinions from upstream maintainers on:

• Which of the five items below are worth landing in 3006.x (LTS) versus deferring to 3009.
• The preferred backwards-compatibility strategy when the documented schema changes.
• Whether any operators rely on indexes that would be dropped from the default schema.
• The right granularity for follow-up PRs — one per item or grouped.

Recently merged work in salt/returners/pgjsonb.py exposes several of these problems indirectly: #69061 (orphan returns), #69065 (get_fun ordering), and others in the same series. Each works around a missing schema feature; landing the schema changes here would let the workarounds be replaced with simpler code.

---

1. jids table has no creation timestamp — orphan jids accumulate forever

The jids schema is (jid varchar PK, load jsonb). There is no timestamp column.

_purge_jobs cannot directly identify old jids, so it filters via salt_returns:

sql = (
    "delete from jids where jid in (select distinct jid from salt_returns "
    "where alter_time < %s)"
)

A jid with no rows in salt_returns — the job was published against an offline target, all minions failed before replying, or the publish reached gather_job_timeout with zero responses — is never matched by this subquery and the row in jids lives forever.

On a 1000-minion cluster running 100 jobs/min with a 5% no-response rate, that is roughly 50 orphan rows per day, ~18k per year. The jids table grows linearly without an upper bound. Operators have no built-in way to clean it up; the recipe is a manual DELETE FROM jids WHERE jid NOT IN (SELECT DISTINCT jid FROM salt_returns), which itself is a full sequential scan of two large tables.

Proposal: add a creation timestamp.

ALTER TABLE jids ADD COLUMN created_at TIMESTAMP WITH TIME ZONE DEFAULT NOW();
CREATE INDEX idx_jids_created_at ON jids (created_at);

save_load does not need code changes (DEFAULT NOW() populates the column). _purge_jobs adds one extra DELETE behind a feature detection guard:

if _schema_has_jids_created_at(cursor):
    cursor.execute("DELETE FROM jids WHERE created_at < %s", (timestamp,))

2. No index on salt_returns.alter_time

The documented schema has B-tree indexes on id, jid, fun and GIN indexes on return and full_ret, but nothing on alter_time. Every reference to alter_time in the maintenance loop is a sequential scan:

• _purge_jobs — three DELETEs (jids via subquery, salt_returns, salt_events).
• _archive_jobs — three INSERTs into archive tables.
• _purge_jobs after #69061 lands — antijoin WHERE r.alter_time >= %s for the orphan-returns fix.
• get_fun after #69065 lands — ORDER BY alter_time DESC.

On a 1000-minion cluster doing 100 jobs/min the table reaches 6M rows/hour. Each maintenance tick (loop_interval defaults to 60 s) does multiple full scans over a 50M-row table — tens of seconds, sometimes overlapping the next tick.

Proposal:

CREATE INDEX IF NOT EXISTS idx_salt_returns_alter_time ON salt_returns (alter_time);

A B-tree on a monotonically increasing timestamp has very cheap insert maintenance — new rows append to the right end of the tree. The existing idx_salt_returns_jid is still needed for jid lookups.

3. No index on salt_events.alter_time

Same situation as salt_returns, with a higher write rate (presence events, schedule ticks, auth events, every event from every minion). The maintenance loop hits this table for purge and archive identically.

Proposal:

CREATE INDEX IF NOT EXISTS idx_salt_events_alter_time ON salt_events (alter_time);

4. GIN indexes on JSONB columns nobody queries

The schema creates four GIN indexes:

CREATE INDEX idx_jids_jsonb       ON jids          USING gin (load)     WITH (fastupdate=on);
CREATE INDEX idx_salt_returns_return    ON salt_returns USING gin (return)   WITH (fastupdate=on);
CREATE INDEX idx_salt_returns_full_ret  ON salt_returns USING gin (full_ret) WITH (fastupdate=on);
CREATE INDEX idx_salt_events_data       ON salt_events  USING gin (data)     WITH (fastupdate=on);

GIN indexes on JSONB tokenize the value on every INSERT and update an inverted index per key/value/path. With fastupdate=on part of the work is deferred but eventually still runs. Concrete write overhead is workload-dependent but typically 2–5× the cost of a B-tree on the same INSERT for non-trivial JSONB values.

These indexes only matter if someone runs ad-hoc WHERE return @> '{...}' queries against the tables. The Salt code itself does not — every pgjsonb.get_* function looks up by id, jid, or fun, all already covered by B-trees. salt-run jobs.* does not query JSONB content. The GIN indexes are only valuable to the small subset of operators who write their own SQL.

Proposal: drop them from the default documented schema; document them as optional ad-hoc indexes for operators who actually query JSONB content.

-- Optional indexes for operators who run ad-hoc JSONB queries.
-- Not created by default because they impose significant write overhead.
-- CREATE INDEX idx_jids_load ON jids USING gin (load);
-- CREATE INDEX idx_salt_returns_return ON salt_returns USING gin (return);
-- CREATE INDEX idx_salt_returns_full_ret ON salt_returns USING gin (full_ret);
-- CREATE INDEX idx_salt_events_data ON salt_events USING gin (data);

Existing deployments are not auto-migrated. The release notes can note this as a manual perf opportunity for clusters that don't query JSONB.

5. salt_returns.success is varchar(10) instead of boolean

success varchar(10) NOT NULL,

The Python code passes a bool:

ret.get("success", False),

psycopg2 sends Python True / False, Postgres stores 'True' / 'False' strings — 9–10 bytes per row instead of 1. On a 50M-row table that is 400+ MB of disk and shared-buffer cache wasted. The column is wrong by type; ad-hoc queries have to write WHERE success = 'True' or cast.

Proposal:

ALTER TABLE salt_returns ALTER COLUMN success TYPE BOOLEAN USING success::BOOLEAN;

Postgres coerces the existing values automatically. psycopg2 already passes Python booleans, so the writer code does not need to change. Operators who wrote ad-hoc WHERE success = 'True' queries need to switch to WHERE success.

This is the lowest-priority item. It is mostly long-tail hygiene, not a hot-path issue.

---

Backwards-compatibility strategy

Salt should not run DDL against the operator's database. All migrations are operator-driven via the release notes, and the code must work against both old and new schemas during the upgrade window.

The cleanest pattern is a one-shot capability detection cached at module level:

from functools import lru_cache

@lru_cache(maxsize=1)
def _schema_capabilities():
    caps = set()
    with _get_serv() as cur:
        cur.execute(
            "SELECT table_name, column_name "
            "FROM information_schema.columns "
            "WHERE table_schema = current_schema() "
            "AND table_name IN ('jids', 'salt_returns', 'salt_events')"
        )
        for table, column in cur.fetchall():
            if (table, column) == ("jids", "created_at"):
                caps.add("jids.created_at")
            # etc.
    return frozenset(caps)

Used at every site that needs to vary behaviour:

if "jids.created_at" in _schema_capabilities():
    cursor.execute("DELETE FROM jids WHERE created_at < %s", (timestamp,))
else:
    log.warning(
        "pgjsonb: jids.created_at column missing; orphan jid rows "
        "without salt_returns will accumulate. See release notes for migration."
    )

The cache invalidates on process restart, which is the natural boundary after an operator runs the migration. There is no need for a version table or migration tooling — Salt just observes the schema and adapts.

---

Tiered recommendation

• Tier 1 — universal recommendation: indexes on salt_returns.alter_time and salt_events.alter_time. Cheap to add, cheap to maintain, immediate win on every cluster of non-trivial size, no code changes required.
• Tier 2 — recommended for large clusters: jids.created_at (closes the orphan-jid leak) and drop the GIN indexes from the default schema (write throughput).
• Tier 3 — long-tail hygiene: success → boolean.

If the appetite is to land none, two, or all five, the changes are independent enough to ship separately.

---

Questions for maintainers

1. Is there appetite for schema changes in 3006.x (LTS), or does this need to wait for 3008-3009?
2. Do any of you know of deployments that actively use the GIN indexes for ad-hoc queries? That would change the calculus on item 4.
3. Preferred shape for the detection layer — the lru_cache pattern above, a version table similar to django migrations, or something else already in Salt's playbook?
4. Should the migration SQL live in the module docstring (where the schema is currently documented) or as a separate pkg/postgres/migrations/<rev>.sql file?
5. Granularity — one PR per item, or grouped per tier?

Affected branches

3006.x, 3007.x, 3008.x, master — all four carry the same schema and the same documented setup.

Read salt/returners/pgjsonb.py and its module docstring, then review the five proposed schema changes and the related context in #69061 and #69065. The RFC is done when maintainers decide which changes, compatibility strategy, migration location, and follow-up PR granularity to pursue.