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

On Fri, Oct 27, 2023 at 3:28 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Fri, Oct 27, 2023 at 2:26 AM Bharath Rupireddy
> <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> >
> > On Thu, Oct 26, 2023 at 8:11 PM Zhijie Hou (Fujitsu)
> > <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> > >
> > > The BF animal fairywren[1] failed when testing
> > > 003_upgrade_logical_replication_slots.pl.
> > >
> > > From the log, I can see pg_upgrade failed to open the
> > > invalid_logical_replication_slots.txt:
> > >
> > > # Checking for valid logical replication slots
> > > # could not open file "C:/tools/nmsys64/home/pgrunner/bf/root/HEAD/pgsql.build/testrun/pg_upgrade/003_upgrade_logical_replication_slots/data/t_003_upgrade_logical_replication_slots_new_publisher_data/pgdata/pg_upgrade_output.d/20231026T112558.309/invalid_logical_replication_slots.txt": No such file or directory
> > > # Failure, exiting
> > >
> > > The reason could be the length of this path(262) exceed the windows path
> > > limit(260 IIRC). If so, I recall we fixed similar things before (e213de8e7) by
> > > reducing the path somehow.
> >
> > Nice catch. Windows docs say that the file/directory path name can't
> > exceed MAX_PATH, which is defined as 260 characters. However, one must
> > opt-in to enable longer path names -
> > https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation?tabs=registry
> > and https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation?tabs=registry#enable-long-paths-in-windows-10-version-1607-and-later.
> >
> > > In this case, I think one approach is to reduce the file and testname to
> > > xxx_logical_slots instead of xxx_logical_replication_slots. But we will analyze more
> > > and share fix soon.
> > >
> > > [1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2023-10-26%2009%3A04%3A54
> >
> > +1 for s/003_upgrade_logical_replication_slots.pl/003_upgrade_logical_slots.pl
> > and s/invalid_logical_replication_slots.txt/invalid_logical_slots.txt.

+1. The proposed file name sounds reasonable.

> > In fact, we've used "logical slots" instead of "logical replication
> > slots" in the docs to be generic. By looking at the generated
> > directory path name, I think we can use shorter node names - instead
> > of old_publisher, new_publisher, subscriber - either use node1 (for
> > old publisher), node2 (for subscriber), node3 (for new publisher) or
> > use alpha (for old publisher), bravo (for subscriber), charlie (for
> > new publisher) or such shorter names. We don't have to be that
> > descriptive and long in node names, one can look at the test file to
> > know which one is what.
> >
>
> Some more ideas for shortening the filename:
>
> 1. "003_upgrade_logical_replication_slots.pl" -- IMO the word
> "upgrade" is redundant in that filename (earlier patches never had
> this). The test file lives under "pg_upgrade/t" so I felt that
> upgrading is already implied.
>

Agreed. So, how about 003_upgrade_logical_slots.pl or simply
003_upgrade_slots.pl?

> 2. If the node names will be shortened they should still retain *some*
> meaning if possible:
> old_publisher/subscriber/new_publisher --> node1/node2/node3 (means
> nothing without studying the tests)
> old_publisher/subscriber/new_publisher --> alpha/bravo/charlie (means
> nothing without studying the tests)
> How about:
> old_publisher/subscriber/new_publisher --> node_p1/node_s/node_p2
> or similar...
>

Why not simply oldpub/sub/newpub or old_pub/sub/new_pub?

--
With Regards,
Amit Kapila.