> 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/