Hello, Laurenz
On Thursday, Nov19, 2020 4:50 PM Laurenz Albe <laurenz.a...@cybertec.at> wrote > On Thu, 2020-11-19 at 05:24 +0000, osumi.takami...@fujitsu.com wrote: > > > > > ereport(WARNING, > > > > > (errmsg("WAL was generated with wal_level=minimal, data > > > > > may be missing"), > > > > > errhint("This happens if you temporarily set > > > > > wal_level=minimal without taking a new base backup."))); There's > > > > > definitely a question about if a WARNING there is really > > > > > sufficient or not, considering that you could end up with 'logged' > > > > > tables on the replica that are missing data, but I'm not sure > > > > > that inventing a new, independent, mechanism for checking WAL > > > > > level changes makes > > > > sense. > > > > > > I don't know why WARNING was chosen. I think it should be FATAL, > > > resulting in the standby shutdown, disabling restarting it, and > > > urging the user to rebuild the standby. (I guess that's > > > overreaction because the user may not perform operations that lack > > > WAL while wal_level is minimal.) > > > > Yeah, I agree that WARNING is not sufficient. > > I missed that this is only a warning when I looked at it before. > Yes, it should be a fatal error. > > I think that there should two patches: one that turns this warning into a > FATAL and should be backpatched. If you change the test to > > ControlFile->wal_level <= WAL_LEVEL_MINIMAL > > it will automatically work for your new feature too. > Then your new wal_level would be a second patch only for HEAD. Yeah, this suggestion to divide the patch into two sounds really good. Thank you. I'll post separated patches next time. > > With that, the only remaining consideration with this patch is the danger that > enabling wal_level=none without taking a backup before can lead to data loss. > But that is intended, so I think that an unmistakable warning in the > documentation would be good enough. Yes, thank you for reviewing the documents in the patch. Best, Takamichi Osumi