Re: GetLockConflicts() and thus recovery conflicts seem pretty broken

On 27 January 2015 at 14:27, Andres Freund <andres@2ndquadrant.com> wrote:

> While investigating other bugs I noticed that
> ResolveRecoveryConflictWithLock() wasn't really working. Turns out
> GetLockConflicts() violates it's API contract which says:
>
>  * The result array is palloc'd and is terminated with an invalid VXID.
>
> Problem 1:
> We don't actually put the terminator there. It happens to more or less
> accidentally work on a master because the array is palloc0()ed there and
> while a 0 is valid backend id it happens to not be a valid local
> transaction id.

Yes, we should put the terminator there.

> In HS we don't actually allocate the array every time,
> but it's instead statically allocated. Without zeroing.

> Problem 2:
> Since bcd8528f001 and 29eedd312274 the "the result array is palloc'd" is
> wrong because we're now doing:
>
>     static VirtualTransactionId *vxids;
>         /*
>          * Allocate memory to store results, and fill with InvalidVXID.  We only
>          * need enough space for MaxBackends + a terminator, since prepared xacts
>          * don't count. InHotStandby allocate once in TopMemoryContext.
>          */
>         if (InHotStandby)
>         {
>                 if (vxids == NULL)
>                         vxids = (VirtualTransactionId *)
>                                 MemoryContextAlloc(TopMemoryContext,
>                                                    sizeof(VirtualTransactionId) * (MaxBackends + 1));
>         }
>         else
>                 vxids = (VirtualTransactionId *)
>                         palloc0(sizeof(VirtualTransactionId) * (MaxBackends + 1));
>
> Obviously that violates the API contract. I'm inclined to rip the HS
> special case out and add a pfree() to the single HS caller.

Agreed. Removing special purpose code seems like a good idea.

-- Simon Riggs                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, RemoteDBA, Training &
Services