Re: [PATCH v7 37/59] perf export-to-postgresql: Port export-to-postgresql to use python module

sashiko-bot
2026-04-25T23:13:24Z

Sashiko AI review found 9 potential issue(s):
- [High] The script silently exports 0 for many critical sample fields due to incorrect attribute names and ignoring branch stacks.
- [High] The call path tree constructed for PostgreSQL is structurally inverted, placing leaf functions at the root.
- [High] The script silently ignores the final result status of the PostgreSQL COPY FROM STDIN command, leading to silent data loss.
- [High] Using the 'q' (signed 64-bit integer) format character in struct.pack for 64-bit memory addresses crashes the exporter on kernel samples.
- [High] The script completely drops the logic for exporting synthetic Intel PT events and context switches, despite advertising Intel PT support.
- [Medium] String values are packed dynamically without length validation, leading to brittle behavior and database import failures.
- [Medium] The ported script drops all database foreign key constraints that were present in the original implementation.
- [Medium] The `comm_threads_view` view is documented in the file header but is never created.
- [Low] Exceptions in setup_db() bypass cleanup logic, leaving temporary directories on disk.
--

commit 6da42dc06630d58ef6cdd8bf94f7bec7384e8f74
Author: Ian Rogers <irogers@google.com>

perf export-to-postgresql: Port export-to-postgresql to use python module

Ported from tools/perf/scripts/python/export-to-postgresql.py to use
the perf Python module API.

> diff --git a/tools/perf/python/export-to-postgresql.py b/tools/perf/python/export-to-postgresql.py
> new file mode 100755
> index 000000000000..0118dc348b1e
> --- /dev/null
> +++ b/tools/perf/python/export-to-postgresql.py
> @@ -0,0 +1,701 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0
> +r"""
> +Export perf data to a postgresql database.

[ ... ]

> +An example of using this script with Intel PT:
> +
> + $ perf record -e intel_pt//u ls
> + $ python tools/perf/python/export-to-postgresql.py -i perf.data -o pt_example

Does this script still support exporting synthetic Intel PT events and context
switches? The docstring advertises an example with Intel PT, but the logic
and table schemas for ptwrite, cbr, mwait, pwre, exstop, pwrx, and
context_switches appear to be missing.

[ ... ]

> +Views:
> +
> + Most of the tables have views for more friendly display. The views are:
> +
> + comm_threads_view

This isn't a bug, but comm_threads_view is listed in the views documentation
here, but doesn't seem to be created in finalize().

[ ... ]

> + def copy_output_file(self, path_name: str, table_name: str):
> + """Copy intermediate file to database."""
> + sql = f"COPY {table_name} FROM STDIN (FORMAT 'binary')"
> + res = PQexec(self.conn, toserverstr(sql))

[ ... ]

> + res = PQgetResult(self.conn)
> + while res:
> + PQclear(res)
> + res = PQgetResult(self.conn)

Does this loop silently ignore the final result status of the PostgreSQL COPY
command?

If PostgreSQL rejects the data due to a constraint violation, PQgetResult()
yields PGRES_FATAL_ERROR, but this loop clears the result without inspecting
PQresultStatus(), potentially masking silent data loss.

[ ... ]

> + def write_comm(self, comm_id: int, comm_str: str, thread_id: int,
> + time: int, exec_flag: int) -> None:
> + """Write comm to binary file."""
> + comm_bytes = toserverstr(comm_str)
> + n = len(comm_bytes)
> + fmt = "!hiqi" + str(n) + "s" + "iqiqiB"
> + value = struct.pack(fmt, 5, 8, comm_id, n, comm_bytes, 8,
> + thread_id, 8, time, 1, exec_flag)
> + self.files['comm'].write(value)

Are the dynamically packed strings validated against the database schema
limits?

The comms table defines comm as varchar(16), but unbounded strings are packed
here. If a string exceeds the limit, will PostgreSQL abort the COPY FROM STDIN
command?

[ ... ]

> + def write_symbol(self, symbol_id: int, dso_id: int, sym_start: int,
> + sym_end: int, binding: int, symbol_name: str) -> None:
> + """Write symbol to binary file."""
> + name_bytes = toserverstr(symbol_name)
> + n = len(name_bytes)
> + fmt = "!hiqiqiqiqiii" + str(n) + "s"
> + value = struct.pack(fmt, 6, 8, symbol_id, 8, dso_id, 8,
> + sym_start, 8, sym_end, 4, binding, n, name_bytes)
> + self.files['symbol'].write(value)

Will using the 'q' (signed 64-bit integer) format character in struct.pack
crash the exporter when processing kernel-space instruction pointers?

Since kernel addresses can exceed the maximum positive value of a signed
64-bit integer, should the 'Q' (unsigned 64-bit) specifier be used instead?

[ ... ]

> + def write_sample(self, sample_id: int, evsel_id: int, thread_id: int,
> + comm_id: int, dso_id: int, symbol_id: int,
> + sample: perf.sample_event, call_path_id: int) -> None:
> + """Write sample to binary file."""
> + value = struct.pack(
> + "!hiqiqiqiqiqiqiqiqiqiqiiiqiqiqiqiqiqiqiqiiiBiqiqiqii",
> + 25, 8, sample_id, 8, evsel_id, 8, 0, 8, thread_id, 8, comm_id,
> + 8, dso_id, 8, symbol_id, 8, getattr(sample, 'sym_offset', 0),
> + 8, sample.sample_ip, 8, sample.sample_time, 4, sample.sample_cpu,
> + 8, 0, 8, 0, 8, 0, 8, 0,
> + 8, getattr(sample, 'sample_period', 0) or 0,
> + 8, getattr(sample, 'sample_weight', 0) or 0,
> + 8, getattr(sample, 'transaction_', 0) or 0,
> + 8, getattr(sample, 'data_src', 0) or 0,
> + 4, 0,
> + 1, getattr(sample, 'in_tx', 0) or 0,
> + 8, call_path_id,
> + 8, getattr(sample, 'insn_count', 0) or 0,
> + 8, getattr(sample, 'cyc_count', 0) or 0,
> + 4, getattr(sample, 'flags', 0) or 0
> + )
> + self.files['sample'].write(value)

Are the attribute names for the sample fields correct, and are branch targets
being handled?

The fields data_src, insn_count, and cyc_count differ from the actual
sample_data_src, sample_insn_count, and sample_cyc_count names. Additionally,
the branch target fields (to_ip, to_dso_id, to_symbol_id, to_sym_offset) are
hardcoded to 0 instead of being extracted from sample.brstack.

Will this result in missing sample fields being silently exported as zeroes?

[ ... ]

> + def process_event(self, sample: perf.sample_event) -> None:
> + """Callback for processing events."""

[ ... ]

> + call_path_id = 0
> + if hasattr(sample, 'callchain') and sample.callchain:
> + parent_id = 0
> + for node in sample.callchain:
> + node_dso = getattr(node, 'dso', None) or getattr(node, 'map', None)
> + node_symbol = getattr(node, 'symbol', None) or getattr(node, 'sym', None)
> +
> + dso_name = "Unknown_dso"
> + if node_dso:
> + dso_name = getattr(node_dso, 'name', "Unknown_dso") or "Unknown_dso"
> +
> + symbol_name = "Unknown_symbol"
> + if node_symbol:
> + symbol_name = getattr(node_symbol, 'name', "Unknown_symbol") or "Unknown_symbol"
> +
> + node_dso_id = self.get_dso_id(dso_name, dso_name, "")
> + node_symbol_id = self.get_symbol_id(node_dso_id, symbol_name, 0, 0)
> +
> + parent_id = self.get_call_path_id(parent_id, node_symbol_id, node.ip)
> + call_path_id = parent_id

Is the call path tree constructed upside-down here?

By iterating sequentially over sample.callchain (from index 0 upwards) and
using the newly generated ID as the parent_id for the next frame, the caller
becomes a child of the callee. Should this be iterating in reverse to
properly build parent-child aggregation?

[ ... ]

> + def finalize(self) -> None:
> + """Copy files to database and add keys/views."""

[ ... ]

> + print("Adding primary keys")
> + self.do_query("ALTER TABLE selected_events ADD PRIMARY KEY (id)")
> + self.do_query("ALTER TABLE machines ADD PRIMARY KEY (id)")
> + self.do_query("ALTER TABLE threads ADD PRIMARY KEY (id)")
> + self.do_query("ALTER TABLE comms ADD PRIMARY KEY (id)")
> + self.do_query("ALTER TABLE comm_threads ADD PRIMARY KEY (id)")
> + self.do_query("ALTER TABLE dsos ADD PRIMARY KEY (id)")
> + self.do_query("ALTER TABLE symbols ADD PRIMARY KEY (id)")
> + self.do_query("ALTER TABLE branch_types ADD PRIMARY KEY (id)")
> + self.do_query("ALTER TABLE samples ADD PRIMARY KEY (id)")
> + self.do_query("ALTER TABLE call_paths ADD PRIMARY KEY (id)")

This isn't a bug, but were the foreign key constraints from the original
implementation intentionally dropped?

The previous script enforced relational integrity by adding foreign keys at
the end of the export, which seem to be missing here.

[ ... ]

> +if __name__ == "__main__":
> + ap = argparse.ArgumentParser(
> + description="Export perf data to a postgresql database")
> + ap.add_argument("-i", "--input", default="perf.data",
> + help="Input file name")
> + ap.add_argument("-o", "--output", required=True,
> + help="Output database name")
> + args = ap.parse_args()
> +
> + exporter = PostgresExporter(args.output)
> + exporter.setup_db()
> +
> + session = None
> + error_occurred = False
> + try:

Could exceptions in setup_db() bypass the cleanup logic?

If setup_db() fails, the script will terminate immediately without executing
the finally block, which might leave orphaned temporary directories on the
filesystem.

--
Sashiko AI review · https://sashiko.dev/#/patchset/20260425224951.174663-1-irogers@google.com?part=37