Alvaro Herrera <alvherre@commandprompt.com> wrote:
> Excerpts from Kevin Grittner's message:
> No strong opinion on this, really, but your strcpy should use a
> StringInfo buffer instead of the char[200]. That's going to bite
> someone.
Yeah, this was thrown together in a bit of a hurry because of
development deadlines here, so I cut a few corners based on
knowledge of our particular implementation details. I know that's
something to change for general acceptance.
>> It's using these functions:
>>
>> SPI_getrelname
>> SPI_fname
>> SPI_getvalue
>>
>> If there's a better way to get the info, I'm game.
>
> I think you could get away without the first two (in particular
> get rid of the memleak with SPI_getrelname), but the last one
> would require something more involved. No strong opinion, I just
> failed to see those calls in there.
I thought the trigger would be running in a context which would make
that leak immaterial. I thought the general advice in such cases is
to *not* do retail freeing of space, but to let it get cleaned up
through release of the memory context. I'll take another look at
memory contexts around triggers.
> Is this intended for 9.1?
Definitely not. I added it to the first 9.2 CF, as mentioned in
earlier posts. I was going to hold off posting until the beta was
wrapped, but it seemed reasonable to post the patch in response to
Dimitri's post. I wasn't intending to suggest it was ready for
general usage; I was mainly just putting it out there to see if
anyone was interested enough in it that I should polish it up for a
proper submission. For instance, there are no docs or regression
tests yet.
Anyway, I certainly appreciate the pointers, because we have to push
something out to production along these lines in a couple months to
stay on track with the organization's Annual Plan, which we need to
provide to the legislature and are judged against when they
authorize funding each year. I think your advice will bring this
feature from "it works" to "it works really well". :-)
-Kevin