FWIW, a couple of additional comments based on eyeballing the diffs: 1) twophase.c ---------
I think this comment is slightly inaccurate: /* * Coordinate with logical decoding backends that may be already * decoding this prepared transaction. When aborting a transaction, * we need to wait for all of them to leave the decoding group. If * committing, we simply remove all members from the group. */ Strictly speaking, we're not waiting for the workers to leave the decoding group, but to set decodeLocked=false. That is, we may proceed when there still are members, but they must be in unlocked state. 2) reorderbuffer.c ------------------ I've already said it before, I find the "flags" bitmask and rbtxn_* macros way less readable than individual boolean flags. It was claimed this was done on Andres' request, but I don't see that in the thread. I admit it's rather subjective, though. I see ReorederBuffer only does the lock/unlock around apply_change and RelationIdGetRelation. That seems insufficient - RelidByRelfilenode can do heap_open on pg_class, for example. And I guess we need to protect rb->message too, because who knows what the plugin does in the callback? Also, we should not allocate gid[GIDSIZE] for every transaction. For example subxacts never need it, and it seems rather wasteful to allocate 200B when the rest of the struct has only ~100B. This is particularly problematic considering ReorderBufferTXN is not spilled to disk when reaching the memory limit. It needs to be allocated ad-hoc only when actually needed. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
