Re: lock listing - Mailing list pgsql-patches

On Fri, Jul 19, 2002 at 01:21:10PM -0400, Tom Lane wrote:
> Why does this patch arbitrarily remove the #ifdefs and documentation
> for USER_LOCKS?  That seems quite unrelated to the stated purpose.

I probably should have mentioned that -- it looked like dead code,
and the behavior that it referred to (a loadable module called
user-locks.c, which doesn't make sense to begin with) doesn't
appear to exist anymore.

> I'm very unthrilled with this approach to faking up a composite type
> for pg_show_locks to return.

As am I, and I agree that the proper long-term answer is some new
infrastructure for adding builtin SRFs. However, I don't think that's
a really good reason for rejecting the patch -- the dependancy
implications aren't a big deal IMHO (who drops pg_* anyway?), and
the ugliness is limited to a single place in initdb.sh, which would
be very easy to adapt to a new composite type setup.

> > 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.

Well, my preference would be to write the documentation when/if the
patch is applied, so I don't waste my time documentating a feature
that may not exist.

As for the higher-level views, I'd personally view (heh) those as
optional -- IMHO the data returned by the SRF is sufficient for most
competent admins, and is certainly all that a GUI tool would need.
I'll certainly think about whether some high-level views are needed
(and if so, which ones), but I'm not too concerned about it.

> 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.

Can you elaborate a bit on what changes you'd like to see? Can this
be done with the data structures I'm using now, or do I need to roll
my own?

> > +         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.

Okay -- I've replaced that with:

    RelationNameGetTupleDesc("pg_catalog.pg_show_locks_result");

(The SRF API docs should probably include a note on that.)

> > +         /*
> > +          * 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.

Hmmm -- I tested it with '--enable-cassert' and didn't observe any
problems. I've revised to patch to allocate that long-term memory in
FuncCallContext.fctx, which I think should be sufficiently long-lived.

> > +         /* 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?

Ok, both of these are fixed.

> > +              * 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
> [snip]

Ok, I've made this change.

Cheers,

Neil

--
Neil Conway <neilconway@rogers.com>
PGP Key ID: DB3C29FC