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

Em qui., 2 de set. de 2021 às 18:36, Phil Krylov <phil@krylov.eu> escreveu:

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:

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?

/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.

+1 to add free.

regards,
Ranier Vilela

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