Thread: [PATCH] pg_ctl should not truncate command lines at 1024 characters

Hello,

Lacking a tool to edit postgresql.conf programmatically, people resort 
to passing cluster options on the command line. While passing all 
non-default options in this way may sound like an abuse of the feature, 
IMHO pg_ctl should not blindly truncate generated command lines at 
MAXPGPATH (1024 characters) and then run that, resulting in:

/bin/sh: Syntax error: end of file unexpected (expecting word)
pg_ctl: could not start server
Examine the log output.

The attached patch tries to fix it in the least intrusive way.

While we're at it, is it supposed that pg_ctl is a very short-lived 
process and is therefore allowed to leak memory? I've noticed some 
places where I would like to add a free() call.

-- Ph.

On 2021-09-03 00:36, Ranier Vilela wrote:

> The msvc docs says that limit for the command line is 32,767 
> characters,
> while ok for me, I think if not it would be better to check this limit?

Well, it's ARG_MAX in POSIX, and ARG_MAX is defined as 256K in Darwin, 
512K in FreeBSD, 128K in Linux; _POSIX_ARG_MAX is defined as 4096 on all 
three platforms. Windows may differ too. Anyways, allocating even 128K 
in precious stack space is too much, that's why I suggest to use 
psprintf(). As for checking any hard limit, I don't think it would have 
much value - somehow we got the original command line, thus it is 
supported by the system, so we can just pass it on.

-- Ph.

Phil Krylov <phil@krylov.eu> writes:
> IMHO pg_ctl should not blindly truncate generated command lines at 
> MAXPGPATH (1024 characters) and then run that, resulting in:

Fair enough.

> The attached patch tries to fix it in the least intrusive way.

Seems reasonable.  We didn't have psprintf when this code was written,
but now that we do, it's hardly any more complicated to do it without
the length restriction.

> While we're at it, is it supposed that pg_ctl is a very short-lived 
> process and is therefore allowed to leak memory? I've noticed some 
> places where I would like to add a free() call.

I think that these free() calls you propose to add are a complete
waste of code space.  Certainly a free() right before an exit() call
is that; if anything, it's *delaying* recycling the memory space for
some useful purpose.  But no part of pg_ctl runs long enough for it
to be worth worrying about small leaks.

I do not find your proposed test case to be a useful expenditure
of test cycles, either.  If it ever fails, we'd learn nothing,
except that that particular platform has a surprisingly small
command line length limit.

            regards, tom lane

On 2021-09-03 02:09, Tom Lane wrote:
> I think that these free() calls you propose to add are a complete
> waste of code space.  Certainly a free() right before an exit() call
> is that; if anything, it's *delaying* recycling the memory space for
> some useful purpose.  But no part of pg_ctl runs long enough for it
> to be worth worrying about small leaks.

OK, I have removed the free() before exit().

> I do not find your proposed test case to be a useful expenditure
> of test cycles, either.  If it ever fails, we'd learn nothing,
> except that that particular platform has a surprisingly small
> command line length limit.

Hmm, it's a test case that fails with the current code and stops failing 
with my fix, so I've put it there to show the problem. But, truly, it 
does not bring much value after the fix is applied.

Attaching the new version, with the test case and free-before-exit 
removed.

-- Ph.

Phil Krylov <phil@krylov.eu> writes:
> Attaching the new version, with the test case and free-before-exit 
> removed.

Pushed with minor cosmetic adjustments.  Thanks for the patch!

            regards, tom lane