Re: Unlogged relation copy is not fsync'd

On 05/09/2023 21:20, Robert Haas wrote:
> In other words, somehow it feels like we ought to be trying to defer
> the fsync here until a clean shutdown actually occurs, instead of
> performing it immediately.

+1

> Admittedly, the bookkeeping seems like a problem, so maybe this is
> the best we can do, but it's clearly worse than what we do in other
> cases.
I think we can do it, I have been working on a patch along those lines 
on the side. But I want to focus on a smaller, backpatchable fix in this 
thread.


Thinking about this some more, I think this is still not 100% correct, 
even with the patch I posted earlier:

>     /*
>      * When we WAL-logged rel pages, we must nonetheless fsync them.  The
>      * reason is that since we're copying outside shared buffers, a CHECKPOINT
>      * occurring during the copy has no way to flush the previously written
>      * data to disk (indeed it won't know the new rel even exists).  A crash
>      * later on would replay WAL from the checkpoint, therefore it wouldn't
>      * replay our earlier WAL entries. If we do not fsync those pages here,
>      * they might still not be on disk when the crash occurs.
>      */
>     if (use_wal || relpersistence == RELPERSISTENCE_UNLOGGED)
>         smgrimmedsync(dst, forkNum);

Let's walk through each case one by one:

1. Temporary table. No fsync() needed. This is correct.

2. Unlogged rel, main fork. Needs to be fsync'd, because we skipped WAL, 
and also because we bypassed the buffer manager. Correct.

3. Unlogged rel, init fork. Needs to be fsync'd, even though we 
WAL-logged it, because we bypassed the buffer manager. Like the comment 
explains. This is now correct with the patch.

4. Permanent rel, use_wal == true. Needs to be fsync'd, even though we 
WAL-logged it, because we bypassed the buffer manager. Like the comment 
explains. Correct.

5. Permanent rel, use_wal == false. We skip fsync, because it means that 
it's a new relation, so we have a sync request pending for it. (An 
assertion for that would be nice). At the end of transaction, in 
smgrDoPendingSyncs(), we will either fsync it or we will WAL-log all the 
pages if it was a small relation. I believe this is *wrong*. If 
smgrDoPendingSyncs() decides to WAL-log the pages, we have the same race 
condition that's explained in the comment, because we bypassed the 
buffer manager:

1. RelationCopyStorage() copies some of the pages.
2. Checkpoint happens, which fsyncs the relation (smgrcreate() 
registered a dirty segment when the relation was created)
3. RelationCopyStorage() copies the rest of the pages.
4. smgrDoPendingSyncs() WAL-logs all the pages.
5. Another checkpoint happens. It does *not* fsync the relation.
6. Crash.

WAL replay will not see the WAL-logged pages, because they were 
WAL-logged before the last checkpoint. And the contents were not fsync'd 
either.

In other words, we must do smgrimmedsync() here for permanent relations, 
even if use_wal==false, because we bypassed the buffer manager. Same 
reason we need to do it with use_wal==true.

For a backportable fix, I think we should change this to only exempt 
temporary tables, and call smgrimmedsync() for all other cases. Maybe 
would be safe to skip it in some cases, but it feels too dangerous to be 
clever here. The other similar callers of smgrimmedsync() in nbtsort.c, 
gistbuild.c, and rewriteheap.c, need similar treatment.

-- 
Heikki Linnakangas
Neon (https://neon.tech)