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.
Yes, that is an important reason not to backpatch.
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.
I hadn't realised this patch holds ProcArrayLock while removing duplicates. Now I know I vote against applying this patch unless someone can show that the performance effects of doing so are negligable, which I doubt.
--
Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
