public inbox for [email protected]
help / color / mirror / Atom feedFrom: Tom Lane <[email protected]>
To: Robert Haas <[email protected]>
Cc: Paul Guo <[email protected]>
Cc: Ibrar Ahmed <[email protected]>
Cc: Kyotaro Horiguchi <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Fujii Masao <[email protected]>
Cc: Anastasia Lubennikova <[email protected]>
Cc: Asim Praveen <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: standby recovery fails (tablespace related) (tentative patch and discussion)
Date: Fri, 24 Sep 2021 14:14:52 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <CA+TgmoY3oqC3Bh_Q-Qs280WU9v5kNWNtTTBQJ2vG1e68nQ78bg@mail.gmail.com>
References: <[email protected]>
<[email protected]>
<[email protected]>
<CALtqXTejpfCX7UF0D-OFUW6a5-ep3iqD2fj5xgy18yDqk1Hw4w@mail.gmail.com>
<SA1PR05MB85821DEC48A6841277BDBF57C3F29@SA1PR05MB8582.namprd05.prod.outlook.com>
<CA+TgmoY3oqC3Bh_Q-Qs280WU9v5kNWNtTTBQJ2vG1e68nQ78bg@mail.gmail.com>
Robert Haas <[email protected]> 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.
regards, tom lane
view thread (109+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: standby recovery fails (tablespace related) (tentative patch and discussion)
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox