|From:||Boszormenyi Zoltan <zb(at)cybertec(dot)at>|
|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|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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.
> 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)
* 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?
* Do we want that?
* Do we already have it?
* 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)?
* 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?
* Does the feature work as advertised?
* Are there corner cases the author has failed to consider?
* Are there any assertion failures or crashes?
* Does the patch slow down simple tests?
* If it claims to improve performance, does it?
* Does it slow down other things?
* Does it follow the project coding guidelines?
Maybe a little stylistic comment:
+ SeqTableData *ptr = seqtab;
+ SeqTableData *tmp = NULL;
+ while (ptr != NULL)
+ tmp = ptr;
+ ptr = ptr->next;
+ 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:
+ SeqTableData *seq = seqtab;
+ SeqTableData *next;
+ while (seq)
+ next = seq->next;
+ seq = next;
+ seqtab = NULL;
* Are there portability issues?
* 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?
* Does it produce compiler warnings?
src/backend/commands/sequence.c should have
because of this:
sequence.c:1608:1: warning: no previous prototype for 'ReleaseSequenceCaches'
* Can you make it crash?
* Is everything done in a way that fits together coherently with other features/modules?
* Are there interdependencies that can cause problems?
I don't think so.
|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|
|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|