Horton

Thread

Commits

  1. pg_regress: Save errno in emit_tap_output_v() and switch to %m

  2. Use printf's %m format instead of strerror(errno) in more places

  3. Add --copy-file-range option to pg_upgrade.

  1. Using the %m printf format more

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2024-03-06T19:11:19Z

    Hi hackers,

I just noticed that commit d93627bc added a bunch of pg_fatal() calls
using %s and strerror(errno), which could be written more concisely as
%m.  I'm assuming this was done because the surrounding code also uses
this pattern, and hadn't been changed to use %m when support for that
was added to snprintf.c to avoid backporting hazards.  However, that
support was in v12, which is now the oldest still-supported back branch,
so we can safely make that change.

The attached patch does so everywhere appropriate. One place where it's
not appropriate is the TAP-emitting functions in pg_regress, since those
call fprintf() and other potentially errno-modifying functions before
evaluating the format string.

- ilmari

  2. Re: Using the %m printf format more

    Michael Paquier <michael@paquier.xyz> — 2024-03-11T06:30:30Z

    On Wed, Mar 06, 2024 at 07:11:19PM +0000, Dagfinn Ilmari Mannsåker wrote:
> I just noticed that commit d93627bc added a bunch of pg_fatal() calls
> using %s and strerror(errno), which could be written more concisely as
> %m.  I'm assuming this was done because the surrounding code also uses
> this pattern, and hadn't been changed to use %m when support for that
> was added to snprintf.c to avoid backporting hazards.  However, that
> support was in v12, which is now the oldest still-supported back branch,
> so we can safely make that change.

Right.  This may still create some spurious conflicts, but that's
manageable for error strings.  The changes in your patch look OK.

> The attached patch does so everywhere appropriate. One place where it's
> not appropriate is the TAP-emitting functions in pg_regress, since those
> call fprintf()

I am not really following your argument with pg_regress.c and
fprintf().  d6c55de1f99a should make that possible even in the case of
emit_tap_output_v(), no? 

> and other potentially errno-modifying functions before
> evaluating the format string.

Sure.
--
Michael

  3. Re: Using the %m printf format more

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2024-03-11T11:19:16Z

    Michael Paquier <michael@paquier.xyz> writes:

> On Wed, Mar 06, 2024 at 07:11:19PM +0000, Dagfinn Ilmari Mannsåker wrote:
>
>> The attached patch does so everywhere appropriate. One place where it's
>> not appropriate is the TAP-emitting functions in pg_regress, since those
>> call fprintf()
>
> I am not really following your argument with pg_regress.c and
> fprintf().  d6c55de1f99a should make that possible even in the case of
> emit_tap_output_v(), no? 

The problem isn't that emit_tap_output_v() doesn't support %m, which it
does, but that it potentially calls fprintf() to output TAP protocol
elements such as "\n" and "# " before it calls vprintf(…, fmt, …), and
those calls might clobber errno.  An option is to make it save errno at
the start and restore it before the vprintf() calls, as in the second
attached patch.

>> and other potentially errno-modifying functions before
>> evaluating the format string.
>
> Sure.

On closer look, fprintf() is actually the only errno-clobbering function
it calls, I was just hedging my bets in that statement.

- ilmari

  4. Re: Using the %m printf format more

    Michael Paquier <michael@paquier.xyz> — 2024-03-12T01:22:18Z

    On Mon, Mar 11, 2024 at 11:19:16AM +0000, Dagfinn Ilmari Mannsåker wrote:
> On closer look, fprintf() is actually the only errno-clobbering function
> it calls, I was just hedging my bets in that statement.

This makes the whole simpler, so I'd be OK with that.  I am wondering
if others have opinions to offer about that.

I've applied v2-0001 for now, as it is worth on its own and it shaves
a bit of code.
--
Michael

  5. Re: Using the %m printf format more

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2024-03-13T12:24:08Z

    Michael Paquier <michael@paquier.xyz> writes:

> On Mon, Mar 11, 2024 at 11:19:16AM +0000, Dagfinn Ilmari Mannsåker wrote:
>> On closer look, fprintf() is actually the only errno-clobbering function
>> it calls, I was just hedging my bets in that statement.
>
> This makes the whole simpler, so I'd be OK with that.  I am wondering
> if others have opinions to offer about that.

If no one chimes in in the next couple of days I'll add it to the
commitfest so it doesn't get lost.

> I've applied v2-0001 for now, as it is worth on its own and it shaves
> a bit of code.

Thanks!

- ilmari

  6. Re: Using the %m printf format more

    Peter Eisentraut <peter@eisentraut.org> — 2024-03-13T13:33:52Z

    On 12.03.24 02:22, Michael Paquier wrote:
> On Mon, Mar 11, 2024 at 11:19:16AM +0000, Dagfinn Ilmari Mannsåker wrote:
>> On closer look, fprintf() is actually the only errno-clobbering function
>> it calls, I was just hedging my bets in that statement.
> 
> This makes the whole simpler, so I'd be OK with that.  I am wondering
> if others have opinions to offer about that.

The 0002 patch looks sensible.  It would be good to fix that, otherwise 
it could have some confusing outcomes in the future.

  7. Re: Using the %m printf format more

    Michael Paquier <michael@paquier.xyz> — 2024-03-14T07:04:50Z

    On Wed, Mar 13, 2024 at 02:33:52PM +0100, Peter Eisentraut wrote:
> The 0002 patch looks sensible.  It would be good to fix that, otherwise it
> could have some confusing outcomes in the future.

You mean if we begin to use %m in future callers of
emit_tap_output_v(), hypothetically?  That's a fair argument.
--
Michael

  8. Re: Using the %m printf format more

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2024-03-14T11:25:30Z

    Michael Paquier <michael@paquier.xyz> writes:

> On Wed, Mar 13, 2024 at 02:33:52PM +0100, Peter Eisentraut wrote:
>> The 0002 patch looks sensible.  It would be good to fix that, otherwise it
>> could have some confusing outcomes in the future.
>
> You mean if we begin to use %m in future callers of
> emit_tap_output_v(), hypothetically?  That's a fair argument.

Yeah, developers would rightfully expect to be able to use %m with
anything that takes a printf format string.  Case in point: when I was
first doing the conversion I did change the bail() and diag() calls in
pg_regress to %m, and only while I was preparing the patch for
submission did I think to check the actual implementation to see if it
was safe to do so.

The alternative would be to document that you can't use %m with these
functions, which is silly IMO, given how simple the fix is.

One minor improvement I can think of is to add a comment by the
save_errno declaration noting that it's needed in order to support %m.

- ilmari

  9. Re: Using the %m printf format more

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2024-03-22T13:58:24Z

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes:

> Michael Paquier <michael@paquier.xyz> writes:
>
>> On Wed, Mar 13, 2024 at 02:33:52PM +0100, Peter Eisentraut wrote:
>>> The 0002 patch looks sensible.  It would be good to fix that, otherwise it
>>> could have some confusing outcomes in the future.
>>
>> You mean if we begin to use %m in future callers of
>> emit_tap_output_v(), hypothetically?  That's a fair argument.
>
> Yeah, developers would rightfully expect to be able to use %m with
> anything that takes a printf format string.  Case in point: when I was
> first doing the conversion I did change the bail() and diag() calls in
> pg_regress to %m, and only while I was preparing the patch for
> submission did I think to check the actual implementation to see if it
> was safe to do so.
>
> The alternative would be to document that you can't use %m with these
> functions, which is silly IMO, given how simple the fix is.
>
> One minor improvement I can think of is to add a comment by the
> save_errno declaration noting that it's needed in order to support %m.

Here's an updated patch that adds such a comment.  I'll add it to the
commitfest later today unless someone commits it before then.

> - ilmari

  10. Re: Using the %m printf format more

    Michael Paquier <michael@paquier.xyz> — 2024-04-04T02:35:58Z

    On Fri, Mar 22, 2024 at 01:58:24PM +0000, Dagfinn Ilmari Mannsåker wrote:
> Here's an updated patch that adds such a comment.  I'll add it to the
> commitfest later today unless someone commits it before then.

I see no problem to do that now rather than later.  So, done to make
pg_regress able to use %m.
--
Michael

  11. Re: Using the %m printf format more

    Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2024-04-04T07:44:25Z

    On Thu, 4 Apr 2024, at 03:35, Michael Paquier wrote:
> On Fri, Mar 22, 2024 at 01:58:24PM +0000, Dagfinn Ilmari Mannsåker wrote:
>> Here's an updated patch that adds such a comment.  I'll add it to the
>> commitfest later today unless someone commits it before then.
>
> I see no problem to do that now rather than later.  So, done to make
> pg_regress able to use %m.

Thanks!

-- 
- ilmari