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

On Mon, Aug 14, 2023 at 2:07 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Aug 14, 2023 at 7:57 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Sat, Aug 12, 2023, 15:20 Amit Kapila <amit.kapila16@gmail.com> wrote:
> >>
> >> I don't think we need the complexity of version-specific checks if we
> >> do what we do in get_control_data(). Basically, invoke
> >> version-specific pg_replslotdata to get version-specific slot
> >> information. There has been a proposal for a tool like that [1]. Do
> >> you have something better in mind? If so, can you please explain the
> >> same a bit more?
> >
> >
> > Yeah, we need something like pg_replslotdata. If there are other useful usecases for this tool, it would be good to
haveit. But I'm not sure other than pg_upgrade usecase. 
> >
> > Another idea is (which might have already discussed thoguh) that we check if the latest shutdown checkpoint LSN in
thecontrol file matches the confirmed_flush_lsn in pg_replication_slots view. That way, we can ensure that the slot has
consumedall WAL records before the last shutdown. We don't need to worry about WAL records generated after starting the
oldcluster during the upgrade, at least for logical replication slots. 
> >
>
> Right, this is somewhat closer to what Patch is already doing. But
> remember in this case we need to remember and use the latest
> checkpoint from the control file before the old cluster is started
> because otherwise the latest checkpoint location could be even updated
> during the upgrade. So, instead of reading from WAL, we need to change
> so that we rely on the control file's latest LSN.

Yes, I was thinking the same idea.

But it works for only replication slots for logical replication. Do we
want to check if no meaningful WAL records are generated after the
latest shutdown checkpoint, for manually created slots (or non-logical
replication slots)? If so, we would need to have something reading WAL
records in the end.

> I would prefer this
> idea than to invent a new API/tool like pg_replslotdata.

+1

>
> The other point you and Bruce seem to be favoring is that instead of
> dumping/restoring slots via pg_dump, we remember the required
> information of slots retrieved during their validation in pg_upgrade
> itself and use that to create the slots in the new cluster. Though I
> am not aware of doing similar treatment for other objects we restore
> in this case it seems reasonable especially because slots are not
> stored in the catalog and we anyway already need to retrieve the
> required information to validate them, so trying to again retrieve it
> via pg_dump doesn't seem useful unless I am missing something. Does
> this match your understanding?

If there are use cases for --logical-replication-slots-only option
other than pg_upgrade, it would be good to have it in pg_dump. I was
just not sure of other use cases.

>
> Yet another thing I am trying to consider is whether we can allow to
> upgrade slots from 16 or 15 to later versions. As of now, the patch
> has the following check:
> getLogicalReplicationSlots()
> {
> ...
> + /* Check whether we should dump or not */
> + if (fout->remoteVersion < 170000)
> + return;
> ...
> }
>
> If we decide to use the existing view pg_replication_slots then can we
> consider upgrading slots from the prior version to 17? Now, if we want
> to invent any new API similar to pg_replslotdata then we can't do this
> because it won't exist in prior versions but OTOH using existing view
> pg_replication_slots can allow us to fetch slot info from older
> versions as well. So, I think it is worth considering.

I think that without 0001 patch the replication slots will not be able
to pass the confirmed_flush_lsn check.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com