Re: Problem with txid_snapshot_in/out() functionality

On 04/12/14 10:03, Andres Freund wrote:
> On 2014-04-12 09:47:24 -0400, Tom Lane wrote:
>> Heikki Linnakangas <hlinnakangas@vmware.com> writes:
>> > On 04/12/2014 12:07 AM, Jan Wieck wrote:
>> >> the Slony team has been getting seldom reports of a problem with the
>> >> txid_snapshot data type.
>> >> The symptom is that a txid_snapshot on output lists the same txid
>> >> multiple times in the xip list part of the external representation.
>>
>> > It's two-phase commit. When preparing a transaction, the state of the
>> > transaction is first transfered to a dummy PGXACT entry, and then the
>> > PGXACT entry of the backend is cleared. There is a transient state when
>> > both PGXACT entries have the same xid.
>>
>> Hm, yeah, but why is that intermediate state visible to anyone else?
>> Don't we have exclusive lock on the PGPROC array while we're doing this?
>
> It's done outside the remit of ProcArray lock :(. And documented to lead
> to duplicate xids in PGXACT.
> EndPrepare():
>     /*
>      * Mark the prepared transaction as valid.    As soon as xact.c marks
>      * MyPgXact as not running our XID (which it will do immediately after
>      * this function returns), others can commit/rollback the xact.
>      *
>      * NB: a side effect of this is to make a dummy ProcArray entry for the
>      * prepared XID.  This must happen before we clear the XID from MyPgXact,
>      * else there is a window where the XID is not running according to
>      * TransactionIdIsInProgress, and onlookers would be entitled to assume
>      * the xact crashed.  Instead we have a window where the same XID appears
>      * twice in ProcArray, which is OK.
>      */
>     MarkAsPrepared(gxact);
>
> It doesn't sound too hard to essentially move PrepareTransaction()'s
> ProcArrayClearTransaction() into MarkAsPrepared() and rejigger the
> locking to remove the intermediate state. But I think it'll lead to
> bigger changes than we'd be comfortable backpatching.
>
>> If we don't, aren't we letting other backends see non-self-consistent
>> state in regards to who holds which locks, for example?
>
> I think that actually works out ok, because the locks aren't owned by
> xids/xacts, but procs. Otherwise we'd be in deep trouble in
> CommitTransaction() as well where ProcArrayEndTransaction() clearing
> that state.
> After the whole xid transfer, there's PostPrepare_Locks() transferring
> the locks.

Since it doesn't seem to produce any side effects, I'd think that making
the snapshot unique within txid_current_snapshot() and filtering
duplicates on input should be sufficient and eligible for backpatching.

The attached patch adds a unique loop to the internal sort_snapshot()
function and skips duplicates on input. The git commit is here:

https://github.com/wieck/postgres/commit/a88a2b2c25b856478d7e2b012fc718106338fe00


Regards,
Jan

--
Jan Wieck
Senior Software Engineer
http://slony.info