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

On Thu, Oct 3, 2013 at 6:38 PM, Kevin Hale Boyes <kcboyes@gmail.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@2ndquadrant.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.paquier@gmail.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@gmail.com> wrote:
> On Sat, Oct 5, 2013 at 8:10 AM, Michael Paquier
> <michael.paquier@gmail.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