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

[sashiko-bot@kernel.org]

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