Re: lock listing
От | Tom Lane |
---|---|
Тема | Re: lock listing |
Дата | |
Msg-id | 13822.1027099270@sss.pgh.pa.us обсуждение исходный текст |
Ответ на | lock listing (nconway@klamath.dyndns.org (Neil Conway)) |
Ответы |
Re: lock listing
|
Список | pgsql-patches |
nconway@klamath.dyndns.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
В списке pgsql-patches по дате отправления: