Re: Memory-leak in BackgroundWriter(and Checkpointer)

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Meh. I'm not impressed with permanently allocating an array large
> enough to hold all the locks GetRunningTransactionLocks
> might return --- that's potentially much larger than the other array,
> and in fact I don't think we have a hard limit on its size at all.

Well, sure, which is why I didn't actually do that- but I did end up
having to make it resize when necessary, which isn't entirely ideal
either.

> Besides which, it's not like there is *no* cleanup for
> GetRunningTransactionData --- it has a lock that has to be released ...

That's true.. I guess my general feeling is that it'd be good to do
this all one way or the other- having it use a static variable into
which we stick the pointer to some reused space for one and then doing a
palloc for the other which needs to be pfree'd struck me as odd.

> I think the proposed fix is fine code-wise; the real problem here is
> crummy commenting. GetRunningTransactionLocks isn't documented as
> returning a palloc'd array, and why the heck do we have a long comment
> about its implementation in LogStandbySnapshot?

Certainly good questions and better comments would have helped here. I
can go back and rework the patch either way.

Thanks,

Stephen