> On 24 Sep 2021, at 20:14, Tom Lane <t...@sss.pgh.pa.us> wrote: > > Robert Haas <robertmh...@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/
