Horton

Thread

Commits

  1. pg_amcheck: Fix block number parsing on command line

  2. pg_resetwal: Improve numeric command-line argument parsing

  1. pg_amcheck: Fix block number parsing on command line

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-07-22T14:56:56Z

    It seems to me that when using the pg_amcheck --startblock and 
--endblock options on platforms where sizeof(long) == 4, you cannot 
specify higher block numbers (unless you do tricks with negative 
numbers).  The attached patch should fix this by using strtoul() instead 
of strtol().  I also tightened up the number scanning a bit in other 
ways, similar to the code in other frontend utilities.  I know some 
people have been working on tightening all this up.  Please check that 
it's up to speed.

  2. Re: pg_amcheck: Fix block number parsing on command line

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-07-22T16:18:51Z

    
> On Jul 22, 2021, at 7:56 AM, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
> 
> Please check that it's up to speed.
> <0001-pg_amcheck-Fix-block-number-parsing-on-command-line.patch>

This looks correct to me.  Thanks for the fix.

Your use of strtoul compares favorably to that in pg_resetwal in that you are checking errno and it is not.  The consequence is:

bin % ./pg_resetwal/pg_resetwal -e 1111111111111111111111111111111111111111111111111111
pg_resetwal: error: transaction ID epoch (-e) must not be -1
bin % ./pg_resetwal/pg_resetwal -e junkstring                                          
pg_resetwal: error: invalid argument for option -e
Try "pg_resetwal --help" for more information.

Unless people are relying on this behavior, I would think pg_resetwal should complain of an invalid argument for both of those, rather than complaining about -1.  That's not to do with this patch, but if we're tightening up the use of strtol in frontend tools, maybe we can use the identical logic in both places.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  3. Re: pg_amcheck: Fix block number parsing on command line

    Michael Paquier <michael@paquier.xyz> — 2021-07-23T05:11:49Z

    On Thu, Jul 22, 2021 at 04:56:56PM +0200, Peter Eisentraut wrote:
> It seems to me that when using the pg_amcheck --startblock and --endblock
> options on platforms where sizeof(long) == 4, you cannot specify higher
> block numbers (unless you do tricks with negative numbers).  The attached
> patch should fix this by using strtoul() instead of strtol().  I also
> tightened up the number scanning a bit in other ways, similar to the code in
> other frontend utilities.  I know some people have been working on
> tightening all this up.  Please check that it's up to speed.

Yeah, some work is happening to tighten all that.  Saying that, the
first round of review I did for the option parsing is not involving
option types other than int32, so the block options of pg_amcheck are
not changing for now, and what you are suggesting here is fine for the
moment for 14~.  Still, note that I am planning to change that on HEAD
with an option parsing API for int64 that could be used for block
numbers, eliminating for example the 4 translatable strings for the
code being changed here.
--
Michael

  4. Re: pg_amcheck: Fix block number parsing on command line

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-08-20T09:08:26Z

    On 22.07.21 18:18, Mark Dilger wrote:
>> On Jul 22, 2021, at 7:56 AM, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>>
>> Please check that it's up to speed.
>> <0001-pg_amcheck-Fix-block-number-parsing-on-command-line.patch>
> 
> This looks correct to me.  Thanks for the fix.

Committed this to 14 and master.

> Your use of strtoul compares favorably to that in pg_resetwal in that you are checking errno and it is not.  The consequence is:
> 
> bin % ./pg_resetwal/pg_resetwal -e 1111111111111111111111111111111111111111111111111111
> pg_resetwal: error: transaction ID epoch (-e) must not be -1
> bin % ./pg_resetwal/pg_resetwal -e junkstring
> pg_resetwal: error: invalid argument for option -e
> Try "pg_resetwal --help" for more information.
> 
> Unless people are relying on this behavior, I would think pg_resetwal should complain of an invalid argument for both of those, rather than complaining about -1.  That's not to do with this patch, but if we're tightening up the use of strtol in frontend tools, maybe we can use the identical logic in both places.

Committed a fix for this to master.