Обсуждение: Re: [GENERAL] currval and DISCARD ALL
On Tue, Apr 16, 2013 at 12:13:48PM -0700, Adrian Klaver wrote: > On 04/16/2013 08:07 AM, Nigel Heron wrote: > > > >On 04/15/2013 05:57 PM, Adrian Klaver wrote: > >>On 04/15/2013 02:42 PM, Nigel Heron wrote: > >>>Hi, > >>>is there a way to clear the session state of sequence values fetched by > >>>currval(regclass)? "DISCARD ALL" doesn't seem to do it. > >>> > ><snip> > >>Might want to take a look at: > >> > >>http://www.depesz.com/2012/12/02/what-is-the-point-of-bouncing/ > >> > >>for some hints on dealing with sequences and pgBouncer. > >> > > > >thanks, I read it (his blogs are always interesting!). I'm not disputing > >that calling currval() at the wrong time is a bad idea. > >I'm just wondering why DISCARD ALL clears everything but this? > > Well per the docs: > > http://www.postgresql.org/docs/9.2/interactive/sql-discard.html > > DISCARD ALL > > is equivalent to: > > SET SESSION AUTHORIZATION DEFAULT; > RESET ALL; > DEALLOCATE ALL; > CLOSE ALL; > UNLISTEN *; > SELECT pg_advisory_unlock_all(); > DISCARD PLANS; > DISCARD TEMP; > > AFAIK, none of the above affect sequences. I think his point is why don't we clear currval() on DISCARD ALL? I can't think of a good reason we don't. He is saying currval() should throw an error after DISCARD ALL: test=> SELECT currval('seq');ERROR: currval of sequence "seq" is not yet defined in this session I have moved this thead to hackers to get comments. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Bruce Momjian <bruce@momjian.us> writes: > I think his point is why don't we clear currval() on DISCARD ALL? I > can't think of a good reason we don't. Because we'd have to invent a new suboperation DISCARD SEQUENCES, for one thing, in order to be consistent. I'd rather ask why it's important that we should throw away such state. It doesn't seem to me to be important enough to justify a new subcommand. Or, if you'd rather a more direct answer: wanting this sounds like evidence of bad application design. Why is your app dependent on getting failures from currval, and isn't there a better way to do it? regards, tom lane
On Tue, Apr 16, 2013 at 6:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
--
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
[...]
Or, if you'd rather a more direct answer: wanting this sounds like
evidence of bad application design. Why is your app dependent on
getting failures from currval, and isn't there a better way to do it?
The sequence cache (seqtab) is used per each backend, so if we use a connection pooler (like pgbouncer in session mode) between our app and postgres we can get a wrong value from 'currval' because the backend isn't completely clean.
This isn't it a good reason to implement this feature?
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
On Tue, Apr 16, 2013 at 05:09:19PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > I think his point is why don't we clear currval() on DISCARD ALL? I > > can't think of a good reason we don't. > > Because we'd have to invent a new suboperation DISCARD SEQUENCES, > for one thing, in order to be consistent. I'd rather ask why it's > important that we should throw away such state. It doesn't seem to > me to be important enough to justify a new subcommand. "consistency" is a philosophical thing. Practical reason for subcommands is possibility to have partial reset for special situations, pooling or otherwise. But such usage seems rather rare in real life. If the sequences are not worth subcommand, then let's not give them subcommand and just wait until someone comes with actual reason to have one. But currval() is quite noticeable thing that DISCARD ALL should clear. > Or, if you'd rather a more direct answer: wanting this sounds like > evidence of bad application design. Why is your app dependent on > getting failures from currval, and isn't there a better way to do it? It just does not sound like, but thats exactly the request - because DISCARD ALL leaves user-visible state around, it's hard to fix application that depends on broken assumptions. In fact, it was surprise to me that currval() works across transactions. My alternative proposal would be to get rid of such silly behaviour... -- marko
Marko Kreen <markokr@gmail.com> writes: > On Tue, Apr 16, 2013 at 05:09:19PM -0400, Tom Lane wrote: >> Bruce Momjian <bruce@momjian.us> writes: >>> I think his point is why don't we clear currval() on DISCARD ALL? I >>> can't think of a good reason we don't. >> Because we'd have to invent a new suboperation DISCARD SEQUENCES, >> for one thing, in order to be consistent. I'd rather ask why it's >> important that we should throw away such state. It doesn't seem to >> me to be important enough to justify a new subcommand. > "consistency" is a philosophical thing. No, it's a critical tool in complexity management. When you're dealing with systems as complicated as a database, every little non-orthogonal detail adds up. DISCARD ALL has a clear definition in terms of simpler commands, and it's going to stay that way. Either this is worth a subcommand, or it's not worth worrying about at all. > But currval() is quite noticeable thing that DISCARD ALL should clear. If it were as obvious and noticeable as all that, somebody would have noticed before now. We've had DISCARD ALL with its current meaning since 8.3, and nobody complained in the five-plus years since that shipped. At this point, even if a concrete case were made why DISCARD ALL should clear currval (and I repeat that no credible case has been made; nobody has for example pointed to a reasonably-well-designed application that this breaks), there would be a pretty strong backwards-compatibility argument not to change it. regards, tom lane
On Wed, Apr 17, 2013 at 6:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > No, it's a critical tool in complexity management. When you're dealing > with systems as complicated as a database, every little non-orthogonal > detail adds up. DISCARD ALL has a clear definition in terms of simpler > commands, and it's going to stay that way. Either this is worth a > subcommand, or it's not worth worrying about at all. We had this same argument back in November of 2008. Marko said: http://www.postgresql.org/message-id/24710.1227732351@sss.pgh.pa.us And Greg Stark said: http://www.postgresql.org/message-id/87iqqapag2.fsf@oxford.xeocode.com And you said: http://www.postgresql.org/message-id/24710.1227732351@sss.pgh.pa.us And then you did this: commit e309739670ac8c2fa0b236d116fcd44b0522025a Author: Tom Lane <tgl@sss.pgh.pa.us> Date: Thu Nov 27 00:28:06 2008 +0000 Tweak wording of DISCARD ALL description to avoid giving the impression that the presented list of equivalent operationsis meant to be the primary definition of what it does. Per comment from Guillaume Smet. So it seems to me that we pretty much already made a decision that the controlling definition of DISCARD ALL is that, as the fine manual says "DISCARD ALL resets a session to its original state". Whatever decision we make now ought to be consistent with that. IOW, I don't care whether we introduce a new subcommand or not. But I *do* think that that we ought to make our best effort to have DISCARD ALL clear everything that smells like session-local state. Random incompatibilities between what you see when running under a connection pooler and what you see when connecting the DB directly are *bad*, regardless of whether a well-designed application should be relying on those particular things or not. The whole point of having a transparent connection pooler is that it's supposed to be transparent to the application. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Apr 19, 2013 at 10:50 AM, Robert Haas <robertmhaas@gmail.com> wrote:
[...]
So it seems to me that we pretty much already made a decision that the
controlling definition of DISCARD ALL is that, as the fine manual says
"DISCARD ALL resets a session to its original state". Whatever
decision we make now ought to be consistent with that.
IOW, I don't care whether we introduce a new subcommand or not. But I
*do* think that that we ought to make our best effort to have DISCARD
ALL clear everything that smells like session-local state. Random
incompatibilities between what you see when running under a connection
pooler and what you see when connecting the DB directly are *bad*,
regardless of whether a well-designed application should be relying on
those particular things or not. The whole point of having a
transparent connection pooler is that it's supposed to be transparent
to the application.
+1
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.
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
Вложения
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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 04/19/2013 06:50 AM, Robert Haas wrote: > On Wed, Apr 17, 2013 at 6:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> No, it's a critical tool in complexity management. When you're dealing >> with systems as complicated as a database, every little non-orthogonal >> detail adds up. DISCARD ALL has a clear definition in terms of simpler >> commands, and it's going to stay that way. Either this is worth a >> subcommand, or it's not worth worrying about at all. > And then you did this: > > commit e309739670ac8c2fa0b236d116fcd44b0522025a > Author: Tom Lane <tgl@sss.pgh.pa.us> > Date: Thu Nov 27 00:28:06 2008 +0000 > > Tweak wording of DISCARD ALL description to avoid giving the impression > that the presented list of equivalent operations is meant to be the > primary definition of what it does. Per comment from Guillaume Smet. > > So it seems to me that we pretty much already made a decision that the > controlling definition of DISCARD ALL is that, as the fine manual says > "DISCARD ALL resets a session to its original state". Whatever > decision we make now ought to be consistent with that. > > IOW, I don't care whether we introduce a new subcommand or not. But I > *do* think that that we ought to make our best effort to have DISCARD > ALL clear everything that smells like session-local state. Random > incompatibilities between what you see when running under a connection > pooler and what you see when connecting the DB directly are *bad*, > regardless of whether a well-designed application should be relying on > those particular things or not. The whole point of having a > transparent connection pooler is that it's supposed to be transparent > to the application. I understand the confusion on what constitutes ALL in DISCARD, though I am not sure about the incompatibility argument. The OP is using the transaction mode from pgBouncer and from their docs: http://wiki.postgresql.org/wiki/PgBouncer "Transaction pooling Server connection is assigned to client only during a transaction. When PgBouncer notices that transaction is over, the server will be put back into pool. This mode breaks few session-based features of PostgreSQL. You can use it only when application cooperates by not using features that break. See the table below for incompatible features." " Note that 'transaction' pooling breaks client expectations of server by design and can be used only if application cooperates by not using non-working features." " Session pooling server_reset_query = DISCARD ALL; This will clean everything. Transaction pooling server_reset_query = Yes, empty. In transaction pooling mode the clients should not use any session-based features, so there is no need to clean anything. The server_reset_query would only add unnecessary round-trip between transactions and would drop various caches that the next transaction would unnecessarily need to fill again." I could see the argument for a transparent pooler where it part of the core code. Not sure if it is the projects responsibility to maintain transparency with the feature matrices of external projects. > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > -- Adrian Klaver adrian.klaver@gmail.com
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 MelloThis patch is quite wrong. It frees seqtab without clearing the
<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.
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
Вложения
Fabrízio de Royes Mello escribió: > 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. It seems a bad idea to backpatch this; whoever wants this functionality in back branches should probably run a patched server. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes: > It seems a bad idea to backpatch this; whoever wants this functionality > in back branches should probably run a patched server. Surely this is 9.4 material at this point in any case. regards, tom lane
On Fri, Apr 19, 2013 at 11:09 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: >> It seems a bad idea to backpatch this; whoever wants this functionality >> in back branches should probably run a patched server. > > Surely this is 9.4 material at this point in any case. I don't know why this couldn't be slipped into 9.3; we have done worse later. But I don't have a personal stake in it either, and will certainly defer to whatever the consensus is. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Disclaimer: I am no hacker, just a PostGreSQL user, trying to provide a user scenario where DISCARD SEQUENCES functionality is required. We have designed a developed a small Application Development platform for which the backend is PostGreSQL. There is a DBLayer which is responsible in generating SQL statements for all the INSERT, UPDATE, DELETE operations. Data can be pushed into multiple tables using this layer. What we provide to this layer is just a DataSet (.NET). DataTables in the DataSet will be named after their respective tables. We also use DataColumn extended properties to push in additional logic. All these happen with in a transaction and also in one shot, just one hit to the DB by appending SQL statements in proper order. There is an interesting feature that we have built into this DBlayer which is auto linking. All tables in our system will have a serial field 'id'. Suppose there is a master table (let us call it 'voucher') and a detail table ('voucher_lines'), and we are using the layer to push one record to the master table and 50 records to the detail table. 'voucher_lines' table will have a integer column 'voucher_id'. '*_id' fields are automatically populated with 'currval('*_id_seq'). All this works like a charm. Now, imagine we want to push data into another table (say 'invoice') which also has a field 'voucher_id'. This is a different activity not connected with the above mentioned transaction. In this scenario this field will get updated as currval('voucher_id_seq') returns a value. But we do not want that to be updated. What we want is to resolve '*_id' fields into values only within a transaction. After the transaction we want to get away with the session. If we could have cleared the session someway (DISCARD ALL does it, but not the currval sequence info) after the first transaction it would have worked for us. Hope I am clear enough. Thanks for PostGres, it is a great DB, love working with it. -- View this message in context: http://postgresql.1045698.n5.nabble.com/Re-GENERAL-currval-and-DISCARD-ALL-tp5752340p5765110.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.
On 25-07-2013 05:32, suresh.balasubra wrote: > Disclaimer: I am no hacker, just a PostGreSQL user, trying to provide a user > scenario where DISCARD SEQUENCES functionality is required. > > We have designed a developed a small Application Development platform for > which the backend is PostGreSQL. > > There is a DBLayer which is responsible in generating SQL statements for all > the INSERT, UPDATE, DELETE operations. Data can be pushed into multiple > tables using this layer. What we provide to this layer is just a DataSet > (.NET). DataTables in the DataSet will be named after their respective > tables. We also use DataColumn extended properties to push in additional > logic. All these happen with in a transaction and also in one shot, just one > hit to the DB by appending SQL statements in proper order. > > There is an interesting feature that we have built into this DBlayer which > is auto linking. All tables in our system will have a serial field 'id'. > Suppose there is a master table (let us call it 'voucher') and a detail > table ('voucher_lines'), and we are using the layer to push one record to > the master table and 50 records to the detail table. 'voucher_lines' table > will have a integer column 'voucher_id'. '*_id' fields are automatically > populated with 'currval('*_id_seq'). All this works like a charm. > > Now, imagine we want to push data into another table (say 'invoice') which > also has a field 'voucher_id'. This is a different activity not connected > with the above mentioned transaction. In this scenario this field will get > updated as currval('voucher_id_seq') returns a value. But we do not want > that to be updated. What we want is to resolve '*_id' fields into values > only within a transaction. After the transaction we want to get away with > the session. If we could have cleared the session someway (DISCARD ALL does > it, but not the currval sequence info) after the first transaction it would > have worked for us. > I already sent a patch to implement DISCARD SEQUENCES [1] that I expect to be part of 9.4 version. Regards, [1] https://commitfest.postgresql.org/action/patch_view?id=1171 -- Fabrízio de Royes Mello Timbira - http://www.timbira.com.br/ PostgreSQL: Consultoria, Desenvolvimento, Suporte24x7 e Treinamento
Hi,
2013-04-19 16:58 keltezéssel, Fabrízio de Royes Mello írta:
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@gmail.com> wrote:On Fri, Apr 19, 2013 at 10:05 AM, Fabrízio de Royes MelloThis patch is quite wrong. It frees seqtab without clearing the
<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.
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/
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@gmail.com> wrote:On Fri, Apr 19, 2013 at 10:05 AM, Fabrízio de Royes MelloThis patch is quite wrong. It frees seqtab without clearing the
<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.
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/
On Fri, 2013-04-19 at 11:58 -0300, Fabrízio de Royes Mello wrote: > > 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. Please fix this compiler warning: sequence.c:1608:1: warning: no previous prototype for ‘ReleaseSequenceCaches’ [-Wmissing-prototypes]
On 19-08-2013 16:10, Boszormenyi Zoltan wrote: >> >> I am reviewing your patch. >> Thanks... >> * 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; >> +} >> Done! >> * 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 > Fixed! >> >> * 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() >> ^ >> Fixed! >> * 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. >> The attached patch fix the items reviewed by you. Regards, -- Fabrízio de Royes Mello Timbira - http://www.timbira.com.br/ PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
Вложения
On Mon, Sep 2, 2013 at 4:35 PM, Fabrízio de Royes Mello <fabrizio@timbira.com.br> wrote: > The attached patch fix the items reviewed by you. Committed with assorted revisions. In particular, I renamed the function that discards cached sequence data, revised the wording of the documentation, added a regression test, and tweaked the list-free code to pop items off one after the other instead of walking the list and then NULLing it out at the end. Although no ERROR is possible here currently, this coding style is generally preferable because it's robust against being interrupted in the middle. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
<div dir="ltr"><div class="gmail_extra"><br />On Thu, Oct 3, 2013 at 5:26 PM, Robert Haas <<a href="mailto:robertmhaas@gmail.com">robertmhaas@gmail.com</a>>wrote:<br />><br />><br />> Committed with assortedrevisions. In particular, I renamed the<br /> > function that discards cached sequence data, revised the wordingof<br />> the documentation, added a regression test, and tweaked the list-free<br />> code to pop items offone after the other instead of walking the list<br /> > and then NULLing it out at the end. Although no ERROR is possible<br/>> here currently, this coding style is generally preferable because it's<br />> robust against being interruptedin the middle.<br />><br /><br />Thanks!<br /><br />--<br /> Fabrízio de Royes Mello Timbira - <ahref="http://www.timbira.com.br/">http://www.timbira.com.br/</a><br /> PostgreSQL: Consultoria, Desenvolvimento, Suporte24x7 e Treinamento</div></div>