| From: | Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com> |
|---|---|
| To: | shihao zhong <zhong950419(at)gmail(dot)com> |
| Cc: | Michael Paquier <michael(at)paquier(dot)xyz>, Jim Jones <jim(dot)jones(at)uni-muenster(dot)de>, pgsql-hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: Add a permission check to pg_stat_get_backend_subxact() |
| Date: | 2026-09-23 07:23:01 |
| Message-ID: | arN+VT3xyiTY/iUD@bdtpg |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi,
On Tue, Sep 22, 2026 at 11:22:40PM -0400, shihao zhong wrote:
> > Do you mean adding the user ID on top of Shihao's 0002? If so, I can have
> a look,
> > unless Shihao is already planning to update the patch along those lines?
>
> Done in v8, attached.
Thanks!
I've a few comments:
> 0001 is the PID check, same as v7-0002. It goes first now because 0002
> needs a field in PgStat_Backend.
>
> 0002 stores the user ID in PgStat_Backend next to the PID when the entry
> is created, and pg_stat_get_backend_io(), wal() and lock() check the
> caller against that instead of the beentry. The reset callback restores
> both fields. pg_stat_get_backend_subxact() still checks the beentry, the
> counters it reports come from there.
=== 1
pgstat_read_current_status() first copies the activity entry and then calls
ProcNumberGetTransactionIds() separately. If the backend exits and its
ProcNumber is reused in between, the userid can belong to the old backend
while the subxact counters belong to the new one.
This race exists before the patch, but it matters for the new permission check.
I wonder if we should pass the copied PID to ProcNumberGetTransactionIds() and
validate it under ProcArrayLock, following the same idea as
pgstat_fetch_stat_backend_by_pid()?
=== 2
typedef struct PgStat_Backend
{
+ int pid; /* PID of the backend owning these stats */
TimestampTz stat_reset_timestamp;
0002 explicitly says that it is not intended for backpatching, but what about
0001? If it is backpatched to v18, adding pid here changes the offsets of all
the existing fields.
I think it would make sense to add pid at the end in the backbranches (if we
back patch it), as suggested in [1]. That would preserve the existing field
offsets, though it would still change sizeof(PgStat_Backend). FWIW, I could not
find any use of sizeof(PgStat_Backend) in a GitHub code search and there is
no padding to add the new field into.
[1]: https://wiki.postgresql.org/wiki/Committing_checklist
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Tristan Partin | 2026-09-23 07:26:47 | Re: Add counted_by attribute |
| Previous Message | Chao Li | 2026-09-23 07:06:29 | Re: [PATCH] pg_combinebackup: make the OID range check in parse_oid() effective |