Re: [GENERAL] currval and DISCARD ALL

2013-08-19 21:02 keltezéssel, Boszormenyi Zoltan írta:
> Hi,
>
> 2013-04-19 16:58 keltezéssel, Fabrízio de Royes Mello írta:
>>
>> On Fri, Apr 19, 2013 at 11:12 AM, Robert Haas <robertmhaas(at)gmail(dot)com
>> <mailto:robertmhaas(at)gmail(dot)com>> wrote:
>>
>> On Fri, Apr 19, 2013 at 10:05 AM, Fabrízio de Royes Mello
>> <fabriziomello(at)gmail(dot)com <mailto:fabriziomello(at)gmail(dot)com>> wrote:
>> > The attached wip patch do that and introduce a subcommand 'SEQUENCES', but
>> > if we decide to don't add a new subcommand to DISCARD, then its easier to
>> > modify the patch.
>>
>> This patch is quite wrong. It frees seqtab without clearing the
>> pointer, so the next reference will stomp on memory that may have been
>> reallocated. And it doesn't even free seqtab correctly, since it only
>> frees the first node in the linked list.
>>
>>
>> Ohh sorry... you're all right... I completely forgot to finish the
>> ReleaseSequenceCaches to transverse 'seqtab' linked list and free each node.
>>
>> The attached patch have this correct code.
>>
>> Regards,
>>
>> --
>> Fabrízio de Royes Mello
>> Consultoria/Coaching PostgreSQL
>> >> Blog sobre TI: http://fabriziomello.blogspot.com
>> >> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> >> Twitter: http://twitter.com/fabriziomello
>
> I am reviewing your patch.
>
> * 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;
> +}
>
> * 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

>
> * 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()
> ^
>
> * 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.
>
> Best regards,
> Zoltán Böszörményi
>
> --
> ----------------------------------
> Zoltán Böszörményi
> Cybertec Schönig & Schönig GmbH
> Gröhrmühlgasse 26
> A-2700 Wiener Neustadt, Austria
> Web:http://www.postgresql-support.de
> http://www.postgresql.at/

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/