LSN grouping within clog pages

I've been looking at the way that the async-commit patch conserves
shared memory space by remembering async commit LSNs for groups of
transactions on a clog page, rather than having an LSN for each
individual transaction slot. This seems like a good plan to me,
but I'm confused about one point. The README text claims that each
LSN represents a contiguous group of transactions, that is, with the
proposed parameters each LSN would represent 256 sequential
transactions. However, it looks to me that what the code is actually
doing:

#define GetLSNIndex(slotno, xid) ((slotno) * CLOG_LSNS_PER_PAGE + \
(xid) % (TransactionId) CLOG_XACTS_PER_LSN)

results in transactions that are spaced 256 XIDs apart sharing the same
LSN slot. I'm not sure whether the code is good and the README is
bogus, or vice versa. Sharing LSNs among contiguous groups of XIDs
seems appealing because you'd expect that such a group would have
relatively close LSNs, and so not much information is lost. OTOH, the
modulo idea is interesting too, because if the transaction rate is less
than 256 commits per walwriter cycle, you'd effectively have exact
information for all the currently unflushed transactions. But the
downside is that transactions that are really quite old might
transiently appear un-hintable because some later transaction that
happens to share that LSN slot isn't flushed yet. Thoughts?

BTW, I don't think I believe at all the arguments given in the README
about what CLOG_LSNS_PER_PAGE should be, particularly since possible
changes in BLCKSZ weren't factored in. I'm inclined to set it so
that the LSNs take up the same amount of space as the clog buffers
themselves, ie, BLCKSZ/8 LSNs per page.

regards, tom lane