I like 0007 quite a bit and am inclined to commit it soon, as it doesn't depend on the earlier patches. But:
- I think the residual comment in processSQLNamePattern beginning with "Note:" could use some wordsmithing to account for the new structure of things -- maybe just "this pass" -> "this function". - I suggest changing initializations like maxbuf = buf + 2 to maxbuf = &buf[2] for clarity. Regarding 0001: - My preference would be to dump on_exit_nicely_final() and just rely on order of registration. - I'm not entirely sure it's a good ideal to expose something named fatal() like this, because that's a fairly short and general name. On the other hand, it's pretty descriptive and it's not clear why someone including exit_utils.h would want any other definitiion. I guess we can always change it later if it proves to be problematic; it's got a lot of callers and I guess there's no point in churning the code without a clear reason. - I don't quite see why we need this at all. Like, exit_nicely() is a pg_dump-ism. It would make sense to centralize it if we were going to use it for pg_amcheck, but you don't. If you were going to, you'd need to adapt 0003 to use exit_nicely() instead of exit(), but you don't, nor do you add any other new calls to exit_nicely() anywhere, except for one in 0002. That makes the PGresultHandler stuff depend on exit_nicely(), which might be important if you were going to refactor pg_dump to use that abstraction, but you don't. I'm not opposed to the idea of centralized exit processing for frontend utilities; indeed, it seems like a good idea. But this doesn't seem to get us there. AFAICS it just entangles pg_dump with pg_amcheck unnecessarily in a way that doesn't really benefit either of them. Regarding 0002: - I don't think this is separately committable because it adds an abstraction but not any uses of that abstraction to demonstrate that it's actually any good. Perhaps it should just be merged into 0005, and even into parallel_slot.h vs. having its own header. I'm not really sure about that, though - Is this really much of an abstraction layer? Like, how generic can this be when the argument list includes ExecStatusType expected_status and int expected_ntups? - The logic seems to be very similar to some of the stuff that you move around in 0003, like executeQuery() and executeCommand(), but it doesn't get unified. I'm not necessarily saying it should be, but it's weird to do all this refactoring and end up with something that still looks this 0003, 0004, and 0006 look pretty boring; they are just moving code around. Is there any point in splitting the code from 0003 across two files? Maybe it's fine. If I run pg_amcheck --all -j4 do I get a serialization boundary across databases? Like, I have to completely finish db1 before I can go onto db2, even though maybe only one worker is still busy with it? -- Robert Haas EDB: http://www.enterprisedb.com
