Re: [GENERAL] currval and DISCARD ALL

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.

eg. (w/ pg 9.2.4)
test=# CREATE SEQUENCE foo_seq;
CREATE SEQUENCE
test=# SELECT nextval('foo_seq');
-[ RECORD 1 ]
nextval | 1

test=# SELECT currval('foo_seq');
-[ RECORD 1 ]
currval | 1

test=# DISCARD ALL;
DISCARD ALL
test=# SELECT currval('foo_seq');
-[ RECORD 1 ]
currval | 1

I'm trying to migrate a large web app to work with pgbouncer's
transaction pool mode and it would be easier to identify issues if
currval() would return the usual "ERROR: currval of sequence "foo_seq"
is not yet defined in this session" if nextval() wasn't called in the
same pgbouncer session instead of getting old numbers from past
transactions.

thanks,
-nigel.

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.
>
> eg. (w/ pg 9.2.4)
> test=# CREATE SEQUENCE foo_seq;
> CREATE SEQUENCE
> test=# SELECT nextval('foo_seq');
> -[ RECORD 1 ]
> nextval | 1
>
> test=# SELECT currval('foo_seq');
> -[ RECORD 1 ]
> currval | 1
>
> test=# DISCARD ALL;
> DISCARD ALL
> test=# SELECT currval('foo_seq');
> -[ RECORD 1 ]
> currval | 1
>
>
> I'm trying to migrate a large web app to work with pgbouncer's
> transaction pool mode and it would be easier to identify issues if
> currval() would return the usual "ERROR: currval of sequence "foo_seq"
> is not yet defined in this session" if nextval() wasn't called in the
> same pgbouncer session instead of getting old numbers from past
> transactions.

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,
> -nigel.
>
>

--
Adrian Klaver
adrian(dot)klaver(at)gmail(dot)com

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?
from the docs:
"DISCARD ALL resets a session to its original state, discarding
temporary resources and resetting session-local configuration changes."
.. but at the beginning of a session currval(foo) would return an error,
whereas calling nexval(foo); DISCARD ALL; currval(foo); does not return
an error.. clearly something isn't getting reset to the original state.

If you create a TEMP sequence, then DISCARD ALL does clear the state,
probably because the underlying table disappears.

-nigel.

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.

> -nigel.
>

--
Adrian Klaver
adrian(dot)klaver(at)gmail(dot)com

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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian <bruce(at)momjian(dot)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(at)sss(dot)pgh(dot)pa(dot)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

On Tue, Apr 16, 2013 at 05:09:19PM -0400, Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)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(at)gmail(dot)com> writes:
> On Tue, Apr 16, 2013 at 05:09:19PM -0400, Tom Lane wrote:
>> Bruce Momjian <bruce(at)momjian(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Apr 19, 2013 at 10:50 AM, Robert Haas <robertmhaas(at)gmail(dot)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(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.

--
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(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)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(dot)klaver(at)gmail(dot)com

On Fri, Apr 19, 2013 at 11:12 AM, Robert Haas <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> 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

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(at)2ndquadrant(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)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, Suporte 24x7 e Treinamento

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/

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(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.

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(at)timbira(dot)com(dot)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

On Thu, Oct 3, 2013 at 5:26 PM, Robert Haas <robertmhaas(at)gmail(dot)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