From: | Amul Sul <sulamul(at)gmail(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Dilip Kumar <dilipbalaut(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, Ibrar Ahmed <ibrar(dot)ahmad(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Prabhat Sahu <prabhat(dot)sahu(at)enterprisedb(dot)com> |
Subject: | Re: [Patch] ALTER SYSTEM READ ONLY |
Date: | 2021-08-04 12:56:30 |
Message-ID: | CAAJ_b95AWmPSUK0u7ZPyooyODPNM35ntHoU7rBfuXY-L9e1TQQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Attached is the rebase version on top of the latest master head
includes refactoring patches posted by Robert.
On Thu, Jul 29, 2021 at 9:46 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Wed, Jul 28, 2021 at 7:33 AM Amul Sul <sulamul(at)gmail(dot)com> wrote:
> > I was too worried about how I could miss that & after thinking more
> > about that, I realized that the operation for ArchiveRecoveryRequested
> > is never going to be skipped in the startup process and that never
> > left for the checkpoint process to do that later. That is the reason
> > that assert was added there.
> >
> > When ArchiveRecoveryRequested, the server will no longer be in
> > the wal prohibited mode, we implicitly change the state to
> > wal-permitted. Here is the snip from the 0003 patch:
>
> Ugh, OK. That makes sense, but I'm still not sure that I like it. I've
> kind of been wondering: why not have XLogAcceptWrites() be the
> responsibility of the checkpointer all the time, in every case? That
> would require fixing some more things, and this is one of them, but
> then it would be consistent, which means that any bugs would be likely
> to get found and fixed. If calling XLogAcceptWrites() from the
> checkpointer is some funny case that only happens when the system
> crashes while WAL is prohibited, then we might fail to notice that we
> have a bug.
>
Unfortunately, I didn't get much time to think about this and don't
have a strong opinion on it either.
> This is especially true given that we have very little test coverage
> in this area. Andres was ranting to me about this earlier this week,
> and I wasn't sure he was right, but then I noticed that we have
> exactly zero tests in the entire source tree that make use of
> recovery_end_command. We really need a TAP test for that, I think.
> It's too scary to do much reorganization of the code without having
> any tests at all for the stuff we're moving around. Likewise, we're
> going to need TAP tests for the stuff that is specific to this patch.
> For example, we should have a test that crashes the server while it's
> read only, brings it back up, checks that we still can't write WAL,
> then re-enables WAL, and checks that we now can write WAL. There are
> probably a bunch of other things that we should test, too.
>
Yes, my next plan is to work on the TAP tests and look into the patch
posted by Prabhat to improve test coverage.
Regards,
Amul Sul
Attachment | Content-Type | Size |
---|---|---|
v31-0007-Documentation.patch | application/octet-stream | 10.4 KB |
v31-0006-Error-or-Assert-before-START_CRIT_SECTION-for-WA.patch | application/octet-stream | 68.4 KB |
v31-0005-Implement-wal-prohibit-state-using-global-barrie.patch | application/octet-stream | 52.5 KB |
v31-0003-Create-XLogAcceptWrites-function-with-code-from-.patch | application/octet-stream | 3.8 KB |
v31-0004-Refactor-add-function-to-set-database-state-in-c.patch | application/octet-stream | 3.1 KB |
v31-0002-Postpone-some-end-of-recovery-operations-relatin.patch | application/octet-stream | 3.3 KB |
v31-0001-Refactor-some-end-of-recovery-code-out-of-Startu.patch | application/octet-stream | 14.2 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Kapila | 2021-08-04 12:58:38 | Re: Failed transaction statistics to measure the logical replication progress |
Previous Message | houzj.fnst@fujitsu.com | 2021-08-04 12:19:54 | RE: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION |