Re: [GENERAL] currval and DISCARD ALL

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.

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