|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|
|Views:||Raw Message | Whole Thread | Download mbox|
> 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.
> - 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.
Attached is an update.
|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|