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