Re: cleaning up a few CLOG-related things

On Wed, Jan 27, 2021 at 12:35:30PM -0500, Robert Haas wrote:
> On Mon, Jan 25, 2021 at 2:11 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> > Having a separate FullTransactionIdToLatestPageNumber() function for
> > this seems like overkill to me.
> 
> I initially thought so too, but it turned out to be pretty useful for
> writing debugging cross-checks and things, so I'm inclined to keep it
> even though I'm not at present proposing to commit any such debugging
> cross-checks. For example I tried making the main redo loop check
> whether XactCtl->shared->latest_page_number ==
> FullTransactionIdToLatestPageNumber(nextXid) which turned out to be
> super-helpful in understanding things.

> +/*
> + * Based on ShmemVariableCache->nextXid, compute the latest CLOG page that
> + * is expected to exist.
> + */
> +static int
> +FullTransactionIdToLatestPageNumber(FullTransactionId nextXid)
> +{
> +	/*
> +	 * nextXid is the next XID that will be used, but we want to set
> +	 * latest_page_number according to the last XID that's already been used.
> +	 * So retreat by one. See also GetNewTransactionId().
> +	 */
> +	FullTransactionIdRetreat(&nextXid);
> +	return TransactionIdToPage(XidFromFullTransactionId(nextXid));
> +}

I don't mind the arguably-overkill function.  I'd probably have named it
FullTransactionIdPredecessorToPage(), to focus on its behavior as opposed to
its caller's behavior, but static function naming isn't a weighty matter.
Overall, the patch looks fine.  If nextXid is the first on a page, the next
GetNewTransactionId() -> ExtendCLOG() -> ZeroCLOGPage() -> SimpleLruZeroPage()
is responsible for updating latest_page_number.