Review of pg_archivecleanup -x option patch

From: Alex Shulgin <ash(at)commandprompt(dot)com>
To: pgsql-hackers(at)postgresql(dot)org
Subject: Review of pg_archivecleanup -x option patch
Date: 2012-02-01 12:53:47
Message-ID: 87obtin290.fsf@commandprompt.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: 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

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Marko Kreen 2012-02-01 12:56:44 libpq: fix sslcompression leak
Previous Message Ashutosh Bapat 2012-02-01 12:38:06 Re: Confusing EXPLAIN output in case of inherited tables