Re: [GENERAL] currval and DISCARD ALL - Mailing list pgsql-hackers

On Fri, Apr 19, 2013 at 11:12 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Fri, Apr 19, 2013 at 10:05 AM, Fabrízio de Royes Mello
<fabriziomello@gmail.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.

* 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/