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.
regards, tom lane
