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

sashiko-bot
2026-04-20T00:28:41Z

Sashiko AI review found 8 potential issue(s):
- [High] Immediate runtime crash in `write_sample` due to `struct.pack` argument mismatch.
- [High] `finalize()` attempts to read from closed files, causing an I/O error.
- [High] Missing `PQgetResult` after `PQputCopyEnd` leaves the PostgreSQL connection in a desynchronized state.
- [High] Caching command IDs solely by name drops relationships for different threads executing the same command.
- [Medium] Temporary output directories and files are permanently leaked on errors.
- [Medium] Unquoted database name in `CREATE DATABASE` query causes syntax errors.
- [Low] The `branch_types` lookup table is completely unpopulated.
- [Low] Hardcoding the specific shared library version 'libpq.so.5' reduces portability.
--

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

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

This commit ports export-to-postgresql.py to use the perf Python module API
instead of PySide2. It uses libpq via ctypes for database operations while
keeping the high-performance binary file generation approach. Lazy population
of lookup tables from sample data is also implemented.

> diff --git a/tools/perf/python/export-to-postgresql.py b/tools/perf/python/export-to-postgresql.py
> new file mode 100755
> index 0000000000000..f9de4b1ef0b19
> --- /dev/null
> +++ b/tools/perf/python/export-to-postgresql.py
> @@ -0,0 +1,666 @@
[ ... ]
> +import argparse
> +from ctypes import CDLL, c_char_p, c_int, c_void_p, c_ubyte
> +import os
> +import struct
> +import sys
> +from typing import Any, Dict, Optional
> +import perf
> +
> +# Need to access PostgreSQL C library directly to use COPY FROM STDIN
> +try:
> + libpq = CDLL("libpq.so.5")

Is hardcoding "libpq.so.5" strictly required here?

On systems where the PostgreSQL client library uses a different naming
convention (e.g., libpq.so or libpq.so.6), this might fail to load.
Could we use ctypes.util.find_library('pq') to locate the library
dynamically?

[ ... ]
> + def copy_output_file(self, f, table_name: str):
> + """Copy intermediate file to database."""
> + f.seek(0)
> + 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)
> +
> + f.seek(0)
> + 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))
> + if ret != 1:
> + raise RuntimeError(f"PQputCopyData failed for {table_name}")
> + data = f.read(65536)
> +
> + ret = PQputCopyEnd(self.conn, None)
> + if ret != 1:
> + raise RuntimeError(f"PQputCopyEnd failed for {table_name}")

After calling PQputCopyEnd(), is it necessary to call PQgetResult() until
it returns NULL?

The libpq C-API requires clearing the PGRES_COMMAND_OK status of the COPY
command. Failing to do so might leave the database connection in a busy
state, causing the next PQexec call (e.g., copying the next table) to fail
with an "another command is already in progress" error.

[ ... ]
> + def setup_db(self) -> None:
> + """Create database and tables. MUST be called after init."""
> + os.mkdir(self.output_dir_name)
> +
> + self.connect('postgres')
> + try:
> + self.do_query(f"CREATE DATABASE {self.dbname}")

Will this query fail if the user provides a database name with hyphens
(e.g., -o my-perf-data)?

PostgreSQL usually requires double-quoted identifiers for hyphens. This might
break functionality for common naming conventions or expose the script to
SQL injection.

[ ... ]
> + self.files['evsel'] = self.open_output_file("evsel_table.bin")
> + self.files['machine'] = self.open_output_file("machine_table.bin")
> + self.files['thread'] = self.open_output_file("thread_table.bin")
> + self.files['comm'] = self.open_output_file("comm_table.bin")
> + self.files['comm_thread'] = self.open_output_file("comm_thread_table.bin")
> + self.files['dso'] = self.open_output_file("dso_table.bin")
> + self.files['symbol'] = self.open_output_file("symbol_table.bin")
> + self.files['branch_type'] = self.open_output_file("branch_type_table.bin")

The script initializes a binary file for the branch_types table, but is there
logic to populate it?

It appears the table is completely unpopulated. Will queries relying on
joining against branch_types to map branch names consistently return NULL?

[ ... ]
> + 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.ip, 8, sample.time, 4, sample.cpu,
> + 8, 0, 8, 0, 8, 0, 8, 0,
> + getattr(sample, 'period', 0),
> + getattr(sample, 'weight', 0),
> + getattr(sample, 'transaction_', 0),
> + getattr(sample, 'data_src', 0),
> + 0,
> + getattr(sample, 'in_tx', 0),
> + call_path_id,
> + getattr(sample, 'insn_count', 0),
> + getattr(sample, 'cyc_count', 0),
> + 4, getattr(sample, 'flags', 0)
> + )

Does this struct.pack format string match the number of provided arguments?

The format string requires 51 arguments to properly serialize the data into
PostgreSQL's binary COPY format, but starting around the period attribute,
the length prefix arguments seem to be omitted. Since only 42 arguments are
provided, won't this unconditionally raise a struct.error exception on the
first processed event?

[ ... ]
> + def get_comm_id(self, comm: str, thread_id: int) -> int:
> + """Get or create comm ID."""
> + if comm in self.caches['comms']:
> + return self.caches['comms'][comm]
> + comm_id = self.next_id['comm']
> + self.write_comm(comm_id, comm, thread_id, 0, 0)
> + self.write_comm_thread(comm_id, comm_id, thread_id)
> + self.caches['comms'][comm] = comm_id
> + self.next_id['comm'] += 1
> + return comm_id

If a second thread executes the same command name, the cache lookup succeeds
and bypasses the write_comm_thread() call. Does this mean no comm_thread
relationship is written for the new thread?

This might silently omit associations for any thread that happens to share a
command name with an earlier thread.

[ ... ]
> + def finalize(self) -> None:
> + """Copy files to database and add keys/views."""
> + print("Copying to database...")
> + for name, f in self.files.items():
> + self.close_output_file(f)
> + 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)

Here finalize() calls self.close_output_file(f) which closes the file, and
then immediately passes it to self.copy_output_file().

Since copy_output_file() attempts to execute f.seek(0) and f.read(65536),
will this raise a ValueError for attempting I/O operations on a closed file?

[ ... ]
> +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
> + 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:
> + print(f"Error processing events: {e}")
> + finally:
> + exporter.disconnect()

If an exception occurs during setup_db() or process_events(), the finalize()
method is bypassed.

Could this leak the temporary directory and binary trace files, causing
subsequent runs to fail with a FileExistsError on os.mkdir()?

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