Horton

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

Daniel Gustafsson <daniel@yesql.se>

View thread View on postgr.es
From: Daniel Gustafsson <daniel@yesql.se>
To: Paul Guo <guopa@vmware.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Robert Haas <robertmhaas@gmail.com>, Ibrar Ahmed <ibrar.ahmad@gmail.com>, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, Alvaro Herrera <alvherre@2ndquadrant.com>, Fujii Masao <masao.fujii@oss.nttdata.com>, Anastasia Lubennikova <a.lubennikova@postgrespro.ru>, Asim Praveen <pasim@vmware.com>, "thomas.munro@gmail.com" <thomas.munro@gmail.com>, "horiguchi.kyotaro@lab.ntt.co.jp" <horiguchi.kyotaro@lab.ntt.co.jp>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Date: 2021-11-04T12:34:33Z
Lists: pgsql-hackers 
> On 24 Sep 2021, at 20:14, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Robert Haas <robertmhaas@gmail.com> writes:
>> The commit message for 0001 is not clear enough for me to understand
>> what problem it's supposed to be fixing. The code comments aren't
>> really either. They make it sound like there's some problem with
>> copying symlinks but mostly they just talk about callbacks, which
>> doesn't really help me understand what problem we'd have if we just
>> didn't commit this (or reverted it later).
> 
>> I am not really convinced by Álvaro's claim that 0004 is a "fix"; I
>> think I'd call it an improvement. But either way I agree that could
>> just be committed.
> 
>> I haven't analyzed 0002 and 0003 yet.
> 
> I took a quick look through this:
> 
> * I don't like 0001 either, though it seems like the issue is mostly
> documentation.  sub _srcsymlink should have a comment explaining
> what it's doing and why.  The documentation of copypath's new parameter
> seems like gobbledegook too --- I suppose it should read more like
> "By default, copypath fails if a source item is a symlink.  But if
> B<srcsymlinkfn> is provided, that subroutine is called to process any
> symlink."
> 
> * I'm allergic to 0002's completely undocumented changes to
> poll_query_until, especially since I don't see anything in the
> patch that actually uses them.  Can't we just drop these diffs
> in PostgresNode.pm?  BTW, the last error message in the patch,
> talking about a 5-second timeout, seems wrong.  With or without
> these changes, poll_query_until's default timeout is 180 sec.
> The actual test case might be okay other than that nit and a
> comment typo or two.
> 
> * 0003 might actually be okay.  I've not read it line-by-line,
> but it seems like it's implementing a sane solution and it's
> adequately commented.
> 
> * I'm inclined to reject 0004 out of hand, because I don't
> agree with what it's doing.  The purpose of the rmgrdesc
> functions is to show you what is in the WAL records, and
> everywhere else we interpret that as "show the verbatim,
> numeric field contents".  heapdesc.c, for example, doesn't
> attempt to look up the name of the table being operated on.
> 0004 isn't adhering to that style, and aside from being
> inconsistent I'm afraid that it's adding failure modes
> we don't want.

This patch again fails to apply (seemingly from the Perl namespace work on the
testcode), and needs a few updates as per the above review.

--
Daniel Gustafsson		https://vmware.com/

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.