Re: [COMMITTERS] pgsql: Add DISCARD SEQUENCES command.

Add DISCARD SEQUENCES command.

DISCARD ALL will now discard cached sequence information, as well.

Fabrízio de Royes Mello, reviewed by Zoltán Böszörményi, with some
further tweaks by me.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/d90ced8bb22194cbb45f58beb0961251103aeff5

Modified Files
--------------
doc/src/sgml/ref/discard.sgml | 12 +++++++++++-
src/backend/commands/discard.c | 8 +++++++-
src/backend/commands/sequence.c | 16 ++++++++++++++++
src/backend/parser/gram.y | 9 ++++++++-
src/backend/tcop/utility.c | 3 +++
src/bin/psql/tab-complete.c | 2 +-
src/include/commands/sequence.h | 1 +
src/include/nodes/parsenodes.h | 1 +
src/test/regress/expected/sequence.out | 3 +++
src/test/regress/sql/sequence.sql | 2 ++
10 files changed, 53 insertions(+), 4 deletions(-)

My C is very rusty but the traversal of SeqTableData doesn't seem correct.
It saves the seqtab->next pointer into next, frees seqtab and then
dereferences it.
Shouldn't that last line be: seqtab = next?

Kevin.

+/*
+ * Flush cached sequence information.
+ */
+void
+ResetSequenceCaches(void)
+{
+ SeqTableData *next;
+
+ while (seqtab != NULL)
+ {
+ next = seqtab->next;
+ free(seqtab);
+ seqtab = seqtab->next;
+ }

On 3 October 2013 14:23, Robert Haas <rhaas(at)postgresql(dot)org> wrote:

> Add DISCARD SEQUENCES command.
>
> DISCARD ALL will now discard cached sequence information, as well.
>
> Fabrízio de Royes Mello, reviewed by Zoltán Böszörményi, with some
> further tweaks by me.
>
> Branch
> ------
> master
>
> Details
> -------
>
> http://git.postgresql.org/pg/commitdiff/d90ced8bb22194cbb45f58beb0961251103aeff5
>
> Modified Files
> --------------
> doc/src/sgml/ref/discard.sgml | 12 +++++++++++-
> src/backend/commands/discard.c | 8 +++++++-
> src/backend/commands/sequence.c | 16 ++++++++++++++++
> src/backend/parser/gram.y | 9 ++++++++-
> src/backend/tcop/utility.c | 3 +++
> src/bin/psql/tab-complete.c | 2 +-
> src/include/commands/sequence.h | 1 +
> src/include/nodes/parsenodes.h | 1 +
> src/test/regress/expected/sequence.out | 3 +++
> src/test/regress/sql/sequence.sql | 2 ++
> 10 files changed, 53 insertions(+), 4 deletions(-)
>
>
> --
> Sent via pgsql-committers mailing list (pgsql-committers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-committers
>

On Thu, Oct 3, 2013 at 6:38 PM, Kevin Hale Boyes <kcboyes(at)gmail(dot)com> wrote:
> My C is very rusty but the traversal of SeqTableData doesn't seem correct.
> It saves the seqtab->next pointer into next, frees seqtab and then
> dereferences it.
> Shouldn't that last line be: seqtab = next?
>
> Kevin.
>
> +/*
> + * Flush cached sequence information.
> + */
> +void
> +ResetSequenceCaches(void)
> +{
> + SeqTableData *next;
> +
> + while (seqtab != NULL)
> + {
> + next = seqtab->next;
> + free(seqtab);
> + seqtab = seqtab->next;
> + }

Oops, good catch. Will fix, thanks.

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

Hi,

While having a look at this feature I found an assertion failure
(commit 1310d4c):
#2 0x000000010eb7856d in ExceptionalCondition
(conditionName=0x10ec3790b "!(last_used_seq->last_valid)",
errorType=0x10ebe25e0 "FailedAssertion", fileName=0x10ec37168
"sequence.c", lineNumber=809) at assert.c:54
#3 0x000000010e7cfc75 in lastval (fcinfo=0x7fd5f1084900) at sequence.c:809
#4 0x000000010e82d70e in ExecMakeFunctionResult
(fcache=0x7fd5f1084890, econtext=0x7fd5f1084668, isNull=0x7fd5f1085240
"~h?\f??", isDone=0x7fd5f1085380) at execQual.c:1938
#5 0x000000010e82e56f in ExecEvalFunc (fcache=0x7fd5f1084890,
econtext=0x7fd5f1084668, isNull=0x7fd5f1085240 "~h?\f??",
isDone=0x7fd5f1085380) at execQual.c:2377
#6 0x000000010e836d92 in ExecTargetList (targetlist=0x7fd5f1085348,
econtext=0x7fd5f1084668, values=0x7fd5f1085220, isnull=0x7fd5f1085240
"~h?\f??", itemIsDone=0x7fd5f1085380, isDone=0x7fff51659954) at
execQual.c:5244
#7 0x000000010e8374ad in ExecProject (projInfo=0x7fd5f1085260,
isDone=0x7fff51659954) at execQual.c:5459
#8 0x000000010e858151 in ExecResult (node=0x7fd5f1084550) at nodeResult.c:155
#9 0x000000010e828bf3 in ExecProcNode (node=0x7fd5f1084550) at
execProcnode.c:373
#10 0x000000010e8262fe in ExecutePlan (estate=0x7fd5f1084438,
planstate=0x7fd5f1084550, operation=CMD_SELECT, sendTuples=1 '\001',
numberTuples=0, direction=ForwardScanDirection, dest=0x7fd5f1035470)
at execMain.c:1472
#11 0x000000010e8238fb in standard_ExecutorRun
(queryDesc=0x7fd5f10cfe38, direction=ForwardScanDirection, count=0) at
execMain.c:307
#12 0x000000010e8236e8 in ExecutorRun (queryDesc=0x7fd5f10cfe38,
direction=ForwardScanDirection, count=0) at execMain.c:255
#13 0x000000010e9dd21e in PortalRunSelect (portal=0x7fd5f10a6c38,
forward=1 '\001', count=0, dest=0x7fd5f1035470) at pquery.c:946
#14 0x000000010e9dcd96 in PortalRun (portal=0x7fd5f10a6c38,
count=9223372036854775807, isTopLevel=1 '\001', dest=0x7fd5f1035470,
altdest=0x7fd5f1035470, completionTag=0x7fff51659e00 "") at
pquery.c:790
#15 0x000000010e9d4764 in exec_simple_query
(query_string=0x7fd5f1033a38 "select lastval();") at postgres.c:1048
#16 0x000000010e9da617 in PostgresMain (argc=1, argv=0x7fd5f101bfc0,
dbname=0x7fd5f101be28 "ioltas", username=0x7fd5f101be08 "ioltas") at
postgres.c:3992
#17 0x000000010e953756 in BackendRun (port=0x7fd5f0c06280) at postmaster.c:4083
#18 0x000000010e952aec in BackendStartup (port=0x7fd5f0c06280) at
postmaster.c:3772
#19 0x000000010e94dfd3 in ServerLoop () at postmaster.c:1583
#20 0x000000010e94d14e in PostmasterMain (argc=3, argv=0x7fd5f0c04150)
at postmaster.c:1239
#21 0x000000010e8854db in main (argc=3, argv=0x7fd5f0c04150) at main.c:196

Here is the test case failing:
=# create sequence foo;
CREATE SEQUENCE
=# select nextval('foo');
nextval
---------
1
(1 row)
=# discard sequences ;
DISCARD SEQUENCES
=# select currval('foo');
ERROR: 55000: currval of sequence "foo" is not yet defined in this session
LOCATION: currval_oid, sequence.c:780
=# select lastval();
The connection to the server was lost. Attempting reset: Failed.

Also, I would have expected that the value cached for lastval is also
discarded after invocating DISCARD SEQUENCES. Is the behavior below
expected?
=# create sequence foo;
CREATE SEQUENCE
=# select nextval('foo');
nextval
---------
1
(1 row)
=# discard sequences ;
DISCARD SEQUENCES
=# select lastval();
lastval
---------
1
(1 row)

I didn't have a look at the code, but perhaps the assertion failure I
am seeing is because of lastval not discarded as well.

Regards,
--
Michael

Michael Paquier escribió:

> =# discard sequences ;
> DISCARD SEQUENCES
> =# select currval('foo');
> ERROR: 55000: currval of sequence "foo" is not yet defined in this session
> LOCATION: currval_oid, sequence.c:780
> =# select lastval();
> The connection to the server was lost. Attempting reset: Failed.

I wonder if it would be better to have this "seqtab" thing be a dlist,
and move the most recently used item to head position on nextval().
That way we don't have to maintain last_used_seq separately.

I also just noticed that the DISCARD SEQUENCE patch made a "NOTE:"
comment in init_sequence be outdated.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sun, Oct 6, 2013 at 1:25 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> Michael Paquier escribió:
>
>> =# discard sequences ;
>> DISCARD SEQUENCES
>> =# select currval('foo');
>> ERROR: 55000: currval of sequence "foo" is not yet defined in this session
>> LOCATION: currval_oid, sequence.c:780
>> =# select lastval();
>> The connection to the server was lost. Attempting reset: Failed.
>
> I wonder if it would be better to have this "seqtab" thing be a dlist,
> and move the most recently used item to head position on nextval().
> That way we don't have to maintain last_used_seq separately.
Indeed, this could simplify a bit the code.
--
Michael

On Sat, Oct 5, 2013 at 8:10 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> Here is the test case failing:
> =# create sequence foo;
> CREATE SEQUENCE
> =# select nextval('foo');
> nextval
> ---------
> 1
> (1 row)
> =# discard sequences ;
> DISCARD SEQUENCES
> =# select currval('foo');
> ERROR: 55000: currval of sequence "foo" is not yet defined in this session
> LOCATION: currval_oid, sequence.c:780
> =# select lastval();
> The connection to the server was lost. Attempting reset: Failed.

Thanks. I have pushed a fix that I hope will be sufficient.

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

On Tue, Oct 8, 2013 at 4:57 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Sat, Oct 5, 2013 at 8:10 AM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> Here is the test case failing:
>> =# create sequence foo;
>> CREATE SEQUENCE
>> =# select nextval('foo');
>> nextval
>> ---------
>> 1
>> (1 row)
>> =# discard sequences ;
>> DISCARD SEQUENCES
>> =# select currval('foo');
>> ERROR: 55000: currval of sequence "foo" is not yet defined in this session
>> LOCATION: currval_oid, sequence.c:780
>> =# select lastval();
>> The connection to the server was lost. Attempting reset: Failed.
>
> Thanks. I have pushed a fix that I hope will be sufficient.
It is fine now. Thanks for the fix.
--
Michael