Re: [HACKERS] [GENERAL] currval and DISCARD ALL
On Mon, Sep 2, 2013 at 4:35 PM, Fabrízio de Royes Mello fabri...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] currval and DISCARD ALL
On Thu, Oct 3, 2013 at 5:26 PM, Robert Haas robertmh...@gmail.com wrote: 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. Thanks! -- Fabrízio de Royes Mello Timbira - http://www.timbira.com.br/ PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
Re: [HACKERS] [GENERAL] currval and DISCARD ALL
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
diff --git a/doc/src/sgml/ref/discard.sgml b/doc/src/sgml/ref/discard.sgml
index 65ebbae..abd3e28 100644
--- a/doc/src/sgml/ref/discard.sgml
+++ b/doc/src/sgml/ref/discard.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
refsynopsisdiv
synopsis
-DISCARD { ALL | PLANS | TEMPORARY | TEMP }
+DISCARD { ALL | PLANS | SEQUENCES | TEMPORARY | TEMP }
/synopsis
/refsynopsisdiv
@@ -67,6 +67,15 @@ DISCARD { ALL | PLANS | TEMPORARY | TEMP }
/varlistentry
varlistentry
+termliteralSEQUENCES/literal/term
+listitem
+ para
+ Releases all internally cached sequences.
+ /para
+/listitem
+ /varlistentry
+
+ varlistentry
termliteralALL/literal/term
listitem
para
@@ -83,6 +92,7 @@ UNLISTEN *;
SELECT pg_advisory_unlock_all();
DISCARD PLANS;
DISCARD TEMP;
+DISCARD SEQUENCES;
/programlisting/para
/listitem
/varlistentry
diff --git a/src/backend/commands/discard.c b/src/backend/commands/discard.c
index 76f3ab6..f4e7e06 100644
--- a/src/backend/commands/discard.c
+++ b/src/backend/commands/discard.c
@@ -18,13 +18,14 @@
#include commands/async.h
#include commands/discard.h
#include commands/prepare.h
+#include commands/sequence.h
#include utils/guc.h
#include utils/portal.h
static void DiscardAll(bool isTopLevel);
/*
- * DISCARD { ALL | TEMP | PLANS }
+ * DISCARD { ALL | SEQUENCES | TEMP | PLANS }
*/
void
DiscardCommand(DiscardStmt *stmt, bool isTopLevel)
@@ -39,6 +40,10 @@ DiscardCommand(DiscardStmt *stmt, bool isTopLevel)
ResetPlanCache();
break;
+ case
Re: [HACKERS] [GENERAL] currval and DISCARD ALL
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] -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [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 robertmh...@gmail.com
mailto:robertmh...@gmail.com wrote:
On Fri, Apr 19, 2013 at 10:05 AM, Fabrízio de Royes Mello
fabriziome...@gmail.com mailto:fabriziome...@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/
Re: [HACKERS] [GENERAL] currval and DISCARD ALL
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 robertmh...@gmail.com
mailto:robertmh...@gmail.com wrote:
On Fri, Apr 19, 2013 at 10:05 AM, Fabrízio de Royes Mello
fabriziome...@gmail.com mailto:fabriziome...@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.
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/
Re: [HACKERS] [GENERAL] currval and DISCARD ALL
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, Suporte 24x7 e Treinamento
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] currval and DISCARD ALL
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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] currval and DISCARD ALL
On Wed, Apr 17, 2013 at 6:17 PM, Tom Lane t...@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.1227732...@sss.pgh.pa.us And Greg Stark said: http://www.postgresql.org/message-id/87iqqapag2@oxford.xeocode.com And you said: http://www.postgresql.org/message-id/24710.1227732...@sss.pgh.pa.us And then you did this: commit e309739670ac8c2fa0b236d116fcd44b0522025a Author: Tom Lane t...@sss.pgh.pa.us Date: Thu Nov 27 00:28:06 2008 + 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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] currval and DISCARD ALL
On Fri, Apr 19, 2013 at 10:50 AM, Robert Haas robertmh...@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 discard_sequences.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] currval and DISCARD ALL
On Fri, Apr 19, 2013 at 10:05 AM, Fabrízio de Royes Mello fabriziome...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] currval and DISCARD ALL
On 04/19/2013 06:50 AM, Robert Haas wrote: On Wed, Apr 17, 2013 at 6:17 PM, Tom Lane t...@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 t...@sss.pgh.pa.us Date: Thu Nov 27 00:28:06 2008 + 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.kla...@gmail.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] currval and DISCARD ALL
On Fri, Apr 19, 2013 at 11:12 AM, Robert Haas robertmh...@gmail.com wrote: On Fri, Apr 19, 2013 at 10:05 AM, Fabrízio de Royes Mello fabriziome...@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 discard_sequences.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] currval and DISCARD ALL
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] currval and DISCARD ALL
Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] currval and DISCARD ALL
On Fri, Apr 19, 2013 at 11:09 AM, Tom Lane t...@sss.pgh.pa.us wrote: Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] currval and DISCARD ALL
On Tue, Apr 16, 2013 at 05:09:19PM -0400, Tom Lane wrote: Bruce Momjian br...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] currval and DISCARD ALL
Marko Kreen mark...@gmail.com writes: On Tue, Apr 16, 2013 at 05:09:19PM -0400, Tom Lane wrote: Bruce Momjian br...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [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 br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] currval and DISCARD ALL
Bruce Momjian br...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] currval and DISCARD ALL
On Tue, Apr 16, 2013 at 6:09 PM, Tom Lane t...@sss.pgh.pa.us wrote: [...] 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
