Horton

Thread

Commits

  1. Revert "Add new simple TAP test for tablespaces."

  2. Remove unnecessary call to ReadCheckpointRecord().

  1. needless complexity in StartupXLOG

    Robert Haas <robertmhaas@gmail.com> — 2021-07-26T16:12:53Z

    StartupXLOG() has code beginning around line 7900 of xlog.c that
decides, at the end of recovery, between four possible courses of
action. It either writes an end-of-recovery record, or requests a
checkpoint, or creates a checkpoint, or does nothing, depending on the
value of 3 flag variables, and also on whether we're still able to
read the last checkpoint record:

                checkPointLoc = ControlFile->checkPoint;

                /*
                 * Confirm the last checkpoint is available for us to recover
                 * from if we fail.
                 */
                record = ReadCheckpointRecord(xlogreader,
checkPointLoc, 1, false);
                if (record != NULL)
                {
                    promoted = true;

It seems to me that ReadCheckpointRecord() should never fail here. It
should always be true, even when we're not in recovery, that the last
checkpoint record is readable. If there's ever a situation where that
is not true, even for an instant, then a crash at that point will be
unrecoverable. Now, one way that it could happen is if we've got a bug
in the code someplace that removes WAL segments too soon. However, if
we have such a bug, we should fix it. What this code does is says "oh,
I guess we removed the old checkpoint too soon, no big deal, let's
just be more aggressive about getting the next one done," which I do
not think is the right response. Aside from a bug, the only other way
I can see it happening is if someone is manually removing WAL segments
as the server is running through recovery, perhaps as some last-ditch
play to avoid running out of disk space. I don't think the server
needs to have - or should have - code to cater to such crazy
scenarios. Therefore I think that this check, at least in its current
form, is not sensible.

My first thought was that we should do the check unconditionally,
rather than just when bgwriterLaunched && LocalPromoteIsTriggered, and
ERROR if it fails. But then I wondered what the point of that would be
exactly. If we have such a bug -- and to the best of my knowledge
there's no evidence that we do -- there's no particular reason it
should only happen at the end of recovery. It could happen any time
the system -- or the user, or malicious space aliens -- remove files
from pg_wal, and we have no real idea about the timing of malicious
space alien activity, so doing the test here rather than anywhere else
just seems like a shot in the dark. Perhaps the most logical place
would be to move it to CreateCheckPoint() just after we remove old
xlog files, but we don't have the xlogreader there, and anyway I don't
see how it's really going to help. What bug do we expect to catch by
removing files we think we don't need and then checking that we didn't
remove the files we think we do need? That seems more like grasping at
straws than a serious attempt to make things work better.

So at the moment I am leaning toward the view that we should just
remove this check entirely, as in the attached, proposed patch.

Really, I think we should consider going further. If it's safe to
write an end-of-recovery record rather than a checkpoint, why not do
so all the time? Right now we fail to do that in the above-described
"impossible" scenario where the previous checkpoint record can't be
read, or if we're exiting archive recovery for some reason other than
a promotion request, or if we're in single-user mode, or if we're in
crash recovery. Presumably, people would like to start up the server
quickly in all of those scenarios, so the only reason not to use this
technology all the time is if we think it's safe in some scenarios and
not others. I can't think of a reason why it matters why we're exiting
archive recovery, nor can I think of a reason why it matters whether
we're in single user mode. The distinction between crash recovery and
archive recovery does seem to matter, but if anything the crash
recovery scenario seems simpler, because then there's only one
timeline involved.

I realize that conservatism may have played a role in this code ending
up looking the way that it does; someone seems to have thought it
would be better not to rely on a new idea in all cases. From my point
of view, though, it's scary to have so many cases, especially cases
that don't seem like they should ever be reached. I think that
simplifying the logic here and trying to do the same things in as many
cases as we can will lead to better robustness. Imagine if instead of
all the hairy logic we have now we just replaced this whole if
(IsInRecovery) stanza with this:

if (InRecovery)
    CreateEndOfRecoveryRecord();

That would be WAY easier to reason about than the rat's nest we have
here today. Now, I am not sure what it would take to get there, but I
think that is the direction we ought to be heading.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  2. Re: needless complexity in StartupXLOG

    Stephen Frost <sfrost@snowman.net> — 2021-07-26T17:32:31Z

    Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> So at the moment I am leaning toward the view that we should just
> remove this check entirely, as in the attached, proposed patch.

Haven't dug in deeply but at least following your explanation and
reading over the patch and the code a bit, I tend to agree.

> Really, I think we should consider going further. If it's safe to
> write an end-of-recovery record rather than a checkpoint, why not do
> so all the time? Right now we fail to do that in the above-described
> "impossible" scenario where the previous checkpoint record can't be
> read, or if we're exiting archive recovery for some reason other than
> a promotion request, or if we're in single-user mode, or if we're in
> crash recovery. Presumably, people would like to start up the server
> quickly in all of those scenarios, so the only reason not to use this
> technology all the time is if we think it's safe in some scenarios and
> not others. I can't think of a reason why it matters why we're exiting
> archive recovery, nor can I think of a reason why it matters whether
> we're in single user mode. The distinction between crash recovery and
> archive recovery does seem to matter, but if anything the crash
> recovery scenario seems simpler, because then there's only one
> timeline involved.

Yeah, tend to agree with this too ... but something I find a bit curious
is the comment:

* Insert a special WAL record to mark the end of
* recovery, since we aren't doing a checkpoint.

... immediately after setting promoted = true, and then at the end of
StartupXLOG() having:

if (promoted)
	RequestCheckpoint(CHECKPOINT_FORCE);

maybe I'm missing something, but seems like that comment isn't being
terribly clear.  Perhaps we aren't doing a full checkpoint *there*, but
sure looks like we're going to do one moments later regardless of
anything else since we've set promoted to true..?

> I realize that conservatism may have played a role in this code ending
> up looking the way that it does; someone seems to have thought it
> would be better not to rely on a new idea in all cases. From my point
> of view, though, it's scary to have so many cases, especially cases
> that don't seem like they should ever be reached. I think that
> simplifying the logic here and trying to do the same things in as many
> cases as we can will lead to better robustness. Imagine if instead of
> all the hairy logic we have now we just replaced this whole if
> (IsInRecovery) stanza with this:
> 
> if (InRecovery)
>     CreateEndOfRecoveryRecord();
> 
> That would be WAY easier to reason about than the rat's nest we have
> here today. Now, I am not sure what it would take to get there, but I
> think that is the direction we ought to be heading.

Agreed that simpler logic is better, provided it's correct logic, of
course.  Finding better ways to test all of this would be really nice.

Thanks!

Stephen

  3. Re: needless complexity in StartupXLOG

    Robert Haas <robertmhaas@gmail.com> — 2021-07-26T19:53:20Z

    On Mon, Jul 26, 2021 at 1:32 PM Stephen Frost <sfrost@snowman.net> wrote:
> Yeah, tend to agree with this too ... but something I find a bit curious
> is the comment:
>
> * Insert a special WAL record to mark the end of
> * recovery, since we aren't doing a checkpoint.
>
> ... immediately after setting promoted = true, and then at the end of
> StartupXLOG() having:
>
> if (promoted)
>         RequestCheckpoint(CHECKPOINT_FORCE);
>
> maybe I'm missing something, but seems like that comment isn't being
> terribly clear.  Perhaps we aren't doing a full checkpoint *there*, but
> sure looks like we're going to do one moments later regardless of
> anything else since we've set promoted to true..?

Yep. So it's a question of whether we allow operations that might
write WAL in the meantime. When we write the checkpoint record right
here, there can't be any WAL from the new server lifetime until the
checkpoint completes. When we write an end-of-recovery record, there
can. And there could actually be quite a bit, because if we do the
checkpoint right in this section of code, it will be a fast
checkpoint, whereas in the code you quoted above, it's a spread
checkpoint, which takes a lot longer. So the question is whether it's
reasonable to give the checkpoint some time to complete or whether it
needs to be completed right now.

> Agreed that simpler logic is better, provided it's correct logic, of
> course.  Finding better ways to test all of this would be really nice.

Yeah, and there again, it's a lot easier to test if we don't have as
many cases. Now no single change is going to fix that, but the number
of flag variables in xlog.c is simply bonkers. This particular stretch
of code uses 3 of them to even decide whether to attempt the test in
question, and all of those are set in complex ways depending on the
values of still more flag variables. The comments where
bgwriterLaunched is set claim that we only do this during archive
recovery, not crash recovery, but the code depends on the value of
ArchiveRecoveryRequested, not InArchiveRecovery. So I wonder if we
can't get the bgwriter to run even during crash recovery in the
scenario described by:

         * It's possible that archive recovery was requested, but we don't
         * know how far we need to replay the WAL before we reach consistency.
         * This can happen for example if a base backup is taken from a
         * running server using an atomic filesystem snapshot, without calling
         * pg_start/stop_backup. Or if you just kill a running primary server
         * and put it into archive recovery by creating a recovery signal
         * file.

If we ran the bgwriter all the time during crash recovery, we'd know
for sure whether that causes any problems. If we only do it like this
in certain corner cases, it's much more likely that we have bugs.
Grumble, grumble.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  4. Re: needless complexity in StartupXLOG

    Stephen Frost <sfrost@snowman.net> — 2021-07-26T20:15:23Z

    Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Mon, Jul 26, 2021 at 1:32 PM Stephen Frost <sfrost@snowman.net> wrote:
> > Yeah, tend to agree with this too ... but something I find a bit curious
> > is the comment:
> >
> > * Insert a special WAL record to mark the end of
> > * recovery, since we aren't doing a checkpoint.
> >
> > ... immediately after setting promoted = true, and then at the end of
> > StartupXLOG() having:
> >
> > if (promoted)
> >         RequestCheckpoint(CHECKPOINT_FORCE);
> >
> > maybe I'm missing something, but seems like that comment isn't being
> > terribly clear.  Perhaps we aren't doing a full checkpoint *there*, but
> > sure looks like we're going to do one moments later regardless of
> > anything else since we've set promoted to true..?
> 
> Yep. So it's a question of whether we allow operations that might
> write WAL in the meantime. When we write the checkpoint record right
> here, there can't be any WAL from the new server lifetime until the
> checkpoint completes. When we write an end-of-recovery record, there
> can. And there could actually be quite a bit, because if we do the
> checkpoint right in this section of code, it will be a fast
> checkpoint, whereas in the code you quoted above, it's a spread
> checkpoint, which takes a lot longer. So the question is whether it's
> reasonable to give the checkpoint some time to complete or whether it
> needs to be completed right now.

All I was really trying to point out above was that the comment might be
improved upon, just so someone understands that we aren't doing a
checkpoint at this particular place, but one will be done later due to
the promotion.  Maybe I'm being a bit extra with that, but that was my
thought when reading the code and the use of the promoted flag variable.

> > Agreed that simpler logic is better, provided it's correct logic, of
> > course.  Finding better ways to test all of this would be really nice.
> 
> Yeah, and there again, it's a lot easier to test if we don't have as
> many cases. Now no single change is going to fix that, but the number
> of flag variables in xlog.c is simply bonkers. This particular stretch
> of code uses 3 of them to even decide whether to attempt the test in
> question, and all of those are set in complex ways depending on the
> values of still more flag variables. The comments where
> bgwriterLaunched is set claim that we only do this during archive
> recovery, not crash recovery, but the code depends on the value of
> ArchiveRecoveryRequested, not InArchiveRecovery. So I wonder if we
> can't get the bgwriter to run even during crash recovery in the
> scenario described by:
> 
>          * It's possible that archive recovery was requested, but we don't
>          * know how far we need to replay the WAL before we reach consistency.
>          * This can happen for example if a base backup is taken from a
>          * running server using an atomic filesystem snapshot, without calling
>          * pg_start/stop_backup. Or if you just kill a running primary server
>          * and put it into archive recovery by creating a recovery signal
>          * file.
> 
> If we ran the bgwriter all the time during crash recovery, we'd know
> for sure whether that causes any problems. If we only do it like this
> in certain corner cases, it's much more likely that we have bugs.
> Grumble, grumble.

Yeah ... not to mention that it really is just incredibly dangerous to
use such an approach for PITR.  For my 2c, I'd rather we figure out a
way to prevent this than to imply that we support it when we have no way
of knowing if we actually have replayed far enough to be consistent.
That isn't to say that using snapshots for database backups isn't
possible, but it should be done in-between pg_start/stop_backup calls
which properly grab the returned info from those and store the backup
label with the snapshot, etc.

Thanks,

Stephen

  5. Re: needless complexity in StartupXLOG

    Justin Pryzby <pryzby@telsasoft.com> — 2021-07-27T00:36:52Z

    On Mon, Jul 26, 2021 at 03:53:20PM -0400, Robert Haas wrote:
> Yeah, and there again, it's a lot easier to test if we don't have as
> many cases. Now no single change is going to fix that, but the number
> of flag variables in xlog.c is simply bonkers. This particular stretch
> of code uses 3 of them to even decide whether to attempt the test in
> question, and all of those are set in complex ways depending on the
> values of still more flag variables. The comments where
> bgwriterLaunched is set claim that we only do this during archive
> recovery, not crash recovery, but the code depends on the value of
> ArchiveRecoveryRequested, not InArchiveRecovery. So I wonder if we
> can't get the bgwriter to run even during crash recovery in the
> scenario described by:

I'm not following along closely and maybe you're already aware of this one?
https://commitfest.postgresql.org/33/2706/
Background writer and checkpointer in crash recovery

@Thomas:
https://www.postgresql.org/message-id/CA%2BTgmoYmw%3D%3DTOJ6EzYb_vcjyS09NkzrVKSyBKUUyo1zBEaJASA%40mail.gmail.com
>          * It's possible that archive recovery was requested, but we don't
>          * know how far we need to replay the WAL before we reach consistency.
>          * This can happen for example if a base backup is taken from a
>          * running server using an atomic filesystem snapshot, without calling
>          * pg_start/stop_backup. Or if you just kill a running primary server
>          * and put it into archive recovery by creating a recovery signal
>          * file.
> 
> If we ran the bgwriter all the time during crash recovery, we'd know
> for sure whether that causes any problems. If we only do it like this
> in certain corner cases, it's much more likely that we have bugs.
> Grumble, grumble.

-- 
Justin

  6. Re: needless complexity in StartupXLOG

    Robert Haas <robertmhaas@gmail.com> — 2021-07-27T12:23:15Z

    On Mon, Jul 26, 2021 at 4:15 PM Stephen Frost <sfrost@snowman.net> wrote:
> All I was really trying to point out above was that the comment might be
> improved upon, just so someone understands that we aren't doing a
> checkpoint at this particular place, but one will be done later due to
> the promotion.  Maybe I'm being a bit extra with that, but that was my
> thought when reading the code and the use of the promoted flag variable.

Yeah, I agree, it confused me too, at first.

> Yeah ... not to mention that it really is just incredibly dangerous to
> use such an approach for PITR.  For my 2c, I'd rather we figure out a
> way to prevent this than to imply that we support it when we have no way
> of knowing if we actually have replayed far enough to be consistent.
> That isn't to say that using snapshots for database backups isn't
> possible, but it should be done in-between pg_start/stop_backup calls
> which properly grab the returned info from those and store the backup
> label with the snapshot, etc.

My position on that is that I would not particularly recommend the
technique described here, but I would not choose to try to block it
either. That's an argument for another thread, though.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  7. Re: needless complexity in StartupXLOG

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-07-27T13:17:08Z

    At Mon, 26 Jul 2021 16:15:23 -0400, Stephen Frost <sfrost@snowman.net> wrote in 
> Greetings,
> 
> * Robert Haas (robertmhaas@gmail.com) wrote:
> > On Mon, Jul 26, 2021 at 1:32 PM Stephen Frost <sfrost@snowman.net> wrote:
> > > Yeah, tend to agree with this too ... but something I find a bit curious
> > > is the comment:
> > >
> > > * Insert a special WAL record to mark the end of
> > > * recovery, since we aren't doing a checkpoint.
> > >
> > > ... immediately after setting promoted = true, and then at the end of
> > > StartupXLOG() having:
> > >
> > > if (promoted)
> > >         RequestCheckpoint(CHECKPOINT_FORCE);
> > >
> > > maybe I'm missing something, but seems like that comment isn't being
> > > terribly clear.  Perhaps we aren't doing a full checkpoint *there*, but
> > > sure looks like we're going to do one moments later regardless of
> > > anything else since we've set promoted to true..?
> > 
> > Yep. So it's a question of whether we allow operations that might
> > write WAL in the meantime. When we write the checkpoint record right
> > here, there can't be any WAL from the new server lifetime until the
> > checkpoint completes. When we write an end-of-recovery record, there
> > can. And there could actually be quite a bit, because if we do the
> > checkpoint right in this section of code, it will be a fast
> > checkpoint, whereas in the code you quoted above, it's a spread
> > checkpoint, which takes a lot longer. So the question is whether it's
> > reasonable to give the checkpoint some time to complete or whether it
> > needs to be completed right now.
> 
> All I was really trying to point out above was that the comment might be
> improved upon, just so someone understands that we aren't doing a
> checkpoint at this particular place, but one will be done later due to
> the promotion.  Maybe I'm being a bit extra with that, but that was my
> thought when reading the code and the use of the promoted flag variable.

(I feel we don't need to check for last-checkpoint, too.)

FWIW, by the way, I complained that the variable name "promoted" is a
bit confusing.  It's old name was fast_promoted, which means that fast
promotion is being *requsted* and ongoing.  On the other hand the
current name "promoted" still means "(fast=non-fallback) promotion is
ongoing" so there was a conversation as the follows.

https://www.postgresql.org/message-id/9fdd994d-a531-a52b-7906-e1cc22701310%40oss.nttdata.com

>> How about changing it to fallback_promotion, or some names with more
>> behavior-specific name like immediate_checkpoint_needed?
> 
> I like doEndOfRecoveryCkpt or something, but I have no strong opinion
> about that flag naming. So I'm ok with immediate_checkpoint_needed
> if others also like that, too.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  8. Re: needless complexity in StartupXLOG

    Robert Haas <robertmhaas@gmail.com> — 2021-07-27T15:03:14Z

    On Tue, Jul 27, 2021 at 9:18 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> FWIW, by the way, I complained that the variable name "promoted" is a
> bit confusing.  It's old name was fast_promoted, which means that fast
> promotion is being *requsted* and ongoing.  On the other hand the
> current name "promoted" still means "(fast=non-fallback) promotion is
> ongoing" so there was a conversation as the follows.
>
> https://www.postgresql.org/message-id/9fdd994d-a531-a52b-7906-e1cc22701310%40oss.nttdata.com

I agree - that variable name is also not great. I am open to making
improvements in that area and in others that have been suggested on
this thread, but my immediate goal is to figure out whether anyone
objects to me committing the posted patch. If nobody comes up with a
reason why it's a bad idea in the next few days, I'll plan to move
ahead with it.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  9. Re: needless complexity in StartupXLOG

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-07-28T09:04:29Z

    At Tue, 27 Jul 2021 11:03:14 -0400, Robert Haas <robertmhaas@gmail.com> wrote in 
> On Tue, Jul 27, 2021 at 9:18 AM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> > FWIW, by the way, I complained that the variable name "promoted" is a
> > bit confusing.  It's old name was fast_promoted, which means that fast
> > promotion is being *requsted* and ongoing.  On the other hand the
> > current name "promoted" still means "(fast=non-fallback) promotion is
> > ongoing" so there was a conversation as the follows.
> >
> > https://www.postgresql.org/message-id/9fdd994d-a531-a52b-7906-e1cc22701310%40oss.nttdata.com
> 
> I agree - that variable name is also not great. I am open to making
> improvements in that area and in others that have been suggested on
> this thread, but my immediate goal is to figure out whether anyone
> objects to me committing the posted patch. If nobody comes up with a
> reason why it's a bad idea in the next few days, I'll plan to move
> ahead with it.

That's fine with me.

I still haven't find a way to lose the last checkpoint due to software
failure.  Repeated promotion without having new checkpoints is safe as
expected. We don't remove WAL files unless a checkpoint completes, and
a checkpoint preserves segments back to the one containing its redo
point.

In short, I'm for it.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  10. Re: needless complexity in StartupXLOG

    Andres Freund <andres@anarazel.de> — 2021-07-28T19:28:00Z

    Hi,

On 2021-07-26 12:12:53 -0400, Robert Haas wrote:
> My first thought was that we should do the check unconditionally,
> rather than just when bgwriterLaunched && LocalPromoteIsTriggered, and
> ERROR if it fails. But then I wondered what the point of that would be
> exactly. If we have such a bug -- and to the best of my knowledge
> there's no evidence that we do -- there's no particular reason it
> should only happen at the end of recovery. It could happen any time
> the system -- or the user, or malicious space aliens -- remove files
> from pg_wal, and we have no real idea about the timing of malicious
> space alien activity, so doing the test here rather than anywhere else
> just seems like a shot in the dark.

Yea. The history of that code being added doesn't suggest that there was
a concrete issue being addressed, from what I can tell.


> So at the moment I am leaning toward the view that we should just
> remove this check entirely, as in the attached, proposed patch.

+1


> Really, I think we should consider going further. If it's safe to
> write an end-of-recovery record rather than a checkpoint, why not do
> so all the time?

+many. The current split doesn't make much sense. For one, it often is a huge
issue if crash recovery takes a long time - why should we incur the cost that
we are OK avoiding during promotions? For another, end-of-recovery is a
crucial path for correctness, reducing the number of non-trivial paths is
good.


> Imagine if instead of
> all the hairy logic we have now we just replaced this whole if
> (IsInRecovery) stanza with this:
> 
> if (InRecovery)
>     CreateEndOfRecoveryRecord();
> 
> That would be WAY easier to reason about than the rat's nest we have
> here today. Now, I am not sure what it would take to get there, but I
> think that is the direction we ought to be heading.

What are we going to do in the single user ([1]) case in this awesome future?
I guess we could just not create a checkpoint until single user mode is shut
down / creates a checkpoint for other reasons?


Greetings,

Andres Freund


[1] I really wish somebody had the energy to just remove single user and
bootstrap modes. The degree to which they increase complexity in the rest of
the system is entirely unreasonable. There's not actually any reason
bootstrapping can't happen with checkpointer et al running, it's just
autovacuum that'd need to be blocked.

  11. Re: needless complexity in StartupXLOG

    Julien Rouhaud <rjuju123@gmail.com> — 2021-07-29T04:49:19Z

    On Thu, Jul 29, 2021 at 3:28 AM Andres Freund <andres@anarazel.de> wrote:
>
> [1] I really wish somebody had the energy to just remove single user and
> bootstrap modes. The degree to which they increase complexity in the rest of
> the system is entirely unreasonable. There's not actually any reason
> bootstrapping can't happen with checkpointer et al running, it's just
> autovacuum that'd need to be blocked.

Any objection to adding an entry for that in the wiki TODO?

  12. Re: needless complexity in StartupXLOG

    Amul Sul <sulamul@gmail.com> — 2021-07-29T05:47:16Z

    On Mon, Jul 26, 2021 at 9:43 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> StartupXLOG() has code beginning around line 7900 of xlog.c that
> decides, at the end of recovery, between four possible courses of
> action. It either writes an end-of-recovery record, or requests a
> checkpoint, or creates a checkpoint, or does nothing, depending on the
> value of 3 flag variables, and also on whether we're still able to
> read the last checkpoint record:
>
>                 checkPointLoc = ControlFile->checkPoint;
>
>                 /*
>                  * Confirm the last checkpoint is available for us to recover
>                  * from if we fail.
>                  */
>                 record = ReadCheckpointRecord(xlogreader,
> checkPointLoc, 1, false);
>                 if (record != NULL)
>                 {
>                     promoted = true;
>
> It seems to me that ReadCheckpointRecord() should never fail here. It
> should always be true, even when we're not in recovery, that the last
> checkpoint record is readable. If there's ever a situation where that
> is not true, even for an instant, then a crash at that point will be
> unrecoverable. Now, one way that it could happen is if we've got a bug
> in the code someplace that removes WAL segments too soon. However, if
> we have such a bug, we should fix it. What this code does is says "oh,
> I guess we removed the old checkpoint too soon, no big deal, let's
> just be more aggressive about getting the next one done," which I do
> not think is the right response. Aside from a bug, the only other way
> I can see it happening is if someone is manually removing WAL segments
> as the server is running through recovery, perhaps as some last-ditch
> play to avoid running out of disk space. I don't think the server
> needs to have - or should have - code to cater to such crazy
> scenarios. Therefore I think that this check, at least in its current
> form, is not sensible.
>
> My first thought was that we should do the check unconditionally,
> rather than just when bgwriterLaunched && LocalPromoteIsTriggered, and
> ERROR if it fails. But then I wondered what the point of that would be
> exactly. If we have such a bug -- and to the best of my knowledge
> there's no evidence that we do -- there's no particular reason it
> should only happen at the end of recovery. It could happen any time
> the system -- or the user, or malicious space aliens -- remove files
> from pg_wal, and we have no real idea about the timing of malicious
> space alien activity, so doing the test here rather than anywhere else
> just seems like a shot in the dark. Perhaps the most logical place
> would be to move it to CreateCheckPoint() just after we remove old
> xlog files, but we don't have the xlogreader there, and anyway I don't
> see how it's really going to help. What bug do we expect to catch by
> removing files we think we don't need and then checking that we didn't
> remove the files we think we do need? That seems more like grasping at
> straws than a serious attempt to make things work better.
>
> So at the moment I am leaning toward the view that we should just
> remove this check entirely, as in the attached, proposed patch.
>

Can we have an elog() fatal error or warning to make sure that the
last checkpoint is still readable? Since the case where the user
(knowingly or unknowingly) or some buggy code has removed the WAL file
containing the last checkpoint could be possible. If it is then we
would have a hard time finding out when we get further unexpected
behavior due to this. Thoughts?

> Really, I think we should consider going further. If it's safe to
> write an end-of-recovery record rather than a checkpoint, why not do
> so all the time? Right now we fail to do that in the above-described
> "impossible" scenario where the previous checkpoint record can't be
> read, or if we're exiting archive recovery for some reason other than
> a promotion request, or if we're in single-user mode, or if we're in
> crash recovery. Presumably, people would like to start up the server
> quickly in all of those scenarios, so the only reason not to use this
> technology all the time is if we think it's safe in some scenarios and
> not others. I can't think of a reason why it matters why we're exiting
> archive recovery, nor can I think of a reason why it matters whether
> we're in single user mode. The distinction between crash recovery and
> archive recovery does seem to matter, but if anything the crash
> recovery scenario seems simpler, because then there's only one
> timeline involved.
>
> I realize that conservatism may have played a role in this code ending
> up looking the way that it does; someone seems to have thought it
> would be better not to rely on a new idea in all cases. From my point
> of view, though, it's scary to have so many cases, especially cases
> that don't seem like they should ever be reached. I think that
> simplifying the logic here and trying to do the same things in as many
> cases as we can will lead to better robustness. Imagine if instead of
> all the hairy logic we have now we just replaced this whole if
> (IsInRecovery) stanza with this:
>
> if (InRecovery)
>     CreateEndOfRecoveryRecord();

+1, and do the checkpoint at the end unconditionally as we are doing
for the promotion.

Regards,
Amul

  13. Re: needless complexity in StartupXLOG

    Robert Haas <robertmhaas@gmail.com> — 2021-07-29T15:49:45Z

    On Wed, Jul 28, 2021 at 3:28 PM Andres Freund <andres@anarazel.de> wrote:
> > Imagine if instead of
> > all the hairy logic we have now we just replaced this whole if
> > (IsInRecovery) stanza with this:
> >
> > if (InRecovery)
> >     CreateEndOfRecoveryRecord();
> >
> > That would be WAY easier to reason about than the rat's nest we have
> > here today. Now, I am not sure what it would take to get there, but I
> > think that is the direction we ought to be heading.
>
> What are we going to do in the single user ([1]) case in this awesome future?
> I guess we could just not create a checkpoint until single user mode is shut
> down / creates a checkpoint for other reasons?

It probably depends on who writes this thus-far-hypothetical patch,
but my thought is that we'd unconditionally request a checkpoint after
writing the end-of-recovery record, same as we do now if (promoted).
If we happened to be in single-user mode, then that checkpoint request
would turn into performing a checkpoint on the spot in the one and
only process we've got, but StartupXLOG() wouldn't really need to care
what happens under the hood after it called RequestCheckpoint().

> [1] I really wish somebody had the energy to just remove single user and
> bootstrap modes. The degree to which they increase complexity in the rest of
> the system is entirely unreasonable. There's not actually any reason
> bootstrapping can't happen with checkpointer et al running, it's just
> autovacuum that'd need to be blocked.

I don't know whether this is the way forward or not. I think a lot of
the complexity of the current system is incidental rather than
intrinsic. If I were going to work on this, I'd probably working on
trying to tidy up the code and reduce the number of places that need
to care about IsUnderPostmaster and IsPostmasterEnvironment, rather
than trying to get rid of them. I suspect that's a necessary
prerequisite step anyway, and not a trivial effort either.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  14. Re: needless complexity in StartupXLOG

    Robert Haas <robertmhaas@gmail.com> — 2021-07-29T16:22:46Z

    On Thu, Jul 29, 2021 at 1:47 AM Amul Sul <sulamul@gmail.com> wrote:
> Can we have an elog() fatal error or warning to make sure that the
> last checkpoint is still readable? Since the case where the user
> (knowingly or unknowingly) or some buggy code has removed the WAL file
> containing the last checkpoint could be possible. If it is then we
> would have a hard time finding out when we get further unexpected
> behavior due to this. Thoughts?

Sure, we could, but I don't think we should. Such crazy things can
happen any time, not just at the point where this check is happening.
It's not particularly more likely to happen here vs. any other place
where we could insert a check. Should we check everywhere, all the
time, just in case?

> > I realize that conservatism may have played a role in this code ending
> > up looking the way that it does; someone seems to have thought it
> > would be better not to rely on a new idea in all cases. From my point
> > of view, though, it's scary to have so many cases, especially cases
> > that don't seem like they should ever be reached. I think that
> > simplifying the logic here and trying to do the same things in as many
> > cases as we can will lead to better robustness. Imagine if instead of
> > all the hairy logic we have now we just replaced this whole if
> > (IsInRecovery) stanza with this:
> >
> > if (InRecovery)
> >     CreateEndOfRecoveryRecord();
>
> +1, and do the checkpoint at the end unconditionally as we are doing
> for the promotion.

Yeah, that was my thought, too.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  15. Re: needless complexity in StartupXLOG

    Andres Freund <andres@anarazel.de> — 2021-07-29T19:16:09Z

    Hi,

On 2021-07-29 12:49:19 +0800, Julien Rouhaud wrote:
> On Thu, Jul 29, 2021 at 3:28 AM Andres Freund <andres@anarazel.de> wrote:
> >
> > [1] I really wish somebody had the energy to just remove single user and
> > bootstrap modes. The degree to which they increase complexity in the rest of
> > the system is entirely unreasonable. There's not actually any reason
> > bootstrapping can't happen with checkpointer et al running, it's just
> > autovacuum that'd need to be blocked.
> 
> Any objection to adding an entry for that in the wiki TODO?

Not sure there's enough concensus on the idea for that. I personally
think that's a good approach at reducing relevant complexity, but I
don't know if anybody agrees...

Greetings,

Andres Freund

  16. Re: needless complexity in StartupXLOG

    Robert Haas <robertmhaas@gmail.com> — 2021-07-30T12:39:15Z

    On Thu, Jul 29, 2021 at 3:16 PM Andres Freund <andres@anarazel.de> wrote:
> Not sure there's enough concensus on the idea for that. I personally
> think that's a good approach at reducing relevant complexity, but I
> don't know if anybody agrees...

There does seem to be agreement on the proposed patch, so I have committed it.

Thanks to all for the discussion and the links to other relevant threads.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  17. using an end-of-recovery record in all cases

    Robert Haas <robertmhaas@gmail.com> — 2021-08-09T19:00:40Z

    On Thu, Jul 29, 2021 at 11:49 AM Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Jul 28, 2021 at 3:28 PM Andres Freund <andres@anarazel.de> wrote:
> > > Imagine if instead of
> > > all the hairy logic we have now we just replaced this whole if
> > > (IsInRecovery) stanza with this:
> > >
> > > if (InRecovery)
> > >     CreateEndOfRecoveryRecord();
> > >
> > > That would be WAY easier to reason about than the rat's nest we have
> > > here today. Now, I am not sure what it would take to get there, but I
> > > think that is the direction we ought to be heading.
> >
> > What are we going to do in the single user ([1]) case in this awesome future?
> > I guess we could just not create a checkpoint until single user mode is shut
> > down / creates a checkpoint for other reasons?
>
> It probably depends on who writes this thus-far-hypothetical patch,
> but my thought is that we'd unconditionally request a checkpoint after
> writing the end-of-recovery record, same as we do now if (promoted).
> If we happened to be in single-user mode, then that checkpoint request
> would turn into performing a checkpoint on the spot in the one and
> only process we've got, but StartupXLOG() wouldn't really need to care
> what happens under the hood after it called RequestCheckpoint().

I decided to try writing a patch to use an end-of-recovery record
rather than a checkpoint record in all cases. The patch itself was
pretty simple but didn't work. There are two different reasons why it
didn't work, which I'll explain in a minute. I'm not sure whether
there are any other problems; these are the only two things that cause
problems with 'make check-world', but that's hardly a guarantee of
anything. Anyway, I thought it would be useful to report these issues
first and hopefully get some feedback.

The first problem I hit was that GetRunningTransactionData() does
Assert(TransactionIdIsNormal(CurrentRunningXacts->latestCompletedXid)).
While that does seem like a superficially reasonable thing to assert,
StartupXLOG() initializes latestCompletedXid by calling
TransactionIdRetreat on the value extracted from checkPoint.nextXid,
and the first-ever checkpoint that is written at the beginning of WAL
has a nextXid of 3, so when starting up from that checkpoint nextXid
is 2, which is not a normal XID. When we try to create the next
checkpoint, CreateCheckPoint() does LogStandbySnapshot() which calls
GetRunningTransactionData() and the assertion fails. In the current
code, we avoid this more or less accidentally because
LogStandbySnapshot() is skipped when starting from a shutdown
checkpoint. If we write an end-of-recovery record and then trigger a
checkpoint afterwards, then we don't avoid the problem. Although I'm
impishly tempted to suggest that we #define SecondNormalTransactionId
4 and then use that to create the first checkpoint instead of
FirstNormalTransactionId -- naturally with no comments explaining why
we're doing it -- I think the real problem is that the assertion is
just wrong. CurrentRunningXacts->latestCompletedXid shouldn't be
InvalidTransactionId or BootstrapTransactionId, but
FrozenTransactionId is a legal, if corner-case, value, at least as the
code stands today.

Unfortunately we can't just relax the assertion, because the
XLOG_RUNNING_XACTS record will eventually be handed to
ProcArrayApplyRecoveryInfo() for processing ... and that function
contains a matching assertion which would in turn fail. It in turn
passes the value to MaintainLatestCompletedXidRecovery() which
contains yet another matching assertion, so the restriction to normal
XIDs here looks pretty deliberate. There are no comments, though, so
the reader is left to guess why. I see one problem:
MaintainLatestCompletedXidRecovery uses FullXidRelativeTo, which
expects a normal XID. Perhaps it's best to just dodge the entire issue
by skipping LogStandbySnapshot() if latestCompletedXid happens to be
2, but that feels like a hack, because AFAICS the real problem is that
StartupXLog() doesn't agree with the rest of the code on whether 2 is
a legal case, and maybe we ought to be storing a value that doesn't
need to be computed via TransactionIdRetreat().

The second problem I hit was a preexisting bug where, under
wal_level=minimal, redo of a "create tablespace" record can blow away
data of which we have no other copy. See
http://postgr.es/m/CA+TgmoaLO9ncuwvr2nN-J4VEP5XyAcy=zKiHxQzBbFRxxGxm0w@mail.gmail.com
for details. That bug happens to make src/test/recovery's
018_wal_optimize.pl fail one of the tests, because the test starts and
stops the server repeatedly, with the result that with the attached
patch, we just keep writing end-of-recovery records and never getting
time to finish a checkpoint before the next shutdown, so every test
replays the CREATE TABLESPACE record and everything that previous
tests did. The "wal_level = minimal, SET TABLESPACE commit
subtransaction" fails because it's the only one that (1) uses the
tablespace for a new table, (2) commits, and (3) runs before a
checkpoint is manually forced.

It's also worth noting that if we go this way,
CHECKPOINT_END_OF_RECOVERY should be ripped out entirely. We'd still
be triggering a checkpoint at the end of recovery, but because it
could be running concurrently with WAL-generating activity, it
wouldn't be an end-of-recovery checkpoint in the sense that we now use
that term. In particular, you couldn't assume that no other write
transactions are running at the point when this checkpoint is
performed. I haven't yet tried ripping that out and doing so might
reveal other problems.

--
Robert Haas
EDB: http://www.enterprisedb.com

  18. Re: using an end-of-recovery record in all cases

    Robert Haas <robertmhaas@gmail.com> — 2021-09-02T15:30:59Z

    On Mon, Aug 9, 2021 at 3:00 PM Robert Haas <robertmhaas@gmail.com> wrote:
> I decided to try writing a patch to use an end-of-recovery record
> rather than a checkpoint record in all cases.
>
> The first problem I hit was that GetRunningTransactionData() does
> Assert(TransactionIdIsNormal(CurrentRunningXacts->latestCompletedXid)).
>
> Unfortunately we can't just relax the assertion, because the
> XLOG_RUNNING_XACTS record will eventually be handed to
> ProcArrayApplyRecoveryInfo() for processing ... and that function
> contains a matching assertion which would in turn fail. It in turn
> passes the value to MaintainLatestCompletedXidRecovery() which
> contains yet another matching assertion, so the restriction to normal
> XIDs here looks pretty deliberate. There are no comments, though, so
> the reader is left to guess why. I see one problem:
> MaintainLatestCompletedXidRecovery uses FullXidRelativeTo, which
> expects a normal XID. Perhaps it's best to just dodge the entire issue
> by skipping LogStandbySnapshot() if latestCompletedXid happens to be
> 2, but that feels like a hack, because AFAICS the real problem is that
> StartupXLog() doesn't agree with the rest of the code on whether 2 is
> a legal case, and maybe we ought to be storing a value that doesn't
> need to be computed via TransactionIdRetreat().

Anyone have any thoughts about this?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  19. Re: using an end-of-recovery record in all cases

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-09-03T04:52:53Z

    At Thu, 2 Sep 2021 11:30:59 -0400, Robert Haas <robertmhaas@gmail.com> wrote in 
> On Mon, Aug 9, 2021 at 3:00 PM Robert Haas <robertmhaas@gmail.com> wrote:
> > I decided to try writing a patch to use an end-of-recovery record
> > rather than a checkpoint record in all cases.
> >
> > The first problem I hit was that GetRunningTransactionData() does
> > Assert(TransactionIdIsNormal(CurrentRunningXacts->latestCompletedXid)).
> >
> > Unfortunately we can't just relax the assertion, because the
> > XLOG_RUNNING_XACTS record will eventually be handed to
> > ProcArrayApplyRecoveryInfo() for processing ... and that function
> > contains a matching assertion which would in turn fail. It in turn
> > passes the value to MaintainLatestCompletedXidRecovery() which
> > contains yet another matching assertion, so the restriction to normal
> > XIDs here looks pretty deliberate. There are no comments, though, so
> > the reader is left to guess why. I see one problem:
> > MaintainLatestCompletedXidRecovery uses FullXidRelativeTo, which
> > expects a normal XID. Perhaps it's best to just dodge the entire issue
> > by skipping LogStandbySnapshot() if latestCompletedXid happens to be
> > 2, but that feels like a hack, because AFAICS the real problem is that
> > StartupXLog() doesn't agree with the rest of the code on whether 2 is
> > a legal case, and maybe we ought to be storing a value that doesn't
> > need to be computed via TransactionIdRetreat().
> 
> Anyone have any thoughts about this?

I tried to reproduce this but just replacing the end-of-recovery
checkpoint request with issuing an end-of-recovery record didn't cause
make check-workd fail for me.  Do you have an idea of any other
requirement to cause that?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  20. Re: using an end-of-recovery record in all cases

    Amul Sul <sulamul@gmail.com> — 2021-09-03T10:26:35Z

    On Fri, Sep 3, 2021 at 10:23 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Thu, 2 Sep 2021 11:30:59 -0400, Robert Haas <robertmhaas@gmail.com> wrote in
> > On Mon, Aug 9, 2021 at 3:00 PM Robert Haas <robertmhaas@gmail.com> wrote:
> > > I decided to try writing a patch to use an end-of-recovery record
> > > rather than a checkpoint record in all cases.
> > >
> > > The first problem I hit was that GetRunningTransactionData() does
> > > Assert(TransactionIdIsNormal(CurrentRunningXacts->latestCompletedXid)).
> > >
> > > Unfortunately we can't just relax the assertion, because the
> > > XLOG_RUNNING_XACTS record will eventually be handed to
> > > ProcArrayApplyRecoveryInfo() for processing ... and that function
> > > contains a matching assertion which would in turn fail. It in turn
> > > passes the value to MaintainLatestCompletedXidRecovery() which
> > > contains yet another matching assertion, so the restriction to normal
> > > XIDs here looks pretty deliberate. There are no comments, though, so
> > > the reader is left to guess why. I see one problem:
> > > MaintainLatestCompletedXidRecovery uses FullXidRelativeTo, which
> > > expects a normal XID. Perhaps it's best to just dodge the entire issue
> > > by skipping LogStandbySnapshot() if latestCompletedXid happens to be
> > > 2, but that feels like a hack, because AFAICS the real problem is that
> > > StartupXLog() doesn't agree with the rest of the code on whether 2 is
> > > a legal case, and maybe we ought to be storing a value that doesn't
> > > need to be computed via TransactionIdRetreat().
> >
> > Anyone have any thoughts about this?
>
> I tried to reproduce this but just replacing the end-of-recovery
> checkpoint request with issuing an end-of-recovery record didn't cause
> make check-workd fail for me.  Do you have an idea of any other
> requirement to cause that?
>

You might need the following change at the end of StartupXLOG():

- if (promoted)
- RequestCheckpoint(CHECKPOINT_FORCE);
+ RequestCheckpoint(CHECKPOINT_FORCE);

Regards,
Amul

  21. Re: using an end-of-recovery record in all cases

    Robert Haas <robertmhaas@gmail.com> — 2021-09-03T14:13:53Z

    On Fri, Sep 3, 2021 at 12:52 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> I tried to reproduce this but just replacing the end-of-recovery
> checkpoint request with issuing an end-of-recovery record didn't cause
> make check-workd fail for me.  Do you have an idea of any other
> requirement to cause that?

Did you use the patch I posted, or a different one?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  22. Re: using an end-of-recovery record in all cases

    Amit Kapila <amit.kapila16@gmail.com> — 2021-09-04T09:51:14Z

    On Tue, Aug 10, 2021 at 12:31 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Thu, Jul 29, 2021 at 11:49 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > On Wed, Jul 28, 2021 at 3:28 PM Andres Freund <andres@anarazel.de> wrote:
> > > > Imagine if instead of
> > > > all the hairy logic we have now we just replaced this whole if
> > > > (IsInRecovery) stanza with this:
> > > >
> > > > if (InRecovery)
> > > >     CreateEndOfRecoveryRecord();
> > > >
> > > > That would be WAY easier to reason about than the rat's nest we have
> > > > here today. Now, I am not sure what it would take to get there, but I
> > > > think that is the direction we ought to be heading.
> > >
> > > What are we going to do in the single user ([1]) case in this awesome future?
> > > I guess we could just not create a checkpoint until single user mode is shut
> > > down / creates a checkpoint for other reasons?
> >
> > It probably depends on who writes this thus-far-hypothetical patch,
> > but my thought is that we'd unconditionally request a checkpoint after
> > writing the end-of-recovery record, same as we do now if (promoted).
> > If we happened to be in single-user mode, then that checkpoint request
> > would turn into performing a checkpoint on the spot in the one and
> > only process we've got, but StartupXLOG() wouldn't really need to care
> > what happens under the hood after it called RequestCheckpoint().
>
> I decided to try writing a patch to use an end-of-recovery record
> rather than a checkpoint record in all cases. The patch itself was
> pretty simple but didn't work. There are two different reasons why it
> didn't work, which I'll explain in a minute. I'm not sure whether
> there are any other problems; these are the only two things that cause
> problems with 'make check-world', but that's hardly a guarantee of
> anything. Anyway, I thought it would be useful to report these issues
> first and hopefully get some feedback.
>
> The first problem I hit was that GetRunningTransactionData() does
> Assert(TransactionIdIsNormal(CurrentRunningXacts->latestCompletedXid)).
> While that does seem like a superficially reasonable thing to assert,
> StartupXLOG() initializes latestCompletedXid by calling
> TransactionIdRetreat on the value extracted from checkPoint.nextXid,
> and the first-ever checkpoint that is written at the beginning of WAL
> has a nextXid of 3, so when starting up from that checkpoint nextXid
> is 2, which is not a normal XID. When we try to create the next
> checkpoint, CreateCheckPoint() does LogStandbySnapshot() which calls
> GetRunningTransactionData() and the assertion fails. In the current
> code, we avoid this more or less accidentally because
> LogStandbySnapshot() is skipped when starting from a shutdown
> checkpoint. If we write an end-of-recovery record and then trigger a
> checkpoint afterwards, then we don't avoid the problem. Although I'm
> impishly tempted to suggest that we #define SecondNormalTransactionId
> 4 and then use that to create the first checkpoint instead of
> FirstNormalTransactionId -- naturally with no comments explaining why
> we're doing it -- I think the real problem is that the assertion is
> just wrong. CurrentRunningXacts->latestCompletedXid shouldn't be
> InvalidTransactionId or BootstrapTransactionId, but
> FrozenTransactionId is a legal, if corner-case, value, at least as the
> code stands today.
>
> Unfortunately we can't just relax the assertion, because the
> XLOG_RUNNING_XACTS record will eventually be handed to
> ProcArrayApplyRecoveryInfo() for processing ... and that function
> contains a matching assertion which would in turn fail. It in turn
> passes the value to MaintainLatestCompletedXidRecovery() which
> contains yet another matching assertion, so the restriction to normal
> XIDs here looks pretty deliberate. There are no comments, though, so
> the reader is left to guess why. I see one problem:
> MaintainLatestCompletedXidRecovery uses FullXidRelativeTo, which
> expects a normal XID. Perhaps it's best to just dodge the entire issue
> by skipping LogStandbySnapshot() if latestCompletedXid happens to be
> 2,
>

By reading above explanation, it seems like it is better to skip
LogStandbySnapshot() for this proposal. Can't we consider skipping
LogStandbySnapshot() by passing some explicit flag-like
'recovery_checkpoint' or something like that?

I think there is some prior discussion in another thread related to
this work but it would be easier to follow if you can summarize the
same.


With Regards,
Amit Kapila.

  23. Re: using an end-of-recovery record in all cases

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-09-07T07:26:33Z

    At Fri, 3 Sep 2021 15:56:35 +0530, Amul Sul <sulamul@gmail.com> wrote in 
> On Fri, Sep 3, 2021 at 10:23 AM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> You might need the following change at the end of StartupXLOG():
> 
> - if (promoted)
> - RequestCheckpoint(CHECKPOINT_FORCE);
> + RequestCheckpoint(CHECKPOINT_FORCE);

At Fri, 3 Sep 2021 10:13:53 -0400, Robert Haas <robertmhaas@gmail.com> wrote in 
> Did you use the patch I posted, or a different one

Thanks to both of you. That was my stupid.

..But even with the patch (with removal of 018_..pl test file), I
didn't get check-world failed..  I'll retry later.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  24. Re: using an end-of-recovery record in all cases

    Amul Sul <sulamul@gmail.com> — 2021-10-05T11:43:41Z

    I was trying to understand the v1 patch and found that at the end
RequestCheckpoint() is called unconditionally, I think that should
have been called if REDO had performed, here is the snip from the v1
patch:

  /*
- * If this was a promotion, request an (online) checkpoint now. This isn't
- * required for consistency, but the last restartpoint might be far back,
- * and in case of a crash, recovering from it might take a longer than is
- * appropriate now that we're not in standby mode anymore.
+ * Request an (online) checkpoint now. Note that, until this is complete,
+ * a crash would start replay from the same WAL location we did, or from
+ * the last restartpoint that completed. We don't want to let that
+ * situation persist for longer than necessary, since users don't like
+ * long recovery times. On the other hand, they also want to be able to
+ * start doing useful work again as quickly as possible. Therfore, we
+ * don't pass CHECKPOINT_IMMEDIATE to avoid bogging down the system.
+ *
+ * Note that the consequence of requesting a checkpoint here only after
+ * we've allowed WAL writes is that a single checkpoint cycle can span
+ * multiple server lifetimes. So for example if you want to something to
+ * happen at least once per checkpoint cycle or at most once per
+ * checkpoint cycle, you have to consider what happens if the server
+ * is restarted someplace in the middle.
  */
- if (promoted)
- RequestCheckpoint(CHECKPOINT_FORCE);
+ RequestCheckpoint(CHECKPOINT_FORCE);

When I try to call that conditionally like attached, I don't see any
regression failure, correct me if I am missing something here.

Regards,
Amul

  25. Re: using an end-of-recovery record in all cases

    Robert Haas <robertmhaas@gmail.com> — 2021-10-05T15:34:11Z

    On Tue, Oct 5, 2021 at 7:44 AM Amul Sul <sulamul@gmail.com> wrote:
> I was trying to understand the v1 patch and found that at the end
> RequestCheckpoint() is called unconditionally, I think that should
> have been called if REDO had performed,

You're right. But I don't think we need an extra variable like this,
right? We can just test InRecovery?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  26. Re: using an end-of-recovery record in all cases

    Amul Sul <sulamul@gmail.com> — 2021-10-05T16:41:10Z

    On Tue, 5 Oct 2021 at 9:04 PM, Robert Haas <robertmhaas@gmail.com> wrote:

> On Tue, Oct 5, 2021 at 7:44 AM Amul Sul <sulamul@gmail.com> wrote:
> > I was trying to understand the v1 patch and found that at the end
> > RequestCheckpoint() is called unconditionally, I think that should
> > have been called if REDO had performed,
>
> You're right. But I don't think we need an extra variable like this,
> right? We can just test InRecovery?


No, InRecovery flag get cleared before this point. I think, we can use
lastReplayedEndRecPtr what you have suggested in other thread.

Regards,
Amul
-- 
Regards,
Amul Sul
EDB: http://www.enterprisedb.com

  27. Re: using an end-of-recovery record in all cases

    Robert Haas <robertmhaas@gmail.com> — 2021-10-05T17:12:03Z

    On Tue, Oct 5, 2021 at 12:41 PM Amul Sul <sulamul@gmail.com> wrote:
> No, InRecovery flag get cleared before this point. I think, we can use lastReplayedEndRecPtr what you have suggested in other thread.

Hmm, right, that makes sense. Perhaps I should start remembering what
I said in my own emails.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  28. Re: using an end-of-recovery record in all cases

    Amul Sul <sulamul@gmail.com> — 2021-10-06T13:54:23Z

    On Tue, Oct 5, 2021 at 10:42 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Tue, Oct 5, 2021 at 12:41 PM Amul Sul <sulamul@gmail.com> wrote:
> > No, InRecovery flag get cleared before this point. I think, we can use lastReplayedEndRecPtr what you have suggested in other thread.
>
> Hmm, right, that makes sense. Perhaps I should start remembering what
> I said in my own emails.
>

Here I end up with the attached version where I have dropped the
changes for standby.c and 018_wal_optimize.pl files.  Also, I am not
sure that we should have the changes for bgwriter.c and slot.c in this
patch, but that's not touched.

Regards,
Amul

  29. Re: using an end-of-recovery record in all cases

    Amul Sul <sulamul@gmail.com> — 2021-10-18T05:26:53Z

    On Wed, Oct 6, 2021 at 7:24 PM Amul Sul <sulamul@gmail.com> wrote:
>
> On Tue, Oct 5, 2021 at 10:42 PM Robert Haas <robertmhaas@gmail.com> wrote:
> >
> > On Tue, Oct 5, 2021 at 12:41 PM Amul Sul <sulamul@gmail.com> wrote:
> > > No, InRecovery flag get cleared before this point. I think, we can use lastReplayedEndRecPtr what you have suggested in other thread.
> >
> > Hmm, right, that makes sense. Perhaps I should start remembering what
> > I said in my own emails.
> >
>
> Here I end up with the attached version where I have dropped the
> changes for standby.c and 018_wal_optimize.pl files.  Also, I am not
> sure that we should have the changes for bgwriter.c and slot.c in this
> patch, but that's not touched.
>

Attached is the rebased and updated version. The patch removes the
newly introduced PerformRecoveryXLogAction() function. In addition to
that, removed the CHECKPOINT_END_OF_RECOVERY flag and its related
code.  Also, dropped changes for bgwriter.c and slot.c in this patch, which
seem unrelated to this work.

Regards,
Amul

  30. Re: using an end-of-recovery record in all cases

    Julien Rouhaud <rjuju123@gmail.com> — 2022-01-16T04:52:28Z

    Hi,

On Mon, Oct 18, 2021 at 10:56:53AM +0530, Amul Sul wrote:
> 
> Attached is the rebased and updated version. The patch removes the
> newly introduced PerformRecoveryXLogAction() function. In addition to
> that, removed the CHECKPOINT_END_OF_RECOVERY flag and its related
> code.  Also, dropped changes for bgwriter.c and slot.c in this patch, which
> seem unrelated to this work.

The cfbot reports that this version of the patch doesn't apply anymore:

http://cfbot.cputube.org/patch_36_3365.log
=== Applying patches on top of PostgreSQL commit ID 0c53a6658e47217ad3dd416a5543fc87c3ecfd58 ===
=== applying patch ./v3-0001-Always-use-an-end-of-recovery-record-rather-than-.patch
patching file src/backend/access/transam/xlog.c
[...]
Hunk #14 FAILED at 9061.
Hunk #15 FAILED at 9241.
2 out of 15 hunks FAILED -- saving rejects to file src/backend/access/transam/xlog.c.rej

Can you send a rebased version?  In the meantime I will switch the cf entry to
Waiting on Author.

  31. Re: using an end-of-recovery record in all cases

    Robert Haas <robertmhaas@gmail.com> — 2022-04-18T20:44:03Z

    On Sat, Jan 15, 2022 at 11:52 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> The cfbot reports that this version of the patch doesn't apply anymore:

Here is a new version of the patch which, unlike v1, I think is
something we could seriously consider applying (not before v16, of
course). It now removes CHECKPOINT_END_OF_RECOVERY completely, and I
attach a second patch as well which nukes checkPoint.PrevTimeLineID as
well.

I mentioned two problems with $SUBJECT in the first email with this
subject line. One was a bug, which Noah has since fixed (thanks,
Noah). The other problem is that LogStandbySnapshot() and a bunch of
its friends expect latestCompletedXid to always be a normal XID, which
is a problem because (1) we currently set nextXid to 3 and (2) at
startup, we compute latestCompletedXid = nextXid - 1. The current code
dodges this kind of accidentally: the checkpoint that happens at
startup is a "shutdown checkpoint" and thus skips logging a standby
snapshot, since a shutdown checkpoint is a sure indicator that there
are no running transactions. With the changes, the checkpoint at
startup happens after we've started allowing write transactions, and
thus a standby snapshot needs to be logged also. In the cases where
the end-of-recovery record was already being used, the problem could
have happened already, except for the fact that those cases involve a
standby promotion, which doesn't happen during initdb. I explored a
few possible ways of solving this problem.

The first thing I considered was replacing latestCompletedXid with a
name like incompleteXidHorizon. The idea is that, where
latestCompletedXid is the highest XID that is known to have committed
or aborted, incompleteXidHorizon would be the lowest XID such that all
known commits or aborts are for lower XIDs. In effect,
incompleteXidHorizon would be latestCompletedXid + 1. Since
latestCompletedXid is always normal except when it's 2,
incompleteXidHorizon would be normal in all cases. Initially this
seemed fairly promising, but it kind of fell down when I realized that
we copy latestCompletedXid into
ComputeXidHorizonsResult.latest_completed. That seemed to me to make
the consequences of the change a bit more far-reaching than I liked.
Also, it wasn't entirely clear to me that I wouldn't be introducing
any off-by-one errors into various wraparound calculations with this
approach.

The second thing I considered was skipping LogStandbySnapshot() during
initdb. There are two ways of doing this and neither of them seem that
great to me. Something that does work is to skip LogStandbySnapshot()
when in single user mode, but there is no particular reason why WAL
generated in single user mode couldn't be replayed on a standby, so
this doesn't seem great. It's too big a hammer for what we really
want, which is just to suppress this during initdb. Another way of
approaching it is to skip it in bootstrap mode, but that actually
doesn't work: initdb then fails during the post-bootstrapping step
rather than during bootstrapping. I thought about patching around that
by forcing the code that generates checkpoint records to forcibly
advance an XID of 3 to 4, but that seemed like working around the
problem from the wrong end.

So ... I decided that the best approach here seems to be to just skip
FirstNormalTransactionId and use FirstNormalTransactionId + 1 for the
first write transaction that the cluster ever processes. That's very
simple and doesn't seem likely to break anything else. On the downside
it seems a bit grotty, but I don't see anything better, and on the
whole, I think with this approach we come out substantially ahead.
0001 removes 3 times as many lines as it inserts, and 0002 saves a few
more lines of code.

Now, I still don't really know that there isn't some theoretical
difficulty here that makes this whole approach a non-starter, but I
also can't think of what it might be. If the promotion case, which has
used the end-of-recovery record for many years, is basically safe,
despite the fact that it switches TLIs, then it seems to me that the
crash recovery case, which doesn't have that complication, ought to be
safe too. But I might well be missing something, so if you see a
problem, please speak up!

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  32. Re: using an end-of-recovery record in all cases

    Amul Sul <sulamul@gmail.com> — 2022-04-19T03:56:00Z

    On Tue, Apr 19, 2022 at 2:14 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Sat, Jan 15, 2022 at 11:52 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > The cfbot reports that this version of the patch doesn't apply anymore:
>
> Here is a new version of the patch which, unlike v1, I think is
> something we could seriously consider applying (not before v16, of
> course). It now removes CHECKPOINT_END_OF_RECOVERY completely, and I
> attach a second patch as well which nukes checkPoint.PrevTimeLineID as
> well.
>
> I mentioned two problems with $SUBJECT in the first email with this
> subject line. One was a bug, which Noah has since fixed (thanks,
> Noah). The other problem is that LogStandbySnapshot() and a bunch of
> its friends expect latestCompletedXid to always be a normal XID, which
> is a problem because (1) we currently set nextXid to 3 and (2) at
> startup, we compute latestCompletedXid = nextXid - 1. The current code
> dodges this kind of accidentally: the checkpoint that happens at
> startup is a "shutdown checkpoint" and thus skips logging a standby
> snapshot, since a shutdown checkpoint is a sure indicator that there
> are no running transactions. With the changes, the checkpoint at
> startup happens after we've started allowing write transactions, and
> thus a standby snapshot needs to be logged also. In the cases where
> the end-of-recovery record was already being used, the problem could
> have happened already, except for the fact that those cases involve a
> standby promotion, which doesn't happen during initdb. I explored a
> few possible ways of solving this problem.
>
> The first thing I considered was replacing latestCompletedXid with a
> name like incompleteXidHorizon. The idea is that, where
> latestCompletedXid is the highest XID that is known to have committed
> or aborted, incompleteXidHorizon would be the lowest XID such that all
> known commits or aborts are for lower XIDs. In effect,
> incompleteXidHorizon would be latestCompletedXid + 1. Since
> latestCompletedXid is always normal except when it's 2,
> incompleteXidHorizon would be normal in all cases. Initially this
> seemed fairly promising, but it kind of fell down when I realized that
> we copy latestCompletedXid into
> ComputeXidHorizonsResult.latest_completed. That seemed to me to make
> the consequences of the change a bit more far-reaching than I liked.
> Also, it wasn't entirely clear to me that I wouldn't be introducing
> any off-by-one errors into various wraparound calculations with this
> approach.
>
> The second thing I considered was skipping LogStandbySnapshot() during
> initdb. There are two ways of doing this and neither of them seem that
> great to me. Something that does work is to skip LogStandbySnapshot()
> when in single user mode, but there is no particular reason why WAL
> generated in single user mode couldn't be replayed on a standby, so
> this doesn't seem great. It's too big a hammer for what we really
> want, which is just to suppress this during initdb. Another way of
> approaching it is to skip it in bootstrap mode, but that actually
> doesn't work: initdb then fails during the post-bootstrapping step
> rather than during bootstrapping. I thought about patching around that
> by forcing the code that generates checkpoint records to forcibly
> advance an XID of 3 to 4, but that seemed like working around the
> problem from the wrong end.
>
> So ... I decided that the best approach here seems to be to just skip
> FirstNormalTransactionId and use FirstNormalTransactionId + 1 for the
> first write transaction that the cluster ever processes. That's very
> simple and doesn't seem likely to break anything else. On the downside
> it seems a bit grotty, but I don't see anything better, and on the
> whole, I think with this approach we come out substantially ahead.
> 0001 removes 3 times as many lines as it inserts, and 0002 saves a few
> more lines of code.
>
> Now, I still don't really know that there isn't some theoretical
> difficulty here that makes this whole approach a non-starter, but I
> also can't think of what it might be. If the promotion case, which has
> used the end-of-recovery record for many years, is basically safe,
> despite the fact that it switches TLIs, then it seems to me that the
> crash recovery case, which doesn't have that complication, ought to be
> safe too. But I might well be missing something, so if you see a
> problem, please speak up!
>

  /*
- * If this was a promotion, request an (online) checkpoint now. This isn't
- * required for consistency, but the last restartpoint might be far back,
- * and in case of a crash, recovering from it might take a longer than is
- * appropriate now that we're not in standby mode anymore.
+ * Request an (online) checkpoint now. This isn't required for consistency,
+ * but the last restartpoint might be far back, and in case of a crash,
+ * recovering from it might take a longer than is appropriate now that
+ * we're not in standby mode anymore.
  */
- if (promoted)
- RequestCheckpoint(CHECKPOINT_FORCE);
+ RequestCheckpoint(CHECKPOINT_FORCE);
 }

I think RequestCheckpoint() should be called conditionally.  What is the need
of the checkpoint if we haven't been through the recovery, in other words,
starting up from a clean shutdown?

Regards,
Amul

  33. Re: using an end-of-recovery record in all cases

    Robert Haas <robertmhaas@gmail.com> — 2022-04-19T12:30:26Z

    On Mon, Apr 18, 2022 at 11:56 PM Amul Sul <sulamul@gmail.com> wrote:
> I think RequestCheckpoint() should be called conditionally.  What is the need
> of the checkpoint if we haven't been through the recovery, in other words,
> starting up from a clean shutdown?

Good point. v5 attached.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  34. Re: using an end-of-recovery record in all cases

    Nathan Bossart <nathandbossart@gmail.com> — 2022-04-19T20:37:59Z

    On Mon, Apr 18, 2022 at 04:44:03PM -0400, Robert Haas wrote:
> Here is a new version of the patch which, unlike v1, I think is
> something we could seriously consider applying (not before v16, of
> course). It now removes CHECKPOINT_END_OF_RECOVERY completely, and I
> attach a second patch as well which nukes checkPoint.PrevTimeLineID as
> well.

I'd like to add a big +1 for this change.  IIUC this should help with some
of the problems I've noted elsewhere [0].

> I mentioned two problems with $SUBJECT in the first email with this
> subject line. One was a bug, which Noah has since fixed (thanks,
> Noah). The other problem is that LogStandbySnapshot() and a bunch of
> its friends expect latestCompletedXid to always be a normal XID, which
> is a problem because (1) we currently set nextXid to 3 and (2) at
> startup, we compute latestCompletedXid = nextXid - 1. The current code
> dodges this kind of accidentally: the checkpoint that happens at
> startup is a "shutdown checkpoint" and thus skips logging a standby
> snapshot, since a shutdown checkpoint is a sure indicator that there
> are no running transactions. With the changes, the checkpoint at
> startup happens after we've started allowing write transactions, and
> thus a standby snapshot needs to be logged also. In the cases where
> the end-of-recovery record was already being used, the problem could
> have happened already, except for the fact that those cases involve a
> standby promotion, which doesn't happen during initdb. I explored a
> few possible ways of solving this problem.

Shouldn't latestCompletedXid be set to MaxTransactionId in this case?  Or
is this related to the logic in FullTransactionIdRetreat() that avoids
skipping over the "actual" special transaction IDs?

> So ... I decided that the best approach here seems to be to just skip
> FirstNormalTransactionId and use FirstNormalTransactionId + 1 for the
> first write transaction that the cluster ever processes. That's very
> simple and doesn't seem likely to break anything else. On the downside
> it seems a bit grotty, but I don't see anything better, and on the
> whole, I think with this approach we come out substantially ahead.
> 0001 removes 3 times as many lines as it inserts, and 0002 saves a few
> more lines of code.

This doesn't seem all that bad to me.  It's a little hacky, but it's very
easy to understand and only happens once per initdb.  I don't think it's
worth any extra complexity.

> Now, I still don't really know that there isn't some theoretical
> difficulty here that makes this whole approach a non-starter, but I
> also can't think of what it might be. If the promotion case, which has
> used the end-of-recovery record for many years, is basically safe,
> despite the fact that it switches TLIs, then it seems to me that the
> crash recovery case, which doesn't have that complication, ought to be
> safe too. But I might well be missing something, so if you see a
> problem, please speak up!

Your reasoning seems sound to me.

[0] https://postgr.es/m/C1EE64B0-D4DB-40F3-98C8-0CED324D34CB%40amazon.com

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  35. Re: using an end-of-recovery record in all cases

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-04-20T01:41:07Z

    At Tue, 19 Apr 2022 13:37:59 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in 
> On Mon, Apr 18, 2022 at 04:44:03PM -0400, Robert Haas wrote:
> > Here is a new version of the patch which, unlike v1, I think is
> > something we could seriously consider applying (not before v16, of
> > course). It now removes CHECKPOINT_END_OF_RECOVERY completely, and I
> > attach a second patch as well which nukes checkPoint.PrevTimeLineID as
> > well.
> 
> I'd like to add a big +1 for this change.  IIUC this should help with some
> of the problems I've noted elsewhere [0].

Agreed.

> > I mentioned two problems with $SUBJECT in the first email with this
> > subject line. One was a bug, which Noah has since fixed (thanks,
> > Noah). The other problem is that LogStandbySnapshot() and a bunch of
> > its friends expect latestCompletedXid to always be a normal XID, which
> > is a problem because (1) we currently set nextXid to 3 and (2) at
> > startup, we compute latestCompletedXid = nextXid - 1. The current code
> > dodges this kind of accidentally: the checkpoint that happens at
> > startup is a "shutdown checkpoint" and thus skips logging a standby
> > snapshot, since a shutdown checkpoint is a sure indicator that there
> > are no running transactions. With the changes, the checkpoint at
> > startup happens after we've started allowing write transactions, and
> > thus a standby snapshot needs to be logged also. In the cases where
> > the end-of-recovery record was already being used, the problem could
> > have happened already, except for the fact that those cases involve a
> > standby promotion, which doesn't happen during initdb. I explored a
> > few possible ways of solving this problem.
> 
> Shouldn't latestCompletedXid be set to MaxTransactionId in this case?  Or
> is this related to the logic in FullTransactionIdRetreat() that avoids
> skipping over the "actual" special transaction IDs?

As the result FullTransactionIdRetreat(FirstNormalFullTransactionId)
results in FrozenTransactionId, which looks odd.  It seems to me
rather should be InvalidFullTransactionId, or simply should assert-out
that case.  But incrmenting the very first xid avoid all that
complexity.  It is somewhat hacky but very smiple and understandable.

> > So ... I decided that the best approach here seems to be to just skip
> > FirstNormalTransactionId and use FirstNormalTransactionId + 1 for the
> > first write transaction that the cluster ever processes. That's very
> > simple and doesn't seem likely to break anything else. On the downside
> > it seems a bit grotty, but I don't see anything better, and on the
> > whole, I think with this approach we come out substantially ahead.
> > 0001 removes 3 times as many lines as it inserts, and 0002 saves a few
> > more lines of code.
> 
> This doesn't seem all that bad to me.  It's a little hacky, but it's very
> easy to understand and only happens once per initdb.  I don't think it's
> worth any extra complexity.

+1.

> > Now, I still don't really know that there isn't some theoretical
> > difficulty here that makes this whole approach a non-starter, but I
> > also can't think of what it might be. If the promotion case, which has
> > used the end-of-recovery record for many years, is basically safe,
> > despite the fact that it switches TLIs, then it seems to me that the
> > crash recovery case, which doesn't have that complication, ought to be
> > safe too. But I might well be missing something, so if you see a
> > problem, please speak up!
> 
> Your reasoning seems sound to me.
> 
> [0] https://postgr.es/m/C1EE64B0-D4DB-40F3-98C8-0CED324D34CB%40amazon.com

FWIW, I don't find a flaw in the reasoning, too.

By the way do we need to leave CheckPoint.PrevTimeLineID? It is always
the same value with ThisTimeLineID.  The most dubious part is
ApplyWalRecord but XLOG_CHECKPOINT_SHUTDOWN no longer induces timeline
switch.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  36. Re: using an end-of-recovery record in all cases

    Amul Sul <sulamul@gmail.com> — 2022-04-20T06:54:07Z

     On Tue, Apr 19, 2022 at 2:14 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Sat, Jan 15, 2022 at 11:52 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > The cfbot reports that this version of the patch doesn't apply anymore:
>
> Here is a new version of the patch which, unlike v1, I think is
> something we could seriously consider applying (not before v16, of
> course). It now removes CHECKPOINT_END_OF_RECOVERY completely, and I
> attach a second patch as well which nukes checkPoint.PrevTimeLineID as
> well.
>
> I mentioned two problems with $SUBJECT in the first email with this
> subject line. One was a bug, which Noah has since fixed (thanks,
> Noah). The other problem is that LogStandbySnapshot() and a bunch of
> its friends expect latestCompletedXid to always be a normal XID, which
> is a problem because (1) we currently set nextXid to 3 and (2) at
> startup, we compute latestCompletedXid = nextXid - 1. The current code
> dodges this kind of accidentally: the checkpoint that happens at
> startup is a "shutdown checkpoint" and thus skips logging a standby
> snapshot, since a shutdown checkpoint is a sure indicator that there
> are no running transactions. With the changes, the checkpoint at
> startup happens after we've started allowing write transactions, and
> thus a standby snapshot needs to be logged also. In the cases where
> the end-of-recovery record was already being used, the problem could
> have happened already, except for the fact that those cases involve a
> standby promotion, which doesn't happen during initdb. I explored a
> few possible ways of solving this problem.
>
> The first thing I considered was replacing latestCompletedXid with a
> name like incompleteXidHorizon. The idea is that, where
> latestCompletedXid is the highest XID that is known to have committed
> or aborted, incompleteXidHorizon would be the lowest XID such that all
> known commits or aborts are for lower XIDs. In effect,
> incompleteXidHorizon would be latestCompletedXid + 1. Since
> latestCompletedXid is always normal except when it's 2,
> incompleteXidHorizon would be normal in all cases. Initially this
> seemed fairly promising, but it kind of fell down when I realized that
> we copy latestCompletedXid into
> ComputeXidHorizonsResult.latest_completed. That seemed to me to make
> the consequences of the change a bit more far-reaching than I liked.
> Also, it wasn't entirely clear to me that I wouldn't be introducing
> any off-by-one errors into various wraparound calculations with this
> approach.
>
> The second thing I considered was skipping LogStandbySnapshot() during
> initdb. There are two ways of doing this and neither of them seem that
> great to me. Something that does work is to skip LogStandbySnapshot()
> when in single user mode, but there is no particular reason why WAL
> generated in single user mode couldn't be replayed on a standby, so
> this doesn't seem great. It's too big a hammer for what we really
> want, which is just to suppress this during initdb. Another way of
> approaching it is to skip it in bootstrap mode, but that actually
> doesn't work: initdb then fails during the post-bootstrapping step
> rather than during bootstrapping. I thought about patching around that
> by forcing the code that generates checkpoint records to forcibly
> advance an XID of 3 to 4, but that seemed like working around the
> problem from the wrong end.
>
> So ... I decided that the best approach here seems to be to just skip
> FirstNormalTransactionId and use FirstNormalTransactionId + 1 for the
> first write transaction that the cluster ever processes. That's very
> simple and doesn't seem likely to break anything else. On the downside
> it seems a bit grotty, but I don't see anything better, and on the
> whole, I think with this approach we come out substantially ahead.

IIUC, the failure was something like this on initdb:

running bootstrap script ... TRAP:
FailedAssertion("TransactionIdIsNormal(CurrentRunningXacts->latestCompletedXid)",
File: "procarray.c", Line: 2892, PID: 60363)

/bin/postgres(ExceptionalCondition+0xb9)[0xb3917d]
/bin/postgres(GetRunningTransactionData+0x36c)[0x96aa26]
/bin/postgres(LogStandbySnapshot+0x64)[0x974393]
/bin/postgres(CreateCheckPoint+0x67f)[0x5928bf]
/bin/postgres(RequestCheckpoint+0x26)[0x8ca649]
/bin/postgres(StartupXLOG+0xf51)[0x591126]
/bin/postgres(InitPostgres+0x188)[0xb4f2ac]
/bin/postgres(BootstrapModeMain+0x4d3)[0x5ac6de]
/bin/postgres(main+0x275)[0x7ca72e]
/lib64/libc.so.6(__libc_start_main+0xf5)[0x7f71af82d445]
/bin/postgres[0x48aae9]
child process was terminated by signal 6: Aborted
initdb: removing data directory "/inst/data"

That was happening because RequestCheckpoint() was called from StartupXLOG()
unconditionally, but with the v5 patch that is not true.

If my understanding is correct then we don't need any handling
for latestCompletedXid, at least in this patch.

Regards,
Amul

  37. Re: using an end-of-recovery record in all cases

    Robert Haas <robertmhaas@gmail.com> — 2022-04-20T13:26:07Z

    On Tue, Apr 19, 2022 at 4:38 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> Shouldn't latestCompletedXid be set to MaxTransactionId in this case?  Or
> is this related to the logic in FullTransactionIdRetreat() that avoids
> skipping over the "actual" special transaction IDs?

The problem here is this code:

    /* also initialize latestCompletedXid, to nextXid - 1 */
    LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
    ShmemVariableCache->latestCompletedXid = ShmemVariableCache->nextXid;
    FullTransactionIdRetreat(&ShmemVariableCache->latestCompletedXid);
    LWLockRelease(ProcArrayLock);

If nextXid is 3, then latestCompletedXid gets 2. But in
GetRunningTransactionData:

    Assert(TransactionIdIsNormal(CurrentRunningXacts->latestCompletedXid));

> Your reasoning seems sound to me.

I was talking with Thomas Munro yesterday and he thinks there is a
problem with relfilenode reuse here. In normal running, when a
relation is dropped, we leave behind a 0-length file until the next
checkpoint; this keeps that relfilenode from being used even if the
OID counter wraps around. If we didn't do that, then imagine that
while running with wal_level=minimal, we drop an existing relation,
create a new relation with the same OID, load some data into it, and
crash, all within the same checkpoint cycle, then we will be able to
replay the drop, but we will not be able to restore the relation
contents afterward because at wal_level=minimal they are not logged.
Apparently, we don't create tombstone files during recovery because we
know that there will be a checkpoint at the end.

With the existing use of the end-of-recovery record, we always know
that wal_level>minimal, because we're only using it on standbys. But
with this use that wouldn't be true any more. So I guess we need to
start creating tombstone files even during recovery, or else do
something like what Dilip coded up in
http://postgr.es/m/CAFiTN-u=r8UTCSzu6_pnihYAtwR1=esq5sRegTEZ2tLa92fovA@mail.gmail.com
which I think would be a better solution at least in the long term.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  38. Re: using an end-of-recovery record in all cases

    Nathan Bossart <nathandbossart@gmail.com> — 2022-04-20T17:02:24Z

    On Wed, Apr 20, 2022 at 09:26:07AM -0400, Robert Haas wrote:
> I was talking with Thomas Munro yesterday and he thinks there is a
> problem with relfilenode reuse here. In normal running, when a
> relation is dropped, we leave behind a 0-length file until the next
> checkpoint; this keeps that relfilenode from being used even if the
> OID counter wraps around. If we didn't do that, then imagine that
> while running with wal_level=minimal, we drop an existing relation,
> create a new relation with the same OID, load some data into it, and
> crash, all within the same checkpoint cycle, then we will be able to
> replay the drop, but we will not be able to restore the relation
> contents afterward because at wal_level=minimal they are not logged.
> Apparently, we don't create tombstone files during recovery because we
> know that there will be a checkpoint at the end.

In the example you provided, won't the tombstone file already be present
before the crash?  During recovery, the tombstone file will be removed, and
the new relation wouldn't use the same relfilenode anyway.  I'm probably
missing something obvious here.

I do see the problem if we drop an existing relation, crash, reuse the
filenode, and then crash again (all within the same checkpoint cycle).  The
first recovery would remove the tombstone file, and the second recovery
would wipe out the new relation's files.

> With the existing use of the end-of-recovery record, we always know
> that wal_level>minimal, because we're only using it on standbys. But
> with this use that wouldn't be true any more. So I guess we need to
> start creating tombstone files even during recovery, or else do
> something like what Dilip coded up in
> http://postgr.es/m/CAFiTN-u=r8UTCSzu6_pnihYAtwR1=esq5sRegTEZ2tLa92fovA@mail.gmail.com
> which I think would be a better solution at least in the long term.

IMO this would be good just to reduce the branching a bit.  I suppose
removing the files immediately during recovery might be an optimization in
some cases, but I am skeptical that it really makes that much of a
difference in practice.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  39. Re: using an end-of-recovery record in all cases

    Thomas Munro <thomas.munro@gmail.com> — 2022-04-20T17:16:20Z

    On Thu, Apr 21, 2022 at 5:02 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
> I do see the problem if we drop an existing relation, crash, reuse the
> filenode, and then crash again (all within the same checkpoint cycle).  The
> first recovery would remove the tombstone file, and the second recovery
> would wipe out the new relation's files.

Right, the double-crash case is what I was worrying about.  I'm not
sure, but it might even be more likely than usual that you'll reuse
the same relfilenode after the first crash, because the OID allocator
will start from the same value.