|From:||Daniel Gustafsson <daniel(at)yesql(dot)se>|
|To:||Paul Guo <guopa(at)vmware(dot)com>|
|Cc:||Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>, Ibrar Ahmed <ibrar(dot)ahmad(at)gmail(dot)com>, Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com>, Anastasia Lubennikova <a(dot)lubennikova(at)postgrespro(dot)ru>, Asim Praveen <pasim(at)vmware(dot)com>, "thomas(dot)munro(at)gmail(dot)com" <thomas(dot)munro(at)gmail(dot)com>, "horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp" <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: standby recovery fails (tablespace related) (tentative patch and discussion)|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
> On 24 Sep 2021, at 20:14, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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
> * 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/
|Next Message||Daniel Gustafsson||2021-11-04 12:38:03||Re: pgbench logging broken by time logic changes|
|Previous Message||Daniel Gustafsson||2021-11-04 12:24:21||Re: should INSERT SELECT use a BulkInsertState?|