On Fri, Apr 15, 2016 at 1:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Thu, Apr 14, 2016 at 12:11 PM, Andres Freund <andres@anarazel.de> wrote: >>> The easiest way to achieve that seems to be to just assign an xid if >>> that's the case; while it's not necessarily safe/efficient to do so at >>> the point the invalidation message was queued, I think it should be safe >>> to do so at commit time. Seems less invasive to backpatch than to either >>> support commit records without xids, or a separate record just >>> transporting invalidation messages. > >> I agree that's better for back-patching. I hope it won't suck >> performance-wise. In master, we might think of inventing something >> new. > > I'm a little worried about whether this will break assumptions that > vacuum doesn't have an XID. I don't immediately see how it would, > but it seems a bit shaky.
Actually, I think there's a bigger problem. If you manage to almost wrap around the XID space, and the cluster shuts down, you are guaranteed to be able to vacuum the whole cluster without actually running out of XIDs. Forcing an XID to be assigned here would make that uncertain - it would depend on how many tables you have versus how many XIDs you have left. That seems unacceptable to me.
I agree this is a bug, similar to the last one.
For me, the issue is that we need to do something to catch bugs. The existing code does not have any test at all to check whether we are doing the wrong thing - it just lets the wrong thing happen.
Fixing it by forcing a new behaviour might be the right thing to do going forwards, but I don't much like the idea of forcing new behaviour in back branches. It might fix this bug, but can easily cause others.
--
Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
