Re: Offline enabling/disabling of data checksums - Mailing list pgsql-hackers

From Fabien COELHO
Subject Re: Offline enabling/disabling of data checksums
Date
Msg-id alpine.DEB.2.21.1903171119120.2506@lancre
Whole thread Raw
In response to Re: Offline enabling/disabling of data checksums  (Michael Paquier <michael@paquier.xyz>)
Responses Re: Offline enabling/disabling of data checksums
Re: Offline enabling/disabling of data checksums
List pgsql-hackers
Michaël-san,

> The issue here is that trying to embed directly the fsync routines
> from file_utils.c into pg_resetwal.c messes up the inclusions because
> pg_resetwal.c includes backend-side includes, which themselves touch
> fd.h :(
>
> In short your approach avoids some extra mess with the include
> dependencies. .

I could remove the two "catalog/" includes from pg_resetwal, I assume that 
you meant these ones.

>> Maybe the two changes could be committed separately.
>
> I was thinking about this one, and for pg_rewind we don't care about
> the fsync of the control file because the full data folder gets
> fsync'd afterwards and in the event of a crash in the middle of a
> rewind the target data folder is surely not something to use, but we
> do for pg_checksums, and we do for pg_resetwal.  Even if there is the
> argument that usually callers of update_controlfile() would care a
> lot about the control file and fsync it, I think that we need some
> control on if we do the fsync or not because many tools have a
> --no-sync and that should be fully respected.

> So while your patch is on a good track, I would suggest to do the 
> following things to complete it:

> - Add an extra argument bits16 to update_controlfile to pass a set of 
> optional flags, with NOSYNC being the only and current value.  The 
> default is to flush the file.

Hmmm. I just did that, but what about just a boolean? What other options 
could be required? Maybe some locking/checking?

> - Move the wait event calls WAIT_EVENT_CONTROL_FILE_WRITE_UPDATE and
> WAIT_EVENT_CONTROL_FILE_SYNC_UPDATE to controldata_utils.c.

Done.

> - And then delete UpdateControlFile() in xlog.c, and use
> update_controlfile() instead to remove even more code.

I was keeping that one for another patch because it touches the backend 
code, but it makes sense to do that in one go for consistency.

I kept the initial no-parameter function which calls the new one with 4 
parameters, though, because it looks more homogeneous this way in the 
backend code. This is debatable.

> The version in xlog.c uses BasicOpenFile(), so we should use also that 
> in update_controlfile() instead of OpenTransientFile(). As any errors 
> result in a PANIC we don't care about leaking fds.

Done.

Attached is an update.

-- 
Fabien.
Attachment

pgsql-hackers by date:

Previous
From: David Rowley
Date:
Subject: Re: using index or check in ALTER TABLE SET NOT NULL
Next
From: Dean Rasheed
Date:
Subject: Re: [HACKERS] PATCH: multivariate histograms and MCV lists