On 04/09/2023 16:59, Melanie Plageman wrote:
> The patch looks reasonable to me. Is this [1] case in hash index build
> that I reported but didn't take the time to reproduce similar?
>
> [1] https://www.postgresql.org/message-id/CAAKRu_bPc81M121pOEU7W%3D%2BwSWEebiLnrie4NpaFC%2BkWATFtSA%40mail.gmail.com
Yes, I think you're right. Any caller of smgrwrite() must either:
a) Call smgrimmedsync(), after smgrwrite() and before the relation is
visible to other transactions. Regardless of the 'skipFsync' parameter!
I don't think this is ever completely safe unless it's a new relation.
Like you pointed out with the hash index case. Or:
b) Hold the buffer lock, so that if a checkpoint happens, it cannot
"race past" the page without seeing the sync request.
The comment on smgwrite() doesn't make this clear. It talks about
'skipFsync', and gives the impression that as long as you pass
'skipFsync=false', you don't need to worry about fsyncing. But that is
not true. Even in these bulk loading cases where we currently call
smgrimmedsync(), we would *still* need to call smgrimmedsync() if we
used 'skipFsync=false'.
--
Heikki Linnakangas
Neon (https://neon.tech)