Re: lock listing

nconway(at)klamath(dot)dyndns(dot)org (Neil Conway) writes:
> The attached patch completes the TODO list item
> * Add SHOW command to display locks

Sorry for not having time to review this sooner, but better late than
never...

Why does this patch arbitrarily remove the #ifdefs and documentation
for USER_LOCKS? That seems quite unrelated to the stated purpose.

I'm very unthrilled with this approach to faking up a composite type
for pg_show_locks to return. Aside from being ugly, this will lead to a
broken dependency structure (pg_show_locks is pinned, being a
built-in function, but its return type isn't pinned and yet there's no
dependency for it). We need a better answer for making built-in
functions that return tuples. I don't have one yet, but I also don't
think that this function is so critical to have in the main backend that
we must put in a kluge to have it there. I still think that the right
short-term answer is to make the function be a contrib item so it can
have an install script. (I don't object to adding stuff to lmgr for the
function to call.)

> When/if the patch is applied, I'll send in another patch adding some
> documentation, and perhaps some higher-level views that use the SRF
> and the system catalogs to return some useful information.

I'd like to see the documentation and the views *first*, as evidence
that this is the right API for the function to have. It may be that
we'll need more or different processing in the function to produce
something that can be displayed usefully by a view.

Minor code complaints:

It seems to me that you could do one palloc, or at most three, while
holding the LockMgrLock --- doing three per holderTable entry will take
significantly longer.

> + tupdesc = RelationNameGetTupleDesc("pg_show_locks_result");

This is certainly unreliable in the schema world; you might get a tupledesc
for some other pg_show_locks_result relation than the one you want.

> + /*
> + * Preload all the locking information that we will eventually format
> + * and send out as a result set. This is palloc'ed, but since the
> + * MemoryContext is reset when the SRF finishes, we don't need to
> + * free it ourselves.
> + */
> + funccxt->user_fctx = (LockData *) palloc(sizeof(LockData));
> +
> + GetLockStatusData(funccxt->user_fctx);

This seems quite wrong; the active context when the function is called
will be a short-term context, which *will* get reset before the next
call. Have you tested this with --enable-cassert (which turns on
CLOBBER_FREED_MEMORY)? I think you need to switch into a longer-lived
context before calling GetLockStatusData.

> + /* The OID of the locked relation */
> + snprintf(values[0], 16, "%d", lock->tag.relId);

OIDs are unsigned and must be printed with %u (same problem multiple
places). Also, why are you using "16" as the buffer length here when
you allocated 32 bytes?

> + * procede to the next one. (Note: "Go To Statement Considered
> + * Harmful" notwithstanding, GOTO is appropriate here IMHO)

Personally, I'd replace "top:" and the if/else construct with

/* loop in case currIdx proves not to have any more locks */
while (lockData->currIdx < lockData->nelements)
{
PROCLOCK *holder;
...
SRF_RETURN_NEXT(funccxt, result);
}

SRF_RETURN_DONE(funccxt);

and use "continue" in place of "goto". This is arguably cleaner,
and will definitely result in better code (many compilers punt on
optimization if there's any "goto" in the routine).

> + /* Cleanup and return next tuple in result set */
> + for (i = 0; i < NUM_ATTRS; i++)
> + pfree(values[i]);
> + pfree(values);

The pfree's here are largely a waste of cycles if I'm right about the
memory management behavior. Also, instead of using a NUM_ATTRS #define
you could get the number of attributes from the tupledesc, which might
be slightly more maintainable.

regards, tom lane