On Mon, Oct 22, 2018 at 07:15:38PM -0300, Alvaro Herrera wrote:
> On 2018-Oct-22, Andres Freund wrote:
>> Hm? My point is that this fix just puts a band-aid onto *one* of the
>> places that read a XLOG_RUNNING_XACTS. Which still leaves the contents
>> of WAL record corrupted. There's not even a note at the WAL-record's
>> definition or its logging denoting that the contents are not what you'd
>> expect. I don't mean that the fix would break logical decoding, but
>> that it's possible that an equivalent of the problem affecting hot
>> standby also affects logical decoding. And even leaving those two users
>> aside, it's possible that there will be further vulernable internal
>> users or extensions parsing the WAL.
>
> Ah! I misinterpreted what you were saying. I agree we shouldn't let
> the WAL message have wrong data. Of course we shouldn't just add a
> code comment stating that the data is wrong :-)
Well, following the same kind of thoughts, txid_current_snapshot() uses
sort_snapshot() to remove all the duplicates after fetching its data
from GetSnapshotData(), so wouldn't we want to do something about
removal of duplicates if dummy PGXACT entries are found while scanning
the ProcArray also in this case? What I would think we should do is not
only to patch GetRunningTransactionData() but also GetSnapshotData() so
as we don't have duplicates also in this case, and do things in such a
way that both code paths use the same logic, and that we don't need to
have sort_snapshot() anymore. That would be more costly though...
--
Michael