On Thu, Mar 19, 2015 at 6:42 AM, Asif Naeem wrote:
> I do agree, pg_ctl refactoring is making current patch more complicated,
> eventually we can come up with robust patch that include pg_ctl but result
> patch could be big and touching lot of areas, I doubt that if such
> complicated and big patch going to make its way into the repository. It
> seems appropriate to do incremental work, we can refactor pg_ctl changes as
> next effort. PFA update patch, it removes pg_ctl related code changes.
> Please do let me know if I missed something. Thanks.
After looking closely at those things, I agree with you: having some
code duplicated in two places instead of five is still a win... Now, I
think having this declaration of write_stderr in restricted_token.c is
confusing:
+#define write_stderr(fmt, ...) fprintf(stderr, fmt, __VA_ARGS__)
We could have bad surprises in the future if there is some work to
link pg_ctl with the stuff of restricted_token.c. Hence, could you
replace that with plain calls to fprintf(stderr, ...)? That's as well
what the other files of src/common are doing, so it would make the new
code more consistent with the rest.
[nitpicking]
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -30,6 +30,7 @@
#endif
#include "common/username.h"
+#include "common/restricted_token.h"
Alphabetical order of headers.
[/nitpicking]
Except those two small things, well I guess that's it for this patch.
The stuff for write_stderr may need a TODO item, but I think that even
with that we are going to finish by fixing the call to isatty that
looks indeed strange...
For the backpatching, the patches sent previously here (=>
http://www.postgresql.org/message-id/CAEB4t-OHNE95=n5U4ySsYkWipQsWeQuTBSJkaYJ63_1VzkzkhA@mail.gmail.com)
are fine IMO. They simply consist of a copy of what is done in
initdb.c. Now, perhaps we had better apply the patch duplicating the
logic to all branches, including HEAD, first, see what the buildfarm
says, and then finish wrapping up the refactoring patch.
Regards,
--
Michael