Review of pg_archivecleanup -x option patch - Mailing list pgsql-hackers

From Alex Shulgin
Subject Review of pg_archivecleanup -x option patch
Date
Msg-id 87obtin290.fsf@commandprompt.com
Whole thread Raw
Responses Re: Review of pg_archivecleanup -x option patch  (Greg Smith <greg@2ndQuadrant.com>)
List pgsql-hackers

Hello,

This is my first patch review ever, so please bear with me.

The patch[1] is in the context diff format and applies cleanly to
current git HEAD.  Documentation is updated by the patch.

The code does implement what's the patch is supposed to do.

Do we want that?  According to Greg's original mail, yes.

One problem I've found while trying the example workflow is this:

~/local/postgresql/HEAD$ ./bin/pg_archivecleanup -d -x .gz data/archive/ 000000010000000000000002.00000020.backup.gz
pg_archivecleanup: invalid filename input
Try "pg_archivecleanup --help" for more information.

I think we could accept the suffixed WAL filename and strip the suffix;
another idea is to actually detect the suffix (if any) from the last
command line argument, thus rendering the -x option unnecessary.

By the way, I for one would use the word 'suffix' instead of 'extension'
which sounds like some MS-DOS thingy, but after briefly grepping the
docs I can see that both words are used in this context already (and the
ratio is not in favor of my choice of wording.)

Other than that the patch looks good.

--
Alex

[1] http://archives.postgresql.org/pgsql-hackers/2011-02/msg00547.php


pgsql-hackers by date:

Previous
From: Ashutosh Bapat
Date:
Subject: Re: Confusing EXPLAIN output in case of inherited tables
Next
From: Marko Kreen
Date:
Subject: libpq: fix sslcompression leak