Re: [HACKERS] Unlogged tables cleanup

On Tue, May 14, 2019 at 2:19 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> How would this protect against the issues I mentioned where recovery
> starts from an earlier checkpoint and the basebackup could pick up a
> random set of version of the different forks?
>
> I don't like the idea of expanding the use of delayChkpt further for
> common operations, if anything we ought to try to reduce it. But I also
> don't see how it'd actually fix the issues, so perhaps that's moot.

Based on the discussion so far, I think there are a number of related
problems here:

1. A base backup might copy the main fork but not the init fork. I
think that's a problem that nobody's thought about before Andres
raised it on this thread. I may be wrong. Anyway, if that happens,
the start-of-recovery code isn't going to do the right thing, because
it won't know that it's dealing with an unlogged relation.

2. Suppose the system reaches the end of
heapam_relation_set_new_filenode and then the system crashes. Because
of the smgrimmedsync(), and only because of the smgrimmedsync(), the
init fork would exist at the start of recovery. However, unlike the
heap, some of the index AMs don't actually have a call to
smgrimmedsync() in their *buildempty() functions. If I'm not
mistaken, the ones that write pages through shared_buffers do not do
smgrimmedsync, and the ones that use private buffers do perform
smgrimmedsync. Therefore, the ones that write pages through
shared_buffers are vulnerable to a crash right after the unlogged
table is created. The init fork could fail to survive the crash, and
therefore the start-of-recovery code would again fail to know that
it's dealign with an unlogged relation.

3. So, why is it like that, anyway? Why do index AMs that write pages
via shared_buffers not have smgrimmedsync? The answer is that we did
it like that because we were worrying about a different problem -
specifically, checkpointing. If I dirty a buffer and write a WAL
record for the changes that I made, it is guaranteed that the dirty
data will make it to disk before the next checkpoint is written; we've
got all sorts of interlocking in BufferSync, SyncOneBuffer, etc. to
make sure that happens. But if I create a file in the filesystem and
write a WAL record for that change, none of that interlocking does
anything. So unless I not only create that file but smgrimmedsync()
it before the next checkpoint's redo location is fixed, a crash could
make the file disappear.

For example, consider RelationCreateStorage. What prevents the
following sequence of events?

S1: sgmropen()
S1: smgrcreate()
S1: log_smgrcreate()
-- at this point the checkpointer fixes the redo pointer, writes every
dirty buffer in the system, and completes a checkpoint
S1: commits
-- crash

On restart, I think we'll could potentially be missing the file
created by smgrcreate(), and we won't replay the WAL record generated
by log_smgrcreate() either because it's before the checkpoint.

I believe that it is one aspect of this third problem that we
previously fixed on this thread, but I think we failed to understand
how general the issue actually is. It affects _init forks of unlogged
tables, but I think it also affects essentially every other use of
smgrcreate(), whether it's got anything to do with unlogged tables or
not. For an index AM, which has a non-empty initial representation,
the consequences are pretty limited, because the transaction can't
commit having created only an empty fork. It's got to put some data
into either the main fork or the init fork, as the case may be. If it
aborts, then we may leave some orphaned files behind, but if we lose
one, we just have fewer orphaned files, so nobody cares. And if it
commits, then either everything's before the checkpoint (and the file
will have been fsync'd because we fsync'd the buffers), or
everything's after the checkpoint (so we will replay the WAL), or only
the smgrcreate() is before the checkpoint and the rest is after (in
which case XLogReadBufferExtended will do the smgrcreate over again
for us and we'll be fine). But since the heap uses an empty initial
representation, it enjoys no similar protection.

So, IIUC, the reason why we were talking about delayCkpt before is
because it would allow us to remove the smgrimmedsync() of the
unlogged fork while still avoiding problem #3. However it does
nothing to protect against problem #1 or #2.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company