Re: Review of pg_archivecleanup -x option patch

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