Horton

Re: standby recovery fails (tablespace related) (tentative patch and discussion)

Kyotaro Horiguchi <horikyota.ntt@gmail.com>

View thread View on postgr.es
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
To: michael@paquier.xyz
Cc: rjuju123@gmail.com, pgsql-hackers@postgresql.org
Date: 2022-03-04T06:30:57Z
Lists: pgsql-hackers 
Thanks to look this!

At Fri, 4 Mar 2022 13:51:12 +0900, Michael Paquier <michael@paquier.xyz> wrote i
n 
> On Fri, Mar 04, 2022 at 09:10:48AM +0900, Kyotaro Horiguchi wrote:
> > And same function contained a maybe-should-have-been-removed line
> > which makes Windows build unhappy.
> > 
> > This should make all platforms in the CI happy.
> 
> d6d317d as solved the issue of tablespace paths across multiple nodes
> with the new GUC called allow_in_place_tablespaces, and is getting
> successfully used in the recovery tests as of 027_stream_regress.pl.

The feature allows only one tablespace directory. but that uses (I'm
not sure it needs, though) multiple tablespace directories so I think
the feature doesn't work for the test.

Maybe I'm missing something, but it doesn't use tablespaces.  I see
that in 002_tablespace.pl but but the test uses only one tablespace
location.

> Shouldn't we rely on that rather than extending more our test perl
> modules?  One tricky part is the emulation of readlink for junction
> points on Windows (dir_readlink in your patch), and the root of the

Yeah, I don't like that as I said before...

> problem is that 0003 cares about the path structure of the
> tablespaces so we have no need, as far as I can see, for any
> dependency with link follow-up in the scope of this patch.

I'm not sure how this related to 0001 but maybe I don't follow this.

> This means that you should be able to simplify the patch set, as we
> could entirely drop 0001 in favor of enforcing the new dev GUC in the
> nodes created in the TAP test of 0002.

Maybe it's possible by breaking the test into ones that need only one
tablespace.  I'll give it a try.

> Speaking of 0002, perhaps this had better be in its own file rather
> than extending more 011_crash_recovery.pl.  0003 looks like a good

Ok, no problem.

> idea to check after the consistency of the path structures created
> during replay, and it touches paths I'd expect it to touch, as of
> database and tbspace redos.
> 
> +       if (!reachedConsistency)
> +           XLogForgetMissingDir(xlrec->ts_id, InvalidOid);
> +
> +       XLogFlush(record->EndRecPtr);
> Not sure to understand why this is required.  A comment may be in
> order to explain the hows and the whys.

Is it about XLogFlush?  As my understanding it is to update
minRecoveryPoint to that LSN.  I'll add a comment like that.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Commits

  1. Improve recently-added test reliability

  2. Fix new recovery test for log_error_verbosity=verbose case

  3. Fix test instability

  4. Fix replay of create database records on standby

  5. Allow "in place" tablespaces.

  6. Fix get_dirent_type() for Windows junction points.

  7. Revert "Fix replay of create database records on standby"

  8. Add end-to-end testing of pg_basebackup's tar-format output.

  9. Make DROP DATABASE command generate less WAL records.

  10. Consolidate methods for translating a Perl path to a Windows path.