trivial patch: show SIREAD pids in pg_locks

While looking into a SSI bug, I noticed that we don't actually display
the pid of the holding transaction, even though we have that
information available.

The attached patch fixes that.

One note is that it will show the pid of the backend that executed the
transaction, even if that transaction has already committed. I have no
particular opinion about whether it's more useful to do that or return
null, so went with the smallest change. (The pid is null for PREPARED
or summarized transactions).

Dan

--
Dan R. K. Ports MIT CSAIL http://drkp.net/

Dan Ports <drkp(at)csail(dot)mit(dot)edu> wrote:
> While looking into a SSI bug, I noticed that we don't actually
> display the pid of the holding transaction, even though we have
> that information available.

I thought we already had that, but clearly I was mistaken.

> The attached patch fixes that.
>
> One note is that it will show the pid of the backend that executed
> the transaction, even if that transaction has already committed. I
> have no particular opinion about whether it's more useful to do
> that or return null, so went with the smallest change. (The pid is
> null for PREPARED or summarized transactions).

The patch looks good to me and a quick test shows the expected
behavior. No warnings. Regression tests pass.

I guess the question is whether it's OK to include this during the
alpha testing phase. Even though it's a little bit of a stretch to
call it a bug, the argument could be made that omitting information
which all the other rows in the view have is an inconsistency which
borders on being a bug. The small size and verifiable safety of the
patch work in its favor.

-Kevin

On Fri, Apr 01, 2011 at 12:20:25PM -0500, Kevin Grittner wrote:
> I thought we already had that, but clearly I was mistaken.

Yeah, so did I. Turns out we had the vxid but not the pid. IIRC, we
weren't tracking a SERIALIZABLEXACT's pid yet, at the time we wrote the
code for pg_locks.

> I guess the question is whether it's OK to include this during the
> alpha testing phase. Even though it's a little bit of a stretch to
> call it a bug, the argument could be made that omitting information
> which all the other rows in the view have is an inconsistency which
> borders on being a bug. The small size and verifiable safety of the
> patch work in its favor.

There's no urgent need to have this, although it's obviously more
correct than the current behavior. It might be useful for debugging. The
patch is also all of four lines. :-)

Dan

--
Dan R. K. Ports MIT CSAIL http://drkp.net/

Dan Ports <drkp(at)csail(dot)mit(dot)edu> wrote:

> There's no urgent need to have this, although it's obviously more
> correct than the current behavior. It might be useful for
> debugging.

Agreed all around. For the benefit of the more casual follower of
the thread, attached is a simple example of the output. For the
SIReadLock rows, the pid column shows as empty without the patch.

-Kevin

On Fri, 2011-04-01 at 13:00 -0400, Dan Ports wrote:
> While looking into a SSI bug, I noticed that we don't actually display
> the pid of the holding transaction, even though we have that
> information available.

Is there a chance that the PID will reference a backend that has either
terminated or is idle? That might be confusing.

Regards,
Jeff Davis

Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> On Fri, 2011-04-01 at 13:00 -0400, Dan Ports wrote:
>> While looking into a SSI bug, I noticed that we don't actually
>> display the pid of the holding transaction, even though we have
>> that information available.
>
> Is there a chance that the PID will reference a backend that has
> either terminated or is idle?

Yes to both.

> That might be confusing.

With a barely larger patch we could suppress that, if that's
desirable. Of course, there is already the same issue for the
virtualtransaction column.

As Dan mentioned, it won't show for locks which are summarized using
SLRU, nor will it show for prepared transactions pending final
commit -- at least if they're loaded from disk after recovery. (I'm
not sure without some digging about a transaction which is prepared
and still pending commit if the server is still running from the
point of prepare.)

-Kevin

On Fri, Apr 1, 2011 at 1:00 PM, Dan Ports <drkp(at)csail(dot)mit(dot)edu> wrote:
> While looking into a SSI bug, I noticed that we don't actually display
> the pid of the holding transaction, even though we have that
> information available.
>
> The attached patch fixes that.

Committed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company