On Thu, Mar 23, 2023 at 11:02 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> [ new patch ]
Well, I guess nobody is too excited about fixing this, because it's
been another 10 months with no discussion. Andres doesn't even seem to
think this is as much a bug as it is a limitation, for all that it's
filed in the CF application under bug fixes. I kind of wonder if we
should just close this entry in the CF, but I'll hold off on that for
now.
/*
* For normal MVCC snapshot this contains the all xact IDs that are in
* progress, unless the snapshot was taken during recovery in which case
- * it's empty. For historic MVCC snapshots, the meaning is inverted, i.e.
- * it contains *committed* transactions between xmin and xmax.
+ * it's empty. In the case of !suboverflowed, there's a situation where
+ * this contains both top and sub-transaction IDs in a mixed manner. For
+ * historic MVCC snapshots, the meaning is inverted, i.e. it contains
+ * *committed* transactions between xmin and xmax.
*
* note: all ids in xip[] satisfy xmin <= xip[i] < xmax
*/
I have to say that I don't like this at all. It's bad enough that we
already use the xip/subxip arrays in two different ways depending on
the situation. Increasing that to three different ways seems painful.
How is anyone supposed to keep track of how the array is being used at
which point in the code?
If we are going to do that, I suspect it needs comment updates in more
places than what the patch does currently. For instance:
+ if (newxcnt < xiplen)
+ newxip[newxcnt++] = xid;
+ else
+ newsubxip[newsubxcnt++] = xid;
Just imagine coming across this code in 5 or 10 years and finding that
it had no comment explaining anything. Yikes!
Aside from the details of the patch, and perhaps more seriously, I'm
not really clear that we have consensus on an approach. A few
different proposals seem to have been floated, and it doesn't seem
like anybody hates anybody else's idea completely, but it doesn't
really seem like everyone agrees on what to do, either.
--
Robert Haas
EDB: http://www.enterprisedb.com
