|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|
|Views:||Raw Message | Whole Thread | Download mbox|
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
> > + 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:
(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, 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
Ok, I've made this change.
Neil Conway <neilconway(at)rogers(dot)com>
PGP Key ID: DB3C29FC
|Next Message||Tom Lane||2002-07-19 21:15:43||Re: lock listing|
|Previous Message||Tom Lane||2002-07-19 20:32:54||Re: prepareable statements|