On Fri, Nov 30, 2018 at 02:55:47PM +0000, Simon Riggs wrote:
> 1df21ddb looks OK to me and was simple enough to backpatch safely.
Thanks for the feedback!
> Seems excessive to say that the WAL record is corrupt, it just contains
> duplicates, just as exported snapshots do. There's a few other imprecise
> things around in WAL, that is why we need the RunningXact data in the first
> place. So we have a choice of whether to remove the duplicates eagerly or
> lazily.
>
> For GetRunningTransactionData(), we can do eager or lazy, since its not a
> foreground process. I don't object to changing it to be eager in this path,
> but this patch is more complex than 1df21ddb and I don't think we should
> backpatch this change, assuming it is acceptable.
Yes, I would avoid a backpatch for this more complicated one, and
we need a solution for already-generated WAL. It is not complicated to
handle duplicates for xacts and subxacts however holding ProcArrayLock
for a longer time stresses me as it is already a bottleneck.
> This patch doesn't do it, but the suggestion that we touch
> GetSnapshotData() in the same way so we de-duplicate eagerly is a different
> matter and would need careful performance testing to ensure we don't slow
> down 2PC users.
Definitely. That's a much hotter code path. I am not completely sure
if that's an effort worth pursuing either..
--
Michael