Re: Offline enabling/disabling of data checksums

From: Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Michael Banck <michael(dot)banck(at)credativ(dot)de>, Postgres hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Offline enabling/disabling of data checksums
Date: 2019-03-17 11:44:39
Message-ID: alpine.DEB.2.21.1903171119120.2506@lancre
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: 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 Content-Type Size
controlfile-update-2.patch text/x-diff 8.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Dean Rasheed 2019-03-17 11:47:47 Re: [HACKERS] PATCH: multivariate histograms and MCV lists
Previous Message David Rowley 2019-03-17 11:40:52 Re: using index or check in ALTER TABLE SET NOT NULL