Re: Review of pg_archivecleanup -x option patch

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

On 02/01/2012 07:53 AM, Alex Shulgin wrote:
> 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.

Going full-on automatic with detecting the suffix seems like it might do
the wrong thing when presented with a strange extension. I know a lot
of the time I use pg_archivecleanup is inside scripts that tend to get
setup and then ignored. If one of those has a bug, or unexpected data
appears in the archive, I'd rather see this sort of major, obvious
failure--not an attempt to sort things out that might make a bad decision.

The smaller step of automatically stripping the specified suffix from
the input file name, when it matches the one we've told the program to
expect, seems like a usability improvement unlikely to lead to bad
things though. I've certainly made the mistake you've shown when using
the patched version of the program myself, just didn't think about
catching and correcting it myself before. We can rev this to add that
feature and resubmit easily enough, will turn that around soon.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

On Sun, Feb 12, 2012 at 9:19 PM, Greg Smith <greg(at)2ndquadrant(dot)com> wrote:
> The smaller step of automatically stripping the specified suffix from the
> input file name, when it matches the one we've told the program to expect,
> seems like a usability improvement unlikely to lead to bad things though.
>  I've certainly made the mistake you've shown when using the patched version
> of the program myself, just didn't think about catching and correcting it
> myself before.  We can rev this to add that feature and resubmit easily
> enough, will turn that around soon.

This change seems plenty small enough to slip in even at this late
date, but I guess we're lacking a new version of the patch.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Mar 8, 2012 at 8:25 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Sun, Feb 12, 2012 at 9:19 PM, Greg Smith <greg(at)2ndquadrant(dot)com> wrote:
>> The smaller step of automatically stripping the specified suffix from the
>> input file name, when it matches the one we've told the program to expect,
>> seems like a usability improvement unlikely to lead to bad things though.
>>  I've certainly made the mistake you've shown when using the patched version
>> of the program myself, just didn't think about catching and correcting it
>> myself before.  We can rev this to add that feature and resubmit easily
>> enough, will turn that around soon.
>
> This change seems plenty small enough to slip in even at this late
> date, but I guess we're lacking a new version of the patch.
>

Sorry, here's the patch rebased and with the suggestion from Alex.
Which reminds me, I never thank him for the review (shame on me) :D

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

On Fri, Mar 9, 2012 at 12:46 AM, Jaime Casanova <jaime(at)2ndquadrant(dot)com> wrote:
> On Thu, Mar 8, 2012 at 8:25 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Sun, Feb 12, 2012 at 9:19 PM, Greg Smith <greg(at)2ndquadrant(dot)com> wrote:
>>> The smaller step of automatically stripping the specified suffix from the
>>> input file name, when it matches the one we've told the program to expect,
>>> seems like a usability improvement unlikely to lead to bad things though.
>>>  I've certainly made the mistake you've shown when using the patched version
>>> of the program myself, just didn't think about catching and correcting it
>>> myself before.  We can rev this to add that feature and resubmit easily
>>> enough, will turn that around soon.
>>
>> This change seems plenty small enough to slip in even at this late
>> date, but I guess we're lacking a new version of the patch.
>>
>
> Sorry, here's the patch rebased and with the suggestion from Alex.
> Which reminds me, I never thank him for the review (shame on me) :D
>

with the patch this time

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

On Fri, Mar 9, 2012 at 12:47 AM, Jaime Casanova <jaime(at)2ndquadrant(dot)com> wrote:
>> Sorry, here's the patch rebased and with the suggestion from Alex.
>> Which reminds me, I never thank him for the review (shame on me) :D
>
> with the patch this time

This may be a stupid idea, but it seems to me that it might be better
to dispense with all of the logic in here to detect whether the file
name is still going to be long enough after chomping the extension. I
feel like that's just making things complicated. I assume the
extensions we're thinking people will want to strip here are things
like ".gz", in which case there should be no confusion; and if
someone's dumb enough to use an extension like "0" (with no dot or
anything) but only sometimes then I think they deserve what they get
(viz: errors). See attached (only lightly tested) patch for what I'm
thinking of.

Also, I'm wondering about this warning in the documentation:

+ extension added by the compression program. Note that the
+ <filename>.backup</> file name passed to the program should not
+ include the extension.

IIUC, the latest change you made makes that warning obsolete, no?

[rhaas pgsql]$ contrib/pg_archivecleanup/pg_archivecleanup -d -x .gz .
000000010000000000000010.00000020.backup.gz
pg_archivecleanup: keep WAL file "./000000010000000000000010" and later

Thoughts?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Mar 9, 2012 at 9:07 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Mar 9, 2012 at 12:47 AM, Jaime Casanova <jaime(at)2ndquadrant(dot)com> wrote:
>>> Sorry, here's the patch rebased and with the suggestion from Alex.
>>> Which reminds me, I never thank him for the review (shame on me) :D
>>
>> with the patch this time
>
> This may be a stupid idea, but it seems to me that it might be better
> to dispense with all of the logic in here to detect whether the file
> name is still going to be long enough after chomping the extension.  I
> feel like that's just making things complicated.

while i like the idea of separating the logic, i don't like the results:

for example i tried this (notice that i forgot the point), and it just
says nothing (not even that the file with the extension but without
the point doesn't exist). that's why we were checking that the length
matches

$ ./pg_archivecleanup -x "bz2" /tmp 000000010000000100000058
$

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

On Sat, Mar 10, 2012 at 2:38 PM, Jaime Casanova <jaime(at)2ndquadrant(dot)com> wrote:
> On Fri, Mar 9, 2012 at 9:07 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Fri, Mar 9, 2012 at 12:47 AM, Jaime Casanova <jaime(at)2ndquadrant(dot)com> wrote:
>>>> Sorry, here's the patch rebased and with the suggestion from Alex.
>>>> Which reminds me, I never thank him for the review (shame on me) :D
>>>
>>> with the patch this time
>>
>> This may be a stupid idea, but it seems to me that it might be better
>> to dispense with all of the logic in here to detect whether the file
>> name is still going to be long enough after chomping the extension.  I
>> feel like that's just making things complicated.
>
> while i like the idea of separating the logic, i don't like the results:
>
> for example i tried this (notice that i forgot the point), and it just
> says nothing (not even that the file with the extension but without
> the point doesn't exist). that's why we were checking that the length
> matches
>
> $ ./pg_archivecleanup -x "bz2" /tmp 000000010000000100000058
> $
>

i would add this in the middle of the TrimExtension() function:

if ((flen - (elen+1) != XLOG_DATA_FNAME_LEN && flen - (elen+1) !=
XLOG_BACKUP_FNAME_LEN) &&
(flen != XLOG_DATA_FNAME_LEN && flen != XLOG_BACKUP_FNAME_LEN))
{
fprintf(stderr, "%s: invalid filename input\n", progname);
fprintf(stderr, "Try \"%s --help\" for more information.\n", progname);
exit(2);
}

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

On Sat, Mar 10, 2012 at 2:38 PM, Jaime Casanova <jaime(at)2ndquadrant(dot)com> wrote:
> On Fri, Mar 9, 2012 at 9:07 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Fri, Mar 9, 2012 at 12:47 AM, Jaime Casanova <jaime(at)2ndquadrant(dot)com> wrote:
>>>> Sorry, here's the patch rebased and with the suggestion from Alex.
>>>> Which reminds me, I never thank him for the review (shame on me) :D
>>>
>>> with the patch this time
>>
>> This may be a stupid idea, but it seems to me that it might be better
>> to dispense with all of the logic in here to detect whether the file
>> name is still going to be long enough after chomping the extension.  I
>> feel like that's just making things complicated.
>
> while i like the idea of separating the logic, i don't like the results:
>
> for example i tried this (notice that i forgot the point), and it just
> says nothing (not even that the file with the extension but without
> the point doesn't exist). that's why we were checking that the length
> matches
>
> $ ./pg_archivecleanup -x "bz2" /tmp 000000010000000100000058

Hmm, but I thought that the idea was that the extension was optional.
Perhaps I'm missing something but I don't think the previous patch
will complain about that either; or at least I don't see why the
behavior should be any different.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sun, Mar 11, 2012 at 9:28 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Sat, Mar 10, 2012 at 2:38 PM, Jaime Casanova <jaime(at)2ndquadrant(dot)com> wrote:
>> On Fri, Mar 9, 2012 at 9:07 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Fri, Mar 9, 2012 at 12:47 AM, Jaime Casanova <jaime(at)2ndquadrant(dot)com> wrote:
>>>>> Sorry, here's the patch rebased and with the suggestion from Alex.
>>>>> Which reminds me, I never thank him for the review (shame on me) :D
>>>>
>>>> with the patch this time
>>>
>>> This may be a stupid idea, but it seems to me that it might be better
>>> to dispense with all of the logic in here to detect whether the file
>>> name is still going to be long enough after chomping the extension.  I
>>> feel like that's just making things complicated.
>>
>> while i like the idea of separating the logic, i don't like the results:
>>
>> for example i tried this (notice that i forgot the point), and it just
>> says nothing (not even that the file with the extension but without
>> the point doesn't exist). that's why we were checking that the length
>> matches
>>
>> $ ./pg_archivecleanup -x "bz2" /tmp 000000010000000100000058
>
> Hmm, but I thought that the idea was that the extension was optional.
> Perhaps I'm missing something but I don't think the previous patch
> will complain about that either; or at least I don't see why the
> behavior should be any different.

Can someone enlighten me on this point?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Mar 28, 2012 at 8:47 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>>
>>> $ ./pg_archivecleanup -x "bz2" /tmp 000000010000000100000058
>>
>> Hmm, but I thought that the idea was that the extension was optional.
>> Perhaps I'm missing something but I don't think the previous patch
>> will complain about that either; or at least I don't see why the
>> behavior should be any different.
>
> Can someone enlighten me on this point?
>

mmm! you're right... it's not complaining either... i was sure it was...
and i'm not sure i want to contor things for that...

so, just forget my last mail about that... your refactor is just fine for me

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

On Wed, Mar 28, 2012 at 12:13 PM, Jaime Casanova <jaime(at)2ndquadrant(dot)com> wrote:
> On Wed, Mar 28, 2012 at 8:47 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>>>
>>>> $ ./pg_archivecleanup -x "bz2" /tmp 000000010000000100000058
>>>
>>> Hmm, but I thought that the idea was that the extension was optional.
>>> Perhaps I'm missing something but I don't think the previous patch
>>> will complain about that either; or at least I don't see why the
>>> behavior should be any different.
>>
>> Can someone enlighten me on this point?
>>
>
> mmm! you're right... it's not complaining either... i was sure it was...
> and i'm not sure i want to contor things for that...
>
> so, just forget my last mail about that... your refactor is just fine for me

OK, committed that way.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company