Skip site navigation (1) Skip section navigation (2)

Re: lock listing

From: nconway(at)klamath(dot)dyndns(dot)org (Neil Conway)
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: PostgreSQL Patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: lock listing
Date: 2002-07-19 21:04:09
Message-ID: 20020719210409.GA23636@klamath.dyndns.org (view raw or flat)
Thread:
Lists: 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(at)rogers(dot)com>
PGP Key ID: DB3C29FC

In response to

Responses

pgsql-patches by date

Next:From: Tom LaneDate: 2002-07-19 21:15:43
Subject: Re: lock listing
Previous:From: Tom LaneDate: 2002-07-19 20:32:54
Subject: Re: prepareable statements

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group