Re: Patch: show xid and xmin in pg_stat_activity and pg_stat_replication

Hi Andres,

ok, lesson learned: don't refactor patches in a rush ;)

> [… documation issues …]

Should be fixed according to your critics.

> > @@ -2785,6 +2787,8 @@ pgstat_read_current_status(void)
> > volatile PgBackendStatus *beentry;
> > PgBackendStatus *localtable;
> > PgBackendStatus *localentry;
> > + PGPROC *proc;
> > + PGXACT *xact;
>
> A bit hard to read from the diff only, but aren't they now unused?

Right. Removed.
(This file creates so many warnings with -W -Wall -ansi -pedantic that
I didn't notice that specific warning)

> > /*
> > + * BackendIdGetTransactionIds
> > + * Get the PGPROC structure for a backend, given the backend ID. Also
> > + * get the xid and xmin of the backend. The result may be out of date
> > + * arbitrarily quickly, so the caller must be careful about how this
> > + * information is used. NULL is returned if the backend is not active.
> > + */
> > +PGPROC *
> > +BackendIdGetTransactionIds(int backendPid, TransactionId *xid, TransactionId *xmin)
> > +{
>
> Hm, why do you even return the PGPROC here? Not that's problematic, but
> it seems a bit pointless. If you remove it you can remove a fair bit of
> the documentation. I think it should note instead that the two values
> can be out of whack with each other.

Originally I returned PGPROC to kill to birds with one stone and to be
able to distinguish an error case, e.g. when the backend could not be
found. But in favor of code quality I think your complaint is right
and I should not do that. Instead now xid and xmin are initialized
with InvalidTransactionId.

> Uh, why do we suddenly need the loop here? BackendIdGetProc() works
> without one, so this certainly shouldn't need it, or am I missing
> something? Note that Robert withdrew his complaint about relying on
> indexing via BackendId in
> CA+TgmoaFjcji=Hu9YhCSEL0Z116TY2zEKZf7Z5=da+BPTeytoA(at)mail(dot)gmail(dot)com .

Because I was in a rush. Originally I wrote this getter in reaction to
Robert's complaints and refactored it when he took back. But I forgot
to remove the loop, too. Won't happen again, I won't refactor in a
hurry again ;-)

Attached you will find an updated version of the patch.

Best regards,

--
Christian Kruse http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services