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

sashiko-bot
2026-04-25T18:12:44Z

Sashiko AI review found 7 potential issue(s):
- [High] Critical branch target metrics hardcoded to zero and branch_types table empty.
- [High] Missing Ported Features causing data loss.
- [High] ctypes Type Error causes immediate runtime crash when writing to DB.
- [High] KeyboardInterrupt permanently leaks massive intermediate directories.
- [High] DSOs are incorrectly cached using only short_name.
- [Medium] setup_db() exceptions permanently leak the intermediate directory.
- [Medium] Script exits with code 0 on failure, breaking automated tools.
--

commit 530a5874e47c51699eb3a71fb80415e9c17f6d7e
Author: Ian Rogers <irogers@google.com>

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

This commit ports the export-to-postgresql.py script to use the modern perf
Python module API instead of PySide2 and QtSql. It relies on libpq via ctypes
and exports the perf data to a local PostgreSQL database.

> 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

[ ... ]

> + symbols
> + 'symbols' contains a record for each symbol. Only symbols that
> + have samples are present.
> +
> + threads
> + 'threads' contains a record for each thread.
> +
> +Views:

The original QtSql-based script exported several specialized tables like
calls, context_switches, ptwrite, mwait, cbr, pwre, exstop, and pwrx.

Will omitting the parsing and export of these tables break downstream
scripts like exported-sql-viewer.py that might rely on them?

[ ... ]

> + 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))
> + if PQresultStatus(res) != 4: # PGRES_COPY_IN
> + PQclear(res)
> + raise RuntimeError(f"COPY FROM STDIN PQexec failed for {table_name}")
> + PQclear(res)
> +
> + with open(path_name, "rb") as f:
> + data = f.read(65536)
> + while len(data) > 0:
> + c_data = (c_ubyte * len(data)).from_buffer_copy(data)
> + ret = PQputCopyData(self.conn, c_data, len(data))

PQputCopyData expects a c_void_p for the second argument. Is passing a
ctypes Array instance directly here going to raise a ctypes.ArgumentError?

Would we need to cast it or use ctypes.byref(c_data)?

[ ... ]

> + 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,

Branch-related data such as to_dso_id, to_symbol_id, to_sym_offset, to_ip,
and branch_type appear to be hardcoded to 0 here instead of being extracted
from the sample event.

Are we silently dropping the branch stack data here?

Also, no data is written to branch_type_table.bin, leaving the branch_types
table completely empty.

[ ... ]

> + def get_dso_id(self, short_name: str, long_name: str,
> + build_id: str) -> int:
> + """Get or create DSO ID."""
> + if short_name in self.caches['dsos']:
> + return self.caches['dsos'][short_name]

Is it safe to cache DSOs using only their short_name? This might conflate
distinct libraries that share the same basename (like libc.so.6 from a host
vs. container).

Could this merge distinct executables into the same dso_id?

[ ... ]

> +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()

If exporter.setup_db() fails here (e.g. database connection error), the
script exits immediately.

Are we leaving the intermediate directory created by setup_db() orphaned
on the filesystem since the cleanup block below is never reached?

> +
> + session = None
> + error_occurred = False
> + try:
> + session = perf.session(perf.data(args.input),
> + sample=exporter.process_event)
> + exporter.session = session
> + session.process_events()
> + exporter.finalize()
> + print(f"Successfully exported to {args.output}")
> + except Exception as e:

Will this except clause miss catching KeyboardInterrupt (which inherits from
BaseException)?

If the user aborts with Ctrl+C, error_occurred would remain False, which
might prevent shutil.rmtree() from running and permanently leak the
intermediate directory.

Also, if an error is caught here, the script still exits with a status of 0.

Should the script return a non-zero exit code on failure to prevent breaking
automated tools or CI pipelines that rely on exit codes?

> + print(f"Error processing events: {e}")
> + error_occurred = True
> + finally:
> + exporter.disconnect()
> + if error_occurred:
> + if os.path.exists(exporter.output_dir_name):
> + shutil.rmtree(exporter.output_dir_name)

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