Re: [GENERAL] currval and DISCARD ALL

On 19-08-2013 16:10, Boszormenyi Zoltan wrote:
>>
>> I am reviewing your patch.
>>

Thanks...


>> * Is the patch in a patch format which has context? (eg: context diff
>> format)
>>
>> Yes.
>>
>> * Does it apply cleanly to the current git master?
>>
>> Almost. No rejects, no fuzz, only offset for some files.
>>
>> * Does it include reasonable tests, necessary doc patches, etc?
>>
>> Documentation, yes. Tests, no.
>>
>> * Does the patch actually implement what it's supposed to do?
>>
>> Yes.
>>
>> * Do we want that?
>>
>> Yes.
>>
>> * Do we already have it?
>>
>> No.
>>
>> * Does it follow SQL spec, or the community-agreed behavior?
>>
>> The SQL standard doesn't have DISCARD.
>> Otherwise the behaviour is obvious.
>>
>> * Does it include pg_dump support (if applicable)?
>>
>> n/a
>>
>> * Are there dangers?
>>
>> It changes applications' assumptions slightly but takes the
>> behaviour closer to the wording of the documentation.
>>
>> * Have all the bases been covered?
>>
>> Yes.
>>
>> * Does the feature work as advertised?
>>
>> Yes.
>>
>> * Are there corner cases the author has failed to consider?
>>
>> No.
>>
>> * Are there any assertion failures or crashes?
>>
>> No.
>>
>> * Does the patch slow down simple tests?
>>
>> No.
>>
>> * If it claims to improve performance, does it?
>>
>> n/a
>>
>> * Does it slow down other things?
>>
>> No.
>>
>> * Does it follow the project coding guidelines?
>>
>> Yes.
>>
>> Maybe a little stylistic comment:
>>
>> +void
>> +ReleaseSequenceCaches()
>> +{
>> +       SeqTableData *ptr = seqtab;
>> +       SeqTableData *tmp = NULL;
>> +
>> +       while (ptr != NULL)
>> +       {
>> +               tmp = ptr;
>> +               ptr = ptr->next;
>> +               free(tmp);
>> +       }
>> +
>> +       seqtab = NULL;
>> +}
>>
>> I would rename the variables to "seq" and "next" from
>> "ptr" and "tmp", respectively, to make them even more
>> obvious. This looks a little better:
>>
>> +void
>> +ReleaseSequenceCaches()
>> +{
>> +       SeqTableData *seq = seqtab;
>> +       SeqTableData *next;
>> +
>> +       while (seq)
>> +       {
>> +               next = seq->next;
>> +               free(seq);
>> +               seq = next;
>> +       }
>> +
>> +       seqtab = NULL;
>> +}
>>

Done!


>> * Are there portability issues?
>>
>> No.
>>
>> * Will it work on Windows/BSD etc?
>>
>> It should. There are no extra system calls.
>>
>> * Are the comments sufficient and accurate?
>>
>> The feature needs very little code which is downright obvious.
>> There are no comments but I don't think the code needs it.
>>
>> * Does it do what it says, correctly?
>>
>> Yes.
>
> I lied.
>
> There is one little problem. There is no command tag
> reported for DISCARD SEQUENCES:
>
> zozo=# create sequence s1;
> CREATE SEQUENCE
> zozo=# select nextval('s1');
>   nextval
> ---------
>         1
> (1 row)
>
> zozo=# select currval('s1');
>   currval
> ---------
>         1
> (1 row)
>
> zozo=# discard all;
> DISCARD ALL
> zozo=# discard sequences;
> ???
> zozo=# select currval('s1');
> ERROR:  currval of sequence "s1" is not yet defined in this session
>

Fixed!


>>
>> * Does it produce compiler warnings?
>>
>> Only one:
>>
>> src/backend/commands/sequence.c should have
>>
>> #include <commands/sequence.h>
>>
>> because of this:
>>
>> sequence.c:1608:1: warning: no previous prototype for
>> ‘ReleaseSequenceCaches’ [-Wmissing-prototypes]
>>  ReleaseSequenceCaches()
>>  ^
>>

Fixed!


>> * Can you make it crash?
>>
>> No.
>>
>> * Is everything done in a way that fits together coherently with other
>> features/modules?
>>
>> Yes.
>>
>> * Are there interdependencies that can cause problems?
>>
>> I don't think so.
>>


The attached patch fix the items reviewed by you.

Regards,

--
    Fabrízio de Royes Mello         Timbira - http://www.timbira.com.br/
    PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento