Re: [PoC] pg_upgrade: allow to upgrade publisher node

On Tue, Oct 3, 2023 at 9:58 AM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Fri, Sep 29, 2023 at 5:27 PM Hayato Kuroda (Fujitsu)
> <kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
> >
> > Yeah, the approach enforces developers to check the decodability.
> > But the benefit seems smaller than required efforts for it because the function
> > would be used only by pg_upgrade. Could you tell me if you have another use case
> > in mind? We may able to adopt if we have...
>
> I'm attaching 0002 patch (on top of v45) which implements the new
> decodable callback approach that I have in mind. IMO, this new
> approach is extensible, better than the current approach (hard-coding
> of certain WAL records that may be generated during pg_upgrade) taken
> by the patch, and helps deal with the issue that custom WAL resource
> managers can have with the current approach taken by the patch.
>

+xlog_is_record_decodable(uint8 info)
+{
+ switch (info)
+ {
+ case XLOG_CHECKPOINT_SHUTDOWN:
+ case XLOG_END_OF_RECOVERY:
+ return true;
+ case XLOG_CHECKPOINT_ONLINE:
+ case XLOG_PARAMETER_CHANGE:
...
+ return false;
}

I think this won't behave correctly. Without your patch, we consider
both XLOG_CHECKPOINT_SHUTDOWN and XLOG_CHECKPOINT_ONLINE as valid
records but after patch only one of these will be considered valid
which won't lead to desired behavior.

BTW, the API proposed in your patch returns the WAL record type as
valid if there is something we do for it during decoding but the check
in upgrade function expects the reverse value. For example, for WAL
record type XLOG_HEAP_INSERT, the API returns true and that is
indication to the caller that this is an expected record after
confirmed_flush LSN location which doesn't seem correct. Am I missing
something?

--
With Regards,
Amit Kapila.