Re: [HACKERS] Unlogged tables cleanup

Hi,

On 2019-05-14 12:50:09 +0900, Michael Paquier wrote:
> On Mon, May 13, 2019 at 10:52:21AM -0700, Andres Freund wrote:
> > I was wrong here - I thought we WAL logged the main fork creation even
> > for unlogged tables. I think it's foolish that we don't, but we don't.
>
> Why? The main fork is not actually necessary, and the beginning of
> recovery would make sure that any existing main fork gets removed and
> that the main fork gets recreated by copying it from the init fork,
> which is WAL-logged.

Several things:

1) A main fork that's created while a base-backup is in progress might
only partially exist on a HS node. We won't necessarily remove it,
because the init fork might *not* have been copied. The init fork
will be re-created by WAL replay, but that'll be too late for the
ResetUnloggedRelations(UNLOGGED_RELATION_CLEANUP) call.

Which then will

a) cause ResetUnloggedRelations(UNLOGGED_RELATION_INIT) to fail, because the
target file already exists, as mentioned elsewhere in this thread

b) cause trouble when access the unlogged table - unfortunately there
are several ways to access unlogged tables on a HS node - so the
main fork better either not exist, or be in a valid empty
state. Case in point:
COPY blarg TO STDOUT;
does not throw an error due to unlogged-ness.

If we instead properly WAL logged the MAIN fork creation b) wouldn't
be a problem.

And we could have the UNLOGGED_RELATION_INIT case re-use the
hashtable built at UNLOGGED_RELATION_CLEANUP time, and avoid a second
scan of the filesystem. As all relations that were created after the
LSN crash recovery started at would be valid, we would not need to do
a full second scan: All unlogged relations created after that LSN are
guaranteed to have a consistent main fork.

2) With a small bit of additional work, the sketch to fix 1) would allow
us to allow read access to unlogged relations during HS. We'd just
have do the UNLOGGED_RELATION_INIT work at the start of recovery.

3) I'm somewhat certain that there are relfilenode reuse issues with the
current approach, again during base backups (as there the checkpoint
logic to prevent relfilenode reuse doesn't work). I think you can
easily end up with main / vm / fsm / init forks that actually don't
belong to the same relation for a base backup. If you don't force
delete all other forks at the time the init fork record is replayed,
there's no defense against that.

There's probably more, but I gotta cook.

> So we definitely have to log the init fork, but logging the main fork
> is just useless data in the WAL record. Or you have something else in
> mind?

Well, as I argued somewhere else in this thread, I think this all should
just be one WAL record.

Something very roughly like:

struct xl_smgr_create
{
RelFileNode rnode;
ForkNumber forkNum;
bool remove_all_other_forks;
bool copy_init_to_main_fork;
size_t data_length;
char fork_contents[];
};

Presumably with a flags argument instead of separate booleans.

Greetings,

Andres Freund