Re: [Patch] ALTER SYSTEM READ ONLY

On Tue, Oct 19, 2021 at 3:50 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Mon, Oct 18, 2021 at 9:54 AM Amul Sul <sulamul(at)gmail(dot)com> wrote:
> > I tried this in the attached version, but I'm a bit skeptical with
> > changes that are needed for CreateCheckPoint(), those don't seem to be
> > clean.
>
> Yeah, that doesn't look great. I don't think it's entirely correct,
> actually, because surely you want LocalXLogInsertAllowed = 0 to be
> executed even if !IsPostmasterEnvironment. It's only
> LocalXLogInsertAllowed = -1 that we would want to have depend on
> IsPostmasterEnvironment. But that's pretty ugly too: I guess the
> reason it has to be like is that, if it does that unconditionally, it
> will overwrite the temporary value of 1 set by the caller, which will
> then cause problems when the caller tries to XLogReportParameters().
>
> I think that problem goes away if we drive the decision off of shared
> state rather than a local variable, but I agree that it's otherwise a
> bit tricky to untangle. One idea might be to have
> LocalSetXLogInsertAllowed return the old value. Then we could use the
> same kind of coding we do when switching memory contexts, where we
> say:
>
> oldcontext = MemoryContextSwitchTo(something);
> // do stuff
> MemoryContextSwitchTo(oldcontext);
>
> Here we could maybe do:
>
> oldxlallowed = LocalSetXLogInsertAllowed();
> // do stuff
> XLogInsertAllowed = oldxlallowed;
>

Ok, did the same in the attached 0001 patch.

There is no harm in calling LocalSetXLogInsertAllowed() calling
multiple times, but the problem I can see is that with this patch user
is allowed to call LocalSetXLogInsertAllowed() at the time it is
supposed not to be called e.g. when LocalXLogInsertAllowed = 0;
WAL writes are explicitly disabled.

> That way, instead of CreateCheckPoint() knowing under what
> circumstances the caller might have changed the value, it only knows
> that some callers might have already changed the value. That seems
> better.
>
> > I agree that calling InitXLOGAccess() from RecoveryInProgress() is not
> > good, but I am not sure about calling it from XLogInsertAllowed()
> > either, perhaps, both are status check function and general
> > expectations might be that status checking functions are not going
> > change and/or initialize the system state. InitXLOGAccess() should
> > get called from the very first WAL write operation if needed, but if
> > we don't want to do that, then I would prefer to call InitXLOGAccess()
> > from XLogInsertAllowed() instead of RecoveryInProgress().
>
> Well, that's a fair point, too, but it might not be safe to, say, move
> this to XLogBeginInsert(). Like, imagine that there's a hypothetical
> piece of code that looks like this:
>
> if (RecoveryInProgress())
> ereport(ERROR, errmsg("can't do that in recovery")));
>
> // do something here that depends on ThisTimeLineID or
> wal_segment_size or RedoRecPtr
>
> XLogBeginInsert();
> ....
> lsn = XLogInsert(...);
>
> Such code would work correctly the way things are today, but if the
> InitXLOGAccess() call were deferred until XLogBeginInsert() time, then
> it would fail.
>
> I was curious whether this is just a theoretical problem. It turns out
> that it's not. I wrote a couple of just-for-testing patches, which I
> attach here. The first one just adjusts things so that we'll fail an
> assertion if we try to make use of ThisTimeLineID before we've set it
> to a legal value. I had to exempt two places from these checks just
> for 'make check-world' to pass; these are shown in the patch, and one
> or both of them might be existing bugs -- or maybe not, I haven't
> looked too deeply. The second one then adjusts the patch to pretend
> that ThisTimeLineID is not necessarily valid just because we've called
> InitXLOGAccess() but that it is valid after XLogBeginInsert(). With
> that change, I find about a dozen places where, apparently, the early
> call to InitXLOGAccess() is critical to getting ThisTimeLineID
> adjusted in time. So apparently a change of this type is not entirely
> trivial. And this is just a quick test, and just for one of the three
> things that get initialized here.
>
> On the other hand, just moving it to XLogInsertAllowed() isn't
> risk-free either and would likely require adjusting some of the same
> places I found with this test. So I guess if we want to do something
> like this we need more study.
>

Yeah, that requires a lot of energy and time -- not done anything
related to this in the attached version.

Please have a look at the attached version where the 0001 patch does
change LocalSetXLogInsertAllowed() as said before. 0002 patch moves
XLogReportParameters() closer to other wal write operations and
removes unnecessary LocalSetXLogInsertAllowed() calls. 0003 is code
movements adds XLogAcceptWrites() function same as the before, and
0004 patch tries to remove the dependency. 0004 patch could change
w.r.t. decision that is going to be made for the patch that I
posted[1] to remove abortedRecPtr global variable. For now, I have
copied abortedRecPtr into shared memory. Thanks !

1] https://postgr.es/m/CAAJ_b94Y75ZwMim+gxxexVwf_yzO-dChof90ky0dB2GstspNjA@mail.gmail.com

Regards,
Amul