> Please find attached the v3 patch. It ensures the current index is
> reset after each index (so vacuuming heap and truncating heap show
> NULL arrays with no stale relid), fixes the docs for the type of
> index_vacuum_pids, adds a note in the docs about the arrays being
> position-aligned, and rewords the commit message a bit.
Thanks for the updates in v3.
It turns out, to my surprise, that leader_pid can be NULL if the user
querying pg_stat_progress_vacuum does not have proper privileges, either
pg_read_all_stats or membership in the role running the vacuum.
Here is the case. "foo" is created with
```
CREATE ROLE foo LOGIN;
GRANT CONNECT ON DATABASE postgres TO foo;
```
A superuser aggregates correctly on leader_pid, because the workers
emit leader_pid in pg_stat_activity.
```
pid | phase | index_vacuum_pids | index_vacuum_oids
-------+-------------------+---------------------+---------------------
23492 | vacuuming indexes | {23492,23570,23571} | {16389,16390,16391}
(1 row)
pid | leader_pid | backend_type | state | query
-------+------------+-----------------+--------+------------------------------
23492 | | client backend | active | VACUUM (PARALLEL 4) vac_demo;
23570 | 23492 | parallel worker | active | VACUUM (PARALLEL 4) vac_demo;
23571 | 23492 | parallel worker | active | VACUUM (PARALLEL 4) vac_demo;
(3 rows)
```
But "foo" cannot, because leader_pid is NULL for this user, so the
aggregation falls apart and each worker emits its own row in
pg_stat_progress_vacuum.
pid | phase | index_vacuum_pids | index_vacuum_oids
-------+-------+-------------------+-------------------
23492 | | |
23570 | | |
23571 | | |
(3 rows)
I think for this patch we should drop the reliance on pg_stat_activity
and have pg_stat_get_progress_info() emit leader_pid directly and
unconditionally. The aggregation in the view then works regardless of
the caller's
privileges. It is the same lockGroupLeader value pg_stat_activity
already computes.
```
pg_stat_get_progress_info(PG_FUNCTION_ARGS)
{
-#define PG_STAT_GET_PROGRESS_COLS PGSTAT_NUM_PROGRESS_PARAM + 3
+#define PG_STAT_GET_PROGRESS_COLS PGSTAT_NUM_PROGRESS_PARAM + 4
int num_backends = pgstat_fetch_stat_numbackends();
int curr_backend;
char *cmd = text_to_cstring(PG_GETARG_TEXT_PP(0));
@@ -373,6 +373,7 @@ pg_stat_get_progress_info(PG_FUNCTION_ARGS)
{
LocalPgBackendStatus *local_beentry;
PgBackendStatus *beentry;
+ PGPROC *proc;
Datum values[PG_STAT_GET_PROGRESS_COLS] = {0};
bool nulls[PG_STAT_GET_PROGRESS_COLS] = {0};
int i;
@@ -391,6 +392,23 @@ pg_stat_get_progress_info(PG_FUNCTION_ARGS)
values[0] = Int32GetDatum(beentry->st_procpid);
values[1] = ObjectIdGetDatum(beentry->st_databaseid);
+ proc = BackendPidGetProc(beentry->st_procpid);
+ if (proc != NULL && proc->lockGroupLeader != NULL &&
+ proc->lockGroupLeader->pid != beentry->st_procpid)
+ values[PGSTAT_NUM_PROGRESS_PARAM + 3] =
+ Int32GetDatum(proc->lockGroupLeader->pid);
+ else
+ values[PGSTAT_NUM_PROGRESS_PARAM + 3] =
Int32GetDatum(0);
+
```
That leaves a more interesting question in my mind, which is why
pg_stat_activity puts leader_pid behind permissions at all. It should be
treated just like pid.
There is probably a larger discussion around what should and should not
be permission controlled in pg_stat_activity, and I could not find a
consistent rule. For example, we do not permission control application_name,
which is user controlled free text, yet we do permission control
query_id, which
is not permission controlled elsewhere such as pg_stat_statements. We probably
need a separate thread to clearly lay out the principles for this.
As far as this patch goes, I don't think it should be blocked and it should
continue to emit the leader_pid, but with the idea I shared above
instead of joining with pg_stat_activity.
thoughts?
--
Sami Imseih
Amazon Web Services (AWS)
