Hi, On 2020-04-07 16:13:07 -0400, Robert Haas wrote: > On Tue, Apr 7, 2020 at 3:24 PM Andres Freund <and...@anarazel.de> wrote: > > > + ProcGlobal->xids[pgxactoff] = InvalidTransactionId; > > > > > > Apparently this array is not dense in the sense that it excludes > > > unused slots, but comments elsewhere don't seem to entirely agree. > > > > What do you mean with "unused slots"? Backends that committed? > > Backends that have no XID. You mean, I guess, that it is "dense" in > the sense that only live backends are in there, not "dense" in the > sense that only active write transactions are in there.
Correct. I tried the "only active write transaction" approach, btw, and had a hard time making it scale well (due to the much more frequent moving of entries at commit/abort time). If we were to go to a 'only active transactions' array at some point we'd imo still need pretty much all the other changes made here - so I'm not worried about it for now. > > > + uint32 TotalProcs = MaxBackends + NUM_AUXILIARY_PROCS + > > > max_prepared_xacts; > > > > > > /* ProcGlobal */ > > > size = add_size(size, sizeof(PROC_HDR)); > > > - /* MyProcs, including autovacuum workers and launcher */ > > > - size = add_size(size, mul_size(MaxBackends, sizeof(PGPROC))); > > > - /* AuxiliaryProcs */ > > > - size = add_size(size, mul_size(NUM_AUXILIARY_PROCS, sizeof(PGPROC))); > > > - /* Prepared xacts */ > > > - size = add_size(size, mul_size(max_prepared_xacts, sizeof(PGPROC))); > > > - /* ProcStructLock */ > > > + size = add_size(size, mul_size(TotalProcs, sizeof(PGPROC))); > > > > > > This seems like a bad idea. If we establish a precedent that it's OK > > > to have sizing routines that don't use add_size() and mul_size(), > > > people are going to cargo cult that into places where there is more > > > risk of overflow than there is here. > > > > Hm. I'm not sure I see the problem. Are you concerned that TotalProcs > > would overflow due to too big MaxBackends or max_prepared_xacts? The > > multiplication itself is still protected by add_size(). It didn't seem > > correct to use add_size for the TotalProcs addition, since that's not > > really a size. And since the limit for procs is much lower than > > UINT32_MAX... > > I'm concerned that there are 0 uses of add_size in any shared-memory > sizing function, and I think it's best to keep it that way. I can't make sense of that sentence? We already have code like this, and have for a long time: /* Size of the ProcArray structure itself */ #define PROCARRAY_MAXPROCS (MaxBackends + max_prepared_xacts) adding NUM_AUXILIARY_PROCS doesn't really change that, does it? > If you initialize TotalProcs = add_size(MaxBackends, > add_size(NUM_AUXILIARY_PROCS, max_prepared_xacts)) then I'm happy. Will do. Greetings, Andres Freund