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

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. I would prefer this
idea than to invent a new API/tool like pg_replslotdata.

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?

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.

Thoughts?

--
With Regards,
Amit Kapila.