Optimize vector get last written LSN

[knizhnik]

No description provided.

[hlinnaka]

Looks correct comparing with the old code. I have some doubts about this though that are applicable to both the old and the new version though:

1. Why does GetLastWrittenLSNv() call SetLastWrittenLSNForBlock() for pages that were not in the cache previously? There are no comments on that and I find it pretty surprising. Luckily the PR description that added it Remember last written LSN when it is first requested #412 explains it. Let's copy that explanation to a comment in GetLastWrittenLSNv().

2. Is it correct to release the lock and then call SetLastWrittenLSNForBlock()? If the page is concurrently updated and written out, the LSN we set here might override the LSN that was set by the concurrent updater. There was discussion on PR Remember last written LSN when it is first requested #412 about a concurrent read scenario. ISTM it was considered to be just a performance issue, but isn't it also a potential correctness issue if the last-written LSN of a page is moved backwards?

3. I don't know if it matters from a performance point of view in practice, but it seems wasteful to release LastWrittenLsnLock, when SetLastWrittenLSNForBlock() will immediately re-acquire it. Instead of calling SetLastWrittenLSNForBlock(), you could hold the lock and copy some of the logic from SetLastWrittenLSNForBlock() into GetLastWrittenLSNv() itself, or introduce some common helper functions to reduce the duplication. That would also address the previous point about concurrent updates.

[knizhnik]

Let's copy that explanation to a comment in GetLastWrittenLSNv().

Done

Is it correct to release the lock and then call SetLastWrittenLSNForBlock()? STM it was considered to be just a performance issue, but isn't it also a potential correctness issue if the last-written LSN of a page is moved backwards?

Values can not be moved backward, because there is check in SetLastWrittenLSN performed in critical section which prevents it. But see 3)

I don't know if it matters from a performance point of view in practice, but it seems wasteful to release LastWrittenLsnLock, when SetLastWrittenLSNForBlock() will immediately re-acquire it.

Fixed. I did it only for pg17 because it is just optimization.

[MMeent]

I think this whole "optimization" is invalid, as it completely ignores LW_EXCLUSIVE / LW_SHARED conflicts.

With this, youre updating a shared hash table if it's locked, regardless of whether you're holding a read lock or write lock. That's just plain incorrect.

[knizhnik]

I think this whole "optimization" is invalid, as it completely ignores LW_EXCLUSIVE / LW_SHARED conflicts.

With this, youre updating a shared hash table if it's locked, regardless of whether you're holding a read lock or write lock. That's just plain incorrect.

Sorry, stupid error:(
Fixed.

[alexanderlaw]

By the way, isn't the comment:

But GetLastWrittenLSN(InvalidOid) is used only by zenith_dbsize which is not performance critical.

above GetLastWrittenLSN outdated?
(I can't find zenith_dbsize in the current code.)

[MMeent]

One bug, and some suggestions to address a lack of comments in the new code.

[knizhnik]

By the way, isn't the comment:

But GetLastWrittenLSN(InvalidOid) is used only by zenith_dbsize which is not performance critical.

above GetLastWrittenLSN outdated? (I can't find zenith_dbsize in the current code.)

It was renamed to neon_dbsize.
Will update comment.

[MMeent]

But GetLastWrittenLSN(InvalidOid) is used only by zenith_dbsize which is not performance critical.

Yeah, I think that's been renamed to neon_dbsize.

[MMeent]

Some nitpicks, but apart from that LGTM.