Re: [GENERAL] currval and DISCARD ALL

From: Boszormenyi Zoltan <zb(at)cybertec(dot)at>
To: fabriziomello(at)gmail(dot)com
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Marko Kreen <markokr(at)gmail(dot)com>, Bruce Momjian <bruce(at)momjian(dot)us>, Adrian Klaver <adrian(dot)klaver(at)gmail(dot)com>, Nigel Heron <nigel(at)psycode(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [GENERAL] currval and DISCARD ALL
Date: 2013-08-19 19:02:21
Message-ID: 52126BBD.8050906@cybertec.at
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-general pgsql-hackers

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/

In response to

Responses

Browse pgsql-general by date

  From Date Subject
Next Message Boszormenyi Zoltan 2013-08-19 19:10:02 Re: [GENERAL] currval and DISCARD ALL
Previous Message Tom Lane 2013-08-19 19:01:34 Re: Create a deferrably-unique index

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2013-08-19 19:06:48 Re: danger of stats_temp_directory = /dev/shm
Previous Message Tom Lane 2013-08-19 18:56:05 Re: danger of stats_temp_directory = /dev/shm