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

Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>

From: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>
To: 'Bharath Rupireddy' <bharath.rupireddyforpostgres@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, Dilip Kumar <dilipbalaut@gmail.com>, "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, Peter Smith <smithpb2250@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Bruce Momjian <bruce@momjian.us>, Julien Rouhaud <rjuju123@gmail.com>, vignesh C <vignesh21@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>
Date: 2023-09-27T04:34:16Z
Lists: pgsql-hackers
Dear Bharath,

Thank you for reviewing!

> Thanks for the new patch. Here's a comment on v46:
> 
> 1.
> +Datum
> +binary_upgrade_validate_wal_logical_end(PG_FUNCTION_ARGS
> +{ oid => '8046', descr => 'for use by pg_upgrade',
> +  proname => 'binary_upgrade_validate_wal_logical_end', proisstrict => 'f',
> +  provolatile => 'v', proparallel => 'u', prorettype => 'bool',
> +  proargtypes => 'pg_lsn',
> +  prosrc => 'binary_upgrade_validate_wal_logical_end' },
> 
> I think this patch can avoid catalog changes by turning
> binary_upgrade_validate_wal_logical_end a FRONTEND-only function
> sitting in xlogreader.c after making InitXLogReaderState(),
> ReadNextXLogRecord() FRONTEND-friendly (replace elog/ereport with
> pg_fatal or such). With this change and back-porting of commit
> e0b2eed0 to save logical slots at shutdown, the patch can help support
> upgrading logical replication slots on PG versions < 17.

Hmm, I think your suggestion may be questionable.

If we implement the upgrading function as FRONTEND-only (I have not checked its
feasibility), it means pg_upgrade uses the latest version WAL reader API to read
WALs in old version cluster, which I didn't think is suggested.

Each WAL page header has a magic number, XLOG_PAGE_MAGIC, which indicates the
content of WAL. Sometimes the value has been changed due to the changes of WAL
contents, and some functions requires that the magic number must be same as
expected. E.g., startup process and pg_walinspect functions require that.
Typically XLogReaderValidatePageHeader() ensures the equality.

Now some functions are ported from pg_walinspect, so upgrading function requires
same restriction. I think we should not ease the restriction to verify the
completeness of files. Followings are the call stack of ported functions
till XLogReaderValidatePageHeader().

```
InitXLogReaderState()
XLogFindNextRecord()
ReadPageInternal()
XLogReaderValidatePageHeader()
```

```
ReadNextXLogRecord()
XLogReadRecord()
XLogReadAhead()
XLogDecodeNextRecord()
ReadPageInternal()
XLogReaderValidatePageHeader()
```

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Commits

  1. Fix issues in binary_upgrade_logical_slot_has_caught_up().

  2. Fix a random failure in 003_logical_slots.pl.

  3. Fix a test in 003_logical_slots.

  4. Fix uninitialized slot array access during the upgrade.

  5. Fix the test 003_logical_slots.

  6. Commit b195e6d482 forgot to update meson.build.

  7. Use shorter file names in the upgrade logical slots test.

  8. Migrate logical slots to the new node during an upgrade.

  9. Flush logical slots to disk during a shutdown checkpoint if required.

  10. Prevent possibility of panics during shutdown checkpoint.