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

sashiko-bot
2026-04-24T17:11:11Z

Sashiko AI review found 5 potential issue(s):
- [High] Missing binary COPY file trailer causes PostgreSQL to reject the data, and silent error swallowing results in an empty database.
- [High] Incorrect ID assignment generates duplicate primary keys for the `comm_threads` table, causing database constraint violations.
- [Medium] Branch target and control flow data fields are hardcoded to 0, breaking Intel PT trace analysis.
- [Medium] Exceptions during database setup bypass the cleanup handlers, leaking temporary directories and connections.
- [Low] The script uses the exact same ID sequence counter for both samples and events.
--

commit ad2eda7d63401a755d355f71742fd43ef19eb5de
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 00000000000000..d1fa87e1d2b1a2
--- /dev/null
+++ b/tools/perf/python/export-to-postgresql.py
@@ -0,0 +1,697 @@
[ ... ]
> + def copy_output_file(self, f, table_name: str):
> + """Copy intermediate file to database."""
[ ... ]
> + ret = PQputCopyEnd(self.conn, None)
> + if ret != 1:
> + raise RuntimeError(f"PQputCopyEnd failed for {table_name}")
> +
> + res = PQgetResult(self.conn)
> + while res:
> + PQclear(res)
> + res = PQgetResult(self.conn)

Does PQgetResult need to check PQresultStatus here? It looks like if a
database error occurs during the copy operation, it would be silently
ignored.

[ ... ]
> + 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,

Are the branch-related fields meant to be zeroed out here?

The commit message mentions using this script with Intel PT, but it
looks like the target branch data fields are currently hardcoded to 0.
Would this prevent proper analysis of Intel PT traces?

> + 8, getattr(sample, 'sample_period', 0) or 0,
> + 8, getattr(sample, 'sample_weight', 0) or 0,
[ ... ]
> + def get_comm_id(self, comm: str, thread_id: int) -> int:
> + """Get or create comm ID."""
[ ... ]
> + key = (comm_id, thread_id)
> + if 'comm_threads' not in self.caches:
> + self.caches['comm_threads'] = {}
> + if key not in self.caches['comm_threads']:
> + self.write_comm_thread(comm_id, comm_id, thread_id)

When calling write_comm_thread, is the first parameter intended to be
comm_id?

Looking at write_comm_thread, the first parameter is comm_thread_id. If
comm_id is passed as the primary key here, threads that share the same
command might create rows with duplicate IDs. Could this cause a constraint
violation when the primary key is added later?

> + self.caches['comm_threads'][key] = True
> +
> + return comm_id
[ ... ]
> + def process_event(self, sample: perf.sample_event) -> None:
> + """Callback for processing events."""
[ ... ]
> + sample_id = self.next_id['event']

Should this use a separate counter for sample IDs instead of
self.next_id['event']? Because get_event_id also increments
self.next_id['event'], this might unintentionally interleave the IDs
for samples and events.

> + self.write_sample(sample_id,
> + self.get_event_id(getattr(sample.evsel, 'name', str(sample.evsel))),
> + thread_id, comm_id, dso_id, symbol_id, sample,
> + call_path_id)
> + self.next_id['event'] += 1
[ ... ]
> + def finalize(self) -> None:
> + """Copy files to database and add keys/views."""
> + print("Copying to database...")
> + for name, f in self.files.items():
> + table_name = name + "s" if name != "call_path" else "call_paths"
> + if name == "evsel":
> + table_name = "selected_events"
> + self.copy_output_file(f, table_name)
> + self.close_output_file(f)

Is it intentional to call copy_output_file before close_output_file?

Looking at close_output_file, it writes the mandatory PostgreSQL binary
trailer to the file. If copy_output_file streams the data to the database
first, won't the stream lack the EOF marker and cause PostgreSQL to abort
the copy command?

> +
> + print("Removing intermediate files...")
[ ... ]
> +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:

If an exception occurs during setup_db (for example, if the database
already exists), does the script terminate immediately and bypass the
cleanup in the finally block?

It looks like this might leave orphaned temporary directories on the
disk since the try/finally block starts after setup_db.

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