Thread: current version: Patch - Have psql show current values for a sequence]
sorry for sending the old version in the previous mail . Here I attach
the recent version of the patch file.
Dhanaraj M wrote:
> I saw the following in the TODO list.
> clients-psql
> ============
> 1. Have psql show current values for a sequence
>
> Hence, this patch displays the current seq. value in a separate column
> when \ds is executed.
> I attach the patch here. The display format may have to be changed. I
> would like to change the display format as you will suggest me.
> I am awaiting for the review.
>
> Thanks
> Dhanaraj
>
>
>
*** ./src/backend/commands/sequence.c.orig Tue May 2 14:51:03 2006
--- ./src/backend/commands/sequence.c Tue May 2 15:00:19 2006
***************
*** 1183,1185 ****
--- 1183,1274 ----
xlrec->node.spcNode, xlrec->node.dbNode, xlrec->node.relNode);
}
+
+ /* Returns the current sequence value even if not available in the cache */
+ Datum
+ retrieveval_oid(PG_FUNCTION_ARGS)
+ {
+
+ Oid relid = PG_GETARG_DATUM(0);
+ int64 result;
+ SeqTable elm;
+ Relation seqrel;
+ Form_pg_sequence seq;
+ Buffer buf;
+
+ /* open and AccessShareLock sequence */
+ init_sequence(relid, &elm, &seqrel);
+
+ if (pg_class_aclcheck(elm->relid, GetUserId(), ACL_SELECT) != ACLCHECK_OK &&
+ pg_class_aclcheck(elm->relid, GetUserId(), ACL_USAGE) != ACLCHECK_OK)
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied for sequence %s",
+ RelationGetRelationName(seqrel))));
+
+
+ if ((elm->increment != 0) ||(elm->last != elm->cached)) /* some numbers were cached */
+ {
+ result = elm->last;
+ relation_close(seqrel, NoLock);
+ PG_RETURN_INT64(result);
+ }
+
+ /* lock page' buffer and read tuple if not cached */
+ seq = read_info(elm, seqrel, &buf);
+ result = seq->last_value;
+
+ UnlockReleaseBuffer(buf);
+ relation_close(seqrel, NoLock);
+
+ seqtab = elm->next;
+ free(elm);
+
+ PG_RETURN_INT64(result);
+ }
+
+
+ /* Checks whether the sequence value is already used or not */
+ Datum
+ retrievecheck_oid(PG_FUNCTION_ARGS)
+ {
+
+ Oid relid = PG_GETARG_DATUM(0);
+ SeqTable elm;
+ Relation seqrel;
+ Form_pg_sequence seq;
+ Buffer buf;
+ bool result;
+
+ /* open and AccessShareLock sequence */
+ init_sequence(relid, &elm, &seqrel);
+
+ if (pg_class_aclcheck(elm->relid, GetUserId(), ACL_SELECT) != ACLCHECK_OK &&
+ pg_class_aclcheck(elm->relid, GetUserId(), ACL_USAGE) != ACLCHECK_OK)
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied for sequence %s",
+ RelationGetRelationName(seqrel))));
+
+
+ if ((elm->increment != 0) ||(elm->last != elm->cached)) /* some numbers were cached */
+ {
+ relation_close(seqrel, NoLock);
+ PG_RETURN_INT64(0);
+ }
+
+ /* lock page' buffer and read tuple */
+ seq = read_info(elm, seqrel, &buf);
+ result = seq->is_called;
+
+ UnlockReleaseBuffer(buf);
+ relation_close(seqrel, NoLock);
+
+ seqtab = elm->next;
+ free(elm);
+
+ if (result == true)
+ PG_RETURN_INT64(0);
+ else
+ PG_RETURN_INT64(1);
+ }
*** ./src/backend/utils/fmgroids.h.orig Tue May 2 14:19:25 2006
--- ./src/backend/utils/fmgroids.h Tue May 2 15:11:08 2006
***************
*** 901,906 ****
--- 901,908 ----
#define F_NEXTVAL_OID 1574
#define F_CURRVAL_OID 1575
#define F_SETVAL_OID 1576
+ #define F_RETRIEVEVAL_OID 1577
+ #define F_RETRIEVECHECK_OID 1578
#define F_VARBIT_IN 1579
#define F_VARBIT_OUT 1580
#define F_BITEQ 1581
*** ./src/backend/utils/fmgrtab.c.orig Tue May 2 14:19:25 2006
--- ./src/backend/utils/fmgrtab.c Tue May 2 15:11:08 2006
***************
*** 952,957 ****
--- 952,959 ----
extern Datum nextval_oid (PG_FUNCTION_ARGS);
extern Datum currval_oid (PG_FUNCTION_ARGS);
extern Datum setval_oid (PG_FUNCTION_ARGS);
+ extern Datum retrieveval_oid (PG_FUNCTION_ARGS);
+ extern Datum retrievecheck_oid (PG_FUNCTION_ARGS);
extern Datum varbit_in (PG_FUNCTION_ARGS);
extern Datum varbit_out (PG_FUNCTION_ARGS);
extern Datum biteq (PG_FUNCTION_ARGS);
***************
*** 2661,2666 ****
--- 2663,2670 ----
{ 1574, "nextval_oid", 1, true, false, nextval_oid },
{ 1575, "currval_oid", 1, true, false, currval_oid },
{ 1576, "setval_oid", 2, true, false, setval_oid },
+ { 1577, "retrieveval_oid", 1, true, false, retrieveval_oid },
+ { 1578, "retrievecheck_oid", 1, true, false, retrievecheck_oid },
{ 1579, "varbit_in", 3, true, false, varbit_in },
{ 1580, "varbit_out", 1, true, false, varbit_out },
{ 1581, "biteq", 2, true, false, biteq },
*** ./src/bin/psql/describe.c.orig Thu Apr 27 04:45:45 2006
--- ./src/bin/psql/describe.c Tue May 2 15:22:49 2006
***************
*** 1479,1484 ****
--- 1479,1489 ----
_("Schema"), _("Name"),
_("table"), _("view"), _("index"), _("sequence"),
_("special"), _("Type"), _("Owner"));
+ if (showSeq && !showTables)
+ appendPQExpBuffer(&buf,
+ ",\n retrieveval(c.oid) as \"%s\""
+ ",\n CASE retrievecheck(c.oid) WHEN '1' THEN '%s' WHEN '0' THEN '%s' END as \"%s\"",
+ _("value"),_(" ***"),_(""),_("Start from"));
if (showIndexes)
appendPQExpBuffer(&buf,
***************
*** 1546,1551 ****
--- 1551,1559 ----
myopt.nullPrint = NULL;
myopt.title = _("List of relations");
+ if (showSeq && !showTables)
+ myopt.title = _("List of relations (*** - specifies the starting value and it has not yet been used)");
+
printQuery(res, &myopt, pset.queryFout, pset.logfile);
}
*** ./src/include/catalog/pg_proc.h.orig Tue May 2 15:06:15 2006
--- ./src/include/catalog/pg_proc.h Tue May 2 15:07:33 2006
***************
*** 2057,2062 ****
--- 2057,2067 ----
DESCR("sequence current value");
DATA(insert OID = 1576 ( setval PGNSP PGUID 12 f f t f v 2 20 "2205 20" _null_ _null_ _null_ setval_oid
-_null_ ));
DESCR("set sequence value");
+ DATA(insert OID = 1577 ( retrieveval PGNSP PGUID 12 f f t f v 1 20 "26" _null_ _null_ _null_
retrieveval_oid- _null_ ));
+ DESCR("sequence current value");
+ DATA(insert OID = 1578 ( retrievecheck PGNSP PGUID 12 f f t f v 1 20 "26" _null_ _null_ _null_
retrievecheck_oid- _null_ ));
+ DESCR("sequence current value check");
+
DATA(insert OID = 1765 ( setval PGNSP PGUID 12 f f t f v 3 20 "2205 20 16" _null_ _null_ _null_
setval3_oid- _null_ ));
DESCR("set sequence value and iscalled status");
*** ./src/include/commands/sequence.h.orig Tue May 2 14:52:23 2006
--- ./src/include/commands/sequence.h Tue May 2 15:00:06 2006
***************
*** 86,91 ****
--- 86,93 ----
extern Datum setval_oid(PG_FUNCTION_ARGS);
extern Datum setval3_oid(PG_FUNCTION_ARGS);
extern Datum lastval(PG_FUNCTION_ARGS);
+ extern Datum retrieveval_oid(PG_FUNCTION_ARGS);
+ extern Datum retrievecheck_oid(PG_FUNCTION_ARGS);
extern void DefineSequence(CreateSeqStmt *stmt);
extern void AlterSequence(AlterSeqStmt *stmt);
Dhanaraj M <Dhanaraj.M@Sun.COM> writes:
> sorry for sending the old version in the previous mail . Here I attach
> the recent version of the patch file.
Surely this problem does not require adding any server-side code.
Something like "select last_value from <seq>" would be more appropriate;
and it'd have some hope of working with back-version servers.
Also, please use something more helpful than "***" as the column
header. Your urge to add a footnote to explain it shows that you
didn't try hard enough to devise a good header to begin with.
[ btw, both fmgroids.h and fmgrtab.c are generated files. Patching
them is unnecessary and inappropriate. ]
regards, tom lane
Tom Lane wrote:
>Dhanaraj M <Dhanaraj.M@Sun.COM> writes:
>
>
>>sorry for sending the old version in the previous mail . Here I attach
>>the recent version of the patch file.
>>
>>
>----------------------------------------------------------------------
>
>
Surely this problem does not require adding any server-side code.
>Something like "select last_value from <seq>" would be more appropriate;
>and it'd have some hope of working with back-version servers.
>
>Also, please use something more helpful than "***" as the column
>header. Your urge to add a footnote to explain it shows that you
>didn't try hard enough to devise a good header to begin with.
>
>[ btw, both fmgroids.h and fmgrtab.c are generated files. Patching
>them is unnecessary and inappropriate. ]
>
----------------------------------------------------------
The existing functions like lastval, currval dont provide the current
sequence value always.
They work only if the sequence is already cached (nextval is called
atleast once for that sequence).
Changing the internals of lastval/currval will give the solution.
However, I feel that the functionality change
may affect the customers who use the current version.
Hence, I am sure that it requires the server side change. There are two
options here
1. Modifying the exisitng functions (or) 2. adding new functions
Thanks for your review
Dhanaraj
As Tom asked, why not use seqname.last_value? Looking at your output:
+ if (showSeq && !showTables)
+ appendPQExpBuffer(&buf,
+ ",\n curval(c.oid) as \"%s\""
+ ",\n CASE curvalcheck(c.oid) WHEN '1' THEN '%s' WHEN '0' THEN '%s' END as \"%s\"",
+ _("value"),_(" ***"),_(""),_("Start from"));
What do you want to show that seqname.last_value doesn't give you?
Curval? I don't see that as useful for a psql display. Now that I look
at the TODO item:
o Have psql show current values for a sequence
It is confusing. It means "the current values" for the sequence, not
"curval" for the sequence. I don't even understand what your function
is returning. Just stick to last_value, though I think
seqname.is_called might be what you were looking for.
What fields do we want to show? Maybe the TODO item is not needed. Is
this all we want to show?
test=> \x
Expanded display is on.
test=> select * from xx;
-[ RECORD 1 ]-+--------------------
sequence_name | xx
last_value | 1
increment_by | 1
max_value | 9223372036854775807
min_value | 1
cache_value | 1
log_cnt | 32
is_cycled | f
is_called | t
---------------------------------------------------------------------------
Dhanaraj M wrote:
> Tom Lane wrote:
>
> >Dhanaraj M <Dhanaraj.M@Sun.COM> writes:
> >
> >
> >>sorry for sending the old version in the previous mail . Here I attach
> >>the recent version of the patch file.
> >>
> >>
> >----------------------------------------------------------------------
> >
> >
> Surely this problem does not require adding any server-side code.
>
> >Something like "select last_value from <seq>" would be more appropriate;
> >and it'd have some hope of working with back-version servers.
> >
> >Also, please use something more helpful than "***" as the column
> >header. Your urge to add a footnote to explain it shows that you
> >didn't try hard enough to devise a good header to begin with.
> >
> >[ btw, both fmgroids.h and fmgrtab.c are generated files. Patching
> >them is unnecessary and inappropriate. ]
> >
> ----------------------------------------------------------
>
> The existing functions like lastval, currval dont provide the current
> sequence value always.
> They work only if the sequence is already cached (nextval is called
> atleast once for that sequence).
> Changing the internals of lastval/currval will give the solution.
> However, I feel that the functionality change
> may affect the customers who use the current version.
>
> Hence, I am sure that it requires the server side change. There are two
> options here
> 1. Modifying the exisitng functions (or) 2. adding new functions
>
>
> Thanks for your review
> Dhanaraj
>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: don't forget to increase your free space map settings
>
--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Dhanaraj M wrote:
> Tom Lane wrote:
>
> >Dhanaraj M <Dhanaraj.M@Sun.COM> writes:
> >
> >
> >>sorry for sending the old version in the previous mail . Here I attach
> >>the recent version of the patch file.
> >>
> >>
> >----------------------------------------------------------------------
> >
> >
> Surely this problem does not require adding any server-side code.
>
> >Something like "select last_value from <seq>" would be more appropriate;
> >and it'd have some hope of working with back-version servers.
> >
> >Also, please use something more helpful than "***" as the column
> >header. Your urge to add a footnote to explain it shows that you
> >didn't try hard enough to devise a good header to begin with.
> >
> >[ btw, both fmgroids.h and fmgrtab.c are generated files. Patching
> >them is unnecessary and inappropriate. ]
> >
> ----------------------------------------------------------
>
> The existing functions like lastval, currval dont provide the current
> sequence value always.
> They work only if the sequence is already cached (nextval is called
> atleast once for that sequence).
> Changing the internals of lastval/currval will give the solution.
> However, I feel that the functionality change
> may affect the customers who use the current version.
>
> Hence, I am sure that it requires the server side change. There are two
> options here
> 1. Modifying the exisitng functions (or) 2. adding new functions
>
>
> Thanks for your review
> Dhanaraj
>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: don't forget to increase your free space map settings
>
--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote:
> What fields do we want to show? Maybe the TODO item is not needed. Is
> this all we want to show?
>
IRC what we want is something like this.
regression=# \d abc
Sequence "public.abc"
Column | Type
--------------+---------
sequence_name | abc
last_value | 1
increment_by | 1
max_value | 9223372036854775807
min_value | 1
cache_value | 1
log_cnt | 1
is_cycled | f
is_called | f
Because "\d abc" doesn't show us any important information.
regression=# \d abc
Sequence "public.abc"
Column | Type
---------------+---------
sequence_name | name
last_value | bigint
increment_by | bigint
max_value | bigint
min_value | bigint
cache_value | bigint
log_cnt | bigint
is_cycled | boolean
is_called | boolean
Last year, I made a patch for this but it was so ugly that I didn't send
to -patches. Maybe Bruce's solution (\x & select * from seq) could be
hardcoded in describe.c.
--
Euler Taveira de Oliveira
http://www.timbira.com/
I am thinking we just add another column to the \d display for sequences showing the current value. --------------------------------------------------------------------------- Euler Taveira de Oliveira wrote: > Bruce Momjian wrote: > > > What fields do we want to show? Maybe the TODO item is not needed. Is > > this all we want to show? > > > IRC what we want is something like this. > > regression=# \d abc > Sequence "public.abc" > Column | Type > --------------+--------- > sequence_name | abc > last_value | 1 > increment_by | 1 > max_value | 9223372036854775807 > min_value | 1 > cache_value | 1 > log_cnt | 1 > is_cycled | f > is_called | f > > > Because "\d abc" doesn't show us any important information. > > regression=# \d abc > Sequence "public.abc" > Column | Type > ---------------+--------- > sequence_name | name > last_value | bigint > increment_by | bigint > max_value | bigint > min_value | bigint > cache_value | bigint > log_cnt | bigint > is_cycled | boolean > is_called | boolean > > > Last year, I made a patch for this but it was so ugly that I didn't send > to -patches. Maybe Bruce's solution (\x & select * from seq) could be > hardcoded in describe.c. > > -- > Euler Taveira de Oliveira > http://www.timbira.com/ > > ---------------------------(end of broadcast)--------------------------- > TIP 3: Have you checked our extensive FAQ? > > http://www.postgresql.org/docs/faq > -- Bruce Momjian http://candle.pha.pa.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote:
>I am thinking we just add another column to the \d display for sequences
>showing the current value.
>
>---------------------------------------------------------------------------
>
>
>
As suggested in the previous mails, I tried to use the following to
display the seq. value.
select last_value from <seq>.
However, it was not possible to display the seq. value using this.
Hence, I made a small change in the currval() function, so that it
retrieves the last_value
even if the the value is not cached.
I hope this patch will be more suitable for this issue. Pl. look at the
patch.
Thanks
Dhanaraj
*** ./src/backend/commands/sequence.c.orig Tue May 2 14:51:03 2006
--- ./src/backend/commands/sequence.c Tue May 9 13:52:38 2006
***************
*** 605,610 ****
--- 605,612 ----
int64 result;
SeqTable elm;
Relation seqrel;
+ Form_pg_sequence seq;
+ Buffer buf;
/* open and AccessShareLock sequence */
init_sequence(relid, &elm, &seqrel);
***************
*** 616,632 ****
errmsg("permission denied for sequence %s",
RelationGetRelationName(seqrel))));
! if (elm->increment == 0) /* nextval/read_info were not called */
! ereport(ERROR,
! (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
! errmsg("currval of sequence \"%s\" is not yet defined in this session",
! RelationGetRelationName(seqrel))));
! result = elm->last;
! relation_close(seqrel, NoLock);
! PG_RETURN_INT64(result);
}
Datum
--- 618,641 ----
errmsg("permission denied for sequence %s",
RelationGetRelationName(seqrel))));
! if ((elm->increment != 0) ||(elm->last != elm->cached)) /* some numbers were cached */
! {
! result = elm->last;
! relation_close(seqrel, NoLock);
! PG_RETURN_INT64(result);
! }
! /* lock page' buffer and read tuple if not cached */
! seq = read_info(elm, seqrel, &buf);
! result = seq->last_value;
! UnlockReleaseBuffer(buf);
! relation_close(seqrel, NoLock);
! seqtab = elm->next;
! free(elm);
!
! PG_RETURN_INT64(result);
}
Datum
*** ./src/bin/psql/describe.c.orig Thu Apr 27 04:45:45 2006
--- ./src/bin/psql/describe.c Tue May 9 16:26:10 2006
***************
*** 1480,1485 ****
--- 1480,1488 ----
_("table"), _("view"), _("index"), _("sequence"),
_("special"), _("Type"), _("Owner"));
+ if (showSeq && !showTables)
+ appendPQExpBuffer(&buf,",\n currval(CAST(c.relname AS pg_catalog.text)) as \"%s\"",_("value"));
+
if (showIndexes)
appendPQExpBuffer(&buf,
",\n c2.relname as \"%s\"",
No differences encountered
Dhanaraj M <Dhanaraj.M@Sun.COM> writes:
> However, it was not possible to display the seq. value using this.
> Hence, I made a small change in the currval() function, so that it
> retrieves the last_value
> even if the the value is not cached.
Breaking currval()'s semantics is not an acceptable solution for this.
The best, fully backward compatible solution is for psql to issue
"SELECT last_value FROM <seq>" queries to get the values. This might
be a bit tricky to wedge into the structure of describe.c, but I don't
see any fundamental reason why it can't be done.
regards, tom lane
Hi all,
This patch was discussed a few months ago.
I could not complete this patch at that time.
I hope that the current version of my patch is acceptable.
Patch details:
**************
1. Assign a new field called 'Seq Value' for \ds command
2. All the sequence values are '1' initially
3. After executing the query, call AssignSeqValue()
4. This function assigns the respective sequence values back to the
resultset
Please review and comment on this patch.
Thanks
Dhanaraj
Tom Lane wrote:
>Dhanaraj M <Dhanaraj.M@Sun.COM> writes:
>
>
>>However, it was not possible to display the seq. value using this.
>>Hence, I made a small change in the currval() function, so that it
>>retrieves the last_value
>>even if the the value is not cached.
>>
>>
>
>Breaking currval()'s semantics is not an acceptable solution for this.
>
>The best, fully backward compatible solution is for psql to issue
>"SELECT last_value FROM <seq>" queries to get the values. This might
>be a bit tricky to wedge into the structure of describe.c, but I don't
>see any fundamental reason why it can't be done.
>
> regards, tom lane
>
>
*** src/bin/psql/describe.c.orig Mon Aug 21 11:21:56 2006
--- src/bin/psql/describe.c Thu Aug 24 10:54:59 2006
***************
*** 14,19 ****
--- 14,20 ----
#include "settings.h"
#include "print.h"
#include "variables.h"
+ #include "libpq-int.h"
#include <ctype.h>
***************
*** 38,43 ****
--- 39,45 ----
static bool add_tablespace_footer(char relkind, Oid tablespace, char **footers,
int *count, PQExpBufferData buf, bool newline);
+ static void AssignSeqValue(PGresult *resultset);
/*----------------
* Handlers for various slash commands displaying some sort of list
***************
*** 1499,1504 ****
--- 1501,1507 ----
bool showSeq = strchr(tabtypes, 's') != NULL;
bool showSystem = strchr(tabtypes, 'S') != NULL;
+ int64 initialSeqValue = 1;
PQExpBufferData buf;
PGresult *res;
printQueryOpt myopt = pset.popt;
***************
*** 1521,1526 ****
--- 1524,1533 ----
_("table"), _("view"), _("index"), _("sequence"),
_("special"), _("Type"), _("Owner"));
+ if ((showSeq) && (!showTables))
+ appendPQExpBuffer(&buf, ",\n " INT64_FORMAT " as \"%s\"",
+ initialSeqValue, _("Seq Value"));
+
if (showIndexes)
appendPQExpBuffer(&buf,
",\n c2.relname as \"%s\"",
***************
*** 1587,1592 ****
--- 1594,1602 ----
myopt.nullPrint = NULL;
myopt.title = _("List of relations");
+ if ((showSeq) && (!showTables))
+ AssignSeqValue(res);
+
printQuery(res, &myopt, pset.queryFout, pset.logfile);
}
***************
*** 1594,1599 ****
--- 1604,1641 ----
return true;
}
+
+ /*
+ * \ds
+ *
+ * Assign the respective sequence value.
+ */
+ static void AssignSeqValue(PGresult *resultset)
+ {
+ int i, rows, nfields;
+ PQExpBufferData buf;
+ PGresult *seqValue;
+
+ rows = PQntuples(resultset);
+ nfields = PQnfields(resultset);
+
+ /*
+ * Execute the select query to get the sequence value for each sequence separately,
+ * by using the retrieved sequence names from the second field of resultset.
+ * Re-assign the respective sequence values to the last field of resultset.
+ */
+ for(i=0; i<rows; i++)
+ {
+ initPQExpBuffer(&buf);
+ printfPQExpBuffer(&buf, "select last_value from %s", PQgetvalue(resultset, i, 1));
+
+ seqValue = PSQLexec(buf.data, false);
+ termPQExpBuffer(&buf);
+ strcpy(resultset->tuples[i][nfields-1].value, seqValue->tuples[0][0].value);
+ PQclear(seqValue);
+ }
+ }
+
/*
* \dD
Sorry for resubmitting this patch.
Just now I found a problem.
Instead of assigning initial sequence value to 1,
I assign LLONG_MAX to avoid the buffer overflow problem.
Please find the current version here.
Dhanaraj M wrote:
> Hi all,
>
> This patch was discussed a few months ago.
> I could not complete this patch at that time.
> I hope that the current version of my patch is acceptable.
>
> Patch details:
> **************
> 1. Assign a new field called 'Seq Value' for \ds command
> 2. All the sequence values are '1' initially
> 3. After executing the query, call AssignSeqValue()
> 4. This function assigns the respective sequence values back to the
> resultset
>
>
> Please review and comment on this patch.
>
> Thanks
> Dhanaraj
>
> Tom Lane wrote:
>
>> Dhanaraj M <Dhanaraj.M@Sun.COM> writes:
>>
>>
>>> However, it was not possible to display the seq. value using this.
>>> Hence, I made a small change in the currval() function, so that it
>>> retrieves the last_value
>>> even if the the value is not cached.
>>>
>>
>>
>> Breaking currval()'s semantics is not an acceptable solution for this.
>>
>> The best, fully backward compatible solution is for psql to issue
>> "SELECT last_value FROM <seq>" queries to get the values. This might
>> be a bit tricky to wedge into the structure of describe.c, but I don't
>> see any fundamental reason why it can't be done.
>>
>> regards, tom lane
>>
>>
*** src/bin/psql/describe.c.orig Mon Aug 21 11:21:56 2006
--- src/bin/psql/describe.c Thu Aug 24 17:18:21 2006
***************
*** 14,20 ****
--- 14,22 ----
#include "settings.h"
#include "print.h"
#include "variables.h"
+ #include "libpq-int.h"
+ #include <limits.h>
#include <ctype.h>
#ifdef WIN32
***************
*** 38,43 ****
--- 40,46 ----
static bool add_tablespace_footer(char relkind, Oid tablespace, char **footers,
int *count, PQExpBufferData buf, bool newline);
+ static void AssignSeqValue(PGresult *resultset);
/*----------------
* Handlers for various slash commands displaying some sort of list
***************
*** 1499,1504 ****
--- 1502,1508 ----
bool showSeq = strchr(tabtypes, 's') != NULL;
bool showSystem = strchr(tabtypes, 'S') != NULL;
+ int64 initialSeqValue = LLONG_MAX;
PQExpBufferData buf;
PGresult *res;
printQueryOpt myopt = pset.popt;
***************
*** 1521,1526 ****
--- 1525,1534 ----
_("table"), _("view"), _("index"), _("sequence"),
_("special"), _("Type"), _("Owner"));
+ if ((showSeq) && (!showTables))
+ appendPQExpBuffer(&buf, ",\n " INT64_FORMAT " as \"%s\"",
+ initialSeqValue, _("Seq Value"));
+
if (showIndexes)
appendPQExpBuffer(&buf,
",\n c2.relname as \"%s\"",
***************
*** 1587,1592 ****
--- 1595,1603 ----
myopt.nullPrint = NULL;
myopt.title = _("List of relations");
+ if ((showSeq) && (!showTables))
+ AssignSeqValue(res);
+
printQuery(res, &myopt, pset.queryFout, pset.logfile);
}
***************
*** 1594,1599 ****
--- 1605,1642 ----
return true;
}
+
+ /*
+ * \ds
+ *
+ * Assign the respective sequence value.
+ */
+ static void AssignSeqValue(PGresult *resultset)
+ {
+ int i, rows, nfields;
+ PQExpBufferData buf;
+ PGresult *seqValue;
+
+ rows = PQntuples(resultset);
+ nfields = PQnfields(resultset);
+
+ /*
+ * Execute the select query to get the sequence value for each sequence separately,
+ * by using the retrieved sequence names from the second field of resultset.
+ * Re-assign the respective sequence values to the last field of resultset.
+ */
+ for(i=0; i<rows; i++)
+ {
+ initPQExpBuffer(&buf);
+ printfPQExpBuffer(&buf, "select last_value from %s", PQgetvalue(resultset, i, 1));
+
+ seqValue = PSQLexec(buf.data, false);
+ termPQExpBuffer(&buf);
+ strcpy(resultset->tuples[i][nfields-1].value, seqValue->tuples[0][0].value);
+ PQclear(seqValue);
+ }
+ }
+
/*
* \dD
Dhanaraj M <Dhanaraj.M@Sun.COM> writes:
> Sorry for resubmitting this patch.
> Just now I found a problem.
> Instead of assigning initial sequence value to 1,
> I assign LLONG_MAX to avoid the buffer overflow problem.
> Please find the current version here.
This patch is a mess. In the first place, it's completely unkosher for
an application to scribble on a PGresult's contents, even if you do take
steps like the above to try to make sure there's enough space. But said
step does not work anyway -- LLONG_MAX might not exist on the client, or
might exist but be smaller than the server's value.
Another problem with it is it's not schema-aware and not proof against
quoting requirements for the sequence name (try it with a mixed-case
sequence name for instance). It also ought to pay some attention to
the possibility that the SELECT for last_value fails --- quite aside
from communication failure or such, there might be a permissions problem
preventing the last_value from being read.
regards, tom lane
Due to Tom's feedback:
This has been saved for the 8.3 release:
http://momjian.postgresql.org/cgi-bin/pgpatches_hold
---------------------------------------------------------------------------
Dhanaraj M wrote:
>
> Sorry for resubmitting this patch.
> Just now I found a problem.
> Instead of assigning initial sequence value to 1,
> I assign LLONG_MAX to avoid the buffer overflow problem.
> Please find the current version here.
>
>
> Dhanaraj M wrote:
>
> > Hi all,
> >
> > This patch was discussed a few months ago.
> > I could not complete this patch at that time.
> > I hope that the current version of my patch is acceptable.
> >
> > Patch details:
> > **************
> > 1. Assign a new field called 'Seq Value' for \ds command
> > 2. All the sequence values are '1' initially
> > 3. After executing the query, call AssignSeqValue()
> > 4. This function assigns the respective sequence values back to the
> > resultset
> >
> >
> > Please review and comment on this patch.
> >
> > Thanks
> > Dhanaraj
> >
> > Tom Lane wrote:
> >
> >> Dhanaraj M <Dhanaraj.M@Sun.COM> writes:
> >>
> >>
> >>> However, it was not possible to display the seq. value using this.
> >>> Hence, I made a small change in the currval() function, so that it
> >>> retrieves the last_value
> >>> even if the the value is not cached.
> >>>
> >>
> >>
> >> Breaking currval()'s semantics is not an acceptable solution for this.
> >>
> >> The best, fully backward compatible solution is for psql to issue
> >> "SELECT last_value FROM <seq>" queries to get the values. This might
> >> be a bit tricky to wedge into the structure of describe.c, but I don't
> >> see any fundamental reason why it can't be done.
> >>
> >> regards, tom lane
> >>
> >>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Have you searched our list archives?
>
> http://archives.postgresql.org
--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Your patch has been added to the PostgreSQL unapplied patches list at:
http://momjian.postgresql.org/cgi-bin/pgpatches
It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.
---------------------------------------------------------------------------
Dhanaraj M wrote:
>
> Sorry for resubmitting this patch.
> Just now I found a problem.
> Instead of assigning initial sequence value to 1,
> I assign LLONG_MAX to avoid the buffer overflow problem.
> Please find the current version here.
>
>
> Dhanaraj M wrote:
>
> > Hi all,
> >
> > This patch was discussed a few months ago.
> > I could not complete this patch at that time.
> > I hope that the current version of my patch is acceptable.
> >
> > Patch details:
> > **************
> > 1. Assign a new field called 'Seq Value' for \ds command
> > 2. All the sequence values are '1' initially
> > 3. After executing the query, call AssignSeqValue()
> > 4. This function assigns the respective sequence values back to the
> > resultset
> >
> >
> > Please review and comment on this patch.
> >
> > Thanks
> > Dhanaraj
> >
> > Tom Lane wrote:
> >
> >> Dhanaraj M <Dhanaraj.M@Sun.COM> writes:
> >>
> >>
> >>> However, it was not possible to display the seq. value using this.
> >>> Hence, I made a small change in the currval() function, so that it
> >>> retrieves the last_value
> >>> even if the the value is not cached.
> >>>
> >>
> >>
> >> Breaking currval()'s semantics is not an acceptable solution for this.
> >>
> >> The best, fully backward compatible solution is for psql to issue
> >> "SELECT last_value FROM <seq>" queries to get the values. This might
> >> be a bit tricky to wedge into the structure of describe.c, but I don't
> >> see any fundamental reason why it can't be done.
> >>
> >> regards, tom lane
> >>
> >>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Have you searched our list archives?
>
> http://archives.postgresql.org
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > > Your patch has been added to the PostgreSQL unapplied patches list at: > > http://momjian.postgresql.org/cgi-bin/pgpatches > > It will be applied as soon as one of the PostgreSQL committers reviews > and approves it. I think the SELECT query is short on quoting and schema-qualification for the target sequence. Also it should probably be adjusted to use strlcpy instead of strcpy. Coding style seems a bit off. Also, since this is likely to be expensive, it may be better to show the value only on \dS+, not plain \dS. Not sure what else. That's what jumps at me. I think we discussed replacing the current sequence representation with a single relation that would contain all sequences in the database, so that you could do "select * from pg_sequence" and get all the values in one go. The idea was considered not implementable at the time due to us not having "non transactional relations", but I'd suggest adding it to the TODO so that we don't forget later. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera wrote: > Bruce Momjian wrote: > > > > Your patch has been added to the PostgreSQL unapplied patches list at: > > > > http://momjian.postgresql.org/cgi-bin/pgpatches > > > > It will be applied as soon as one of the PostgreSQL committers reviews > > and approves it. > > I think the SELECT query is short on quoting and schema-qualification > for the target sequence. > > Also it should probably be adjusted to use strlcpy instead of strcpy. > Coding style seems a bit off. Also, since this is likely to be > expensive, it may be better to show the value only on \dS+, not plain > \dS. > > Not sure what else. That's what jumps at me. Yep, for a small patch, it needs major cleanups. > I think we discussed replacing the current sequence representation with > a single relation that would contain all sequences in the database, so > that you could do "select * from pg_sequence" and get all the values in > one go. The idea was considered not implementable at the time due to us > not having "non transactional relations", but I'd suggest adding it to > the TODO so that we don't forget later. OK: * Consider placing all sequences in a single table, now that system tables are full transactional -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Alvaro Herrera <alvherre@commandprompt.com> writes:
> I think we discussed replacing the current sequence representation with
> a single relation that would contain all sequences in the database, so
> that you could do "select * from pg_sequence" and get all the values in
> one go. The idea was considered not implementable at the time due to us
> not having "non transactional relations", but I'd suggest adding it to
> the TODO so that we don't forget later.
I wonder if we could kluge it by making all sequences inheritance
children of a dummy "pg_sequence" relation. Or perhaps better,
implement this as a system view on a set-returning function --- if there
are lots of sequences the planner's not likely to help you much anyway.
regards, tom lane
Tom Lane wrote: > Alvaro Herrera <alvherre@commandprompt.com> writes: > > I think we discussed replacing the current sequence representation with > > a single relation that would contain all sequences in the database, so > > that you could do "select * from pg_sequence" and get all the values in > > one go. The idea was considered not implementable at the time due to us > > not having "non transactional relations", but I'd suggest adding it to > > the TODO so that we don't forget later. > > I wonder if we could kluge it by making all sequences inheritance > children of a dummy "pg_sequence" relation. Or perhaps better, > implement this as a system view on a set-returning function --- if there > are lots of sequences the planner's not likely to help you much anyway. I like the system view best. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Based on this patch review, I am removing the patch from the patch queue and requiring a resubmission. --------------------------------------------------------------------------- Tom Lane wrote: > Dhanaraj M <Dhanaraj.M@Sun.COM> writes: > > Sorry for resubmitting this patch. > > Just now I found a problem. > > Instead of assigning initial sequence value to 1, > > I assign LLONG_MAX to avoid the buffer overflow problem. > > Please find the current version here. > > This patch is a mess. In the first place, it's completely unkosher for > an application to scribble on a PGresult's contents, even if you do take > steps like the above to try to make sure there's enough space. But said > step does not work anyway -- LLONG_MAX might not exist on the client, or > might exist but be smaller than the server's value. > > Another problem with it is it's not schema-aware and not proof against > quoting requirements for the sequence name (try it with a mixed-case > sequence name for instance). It also ought to pay some attention to > the possibility that the SELECT for last_value fails --- quite aside > from communication failure or such, there might be a permissions problem > preventing the last_value from being read. > > regards, tom lane > > ---------------------------(end of broadcast)--------------------------- > TIP 9: In versions below 8.0, the planner will ignore your desire to > choose an index scan if your joining column's datatypes do not > match -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Patch removed from queue based on patch review. Resubmit. --------------------------------------------------------------------------- Dhanaraj M wrote: > > Sorry for resubmitting this patch. > Just now I found a problem. > Instead of assigning initial sequence value to 1, > I assign LLONG_MAX to avoid the buffer overflow problem. > Please find the current version here. > > > Dhanaraj M wrote: > > > Hi all, > > > > This patch was discussed a few months ago. > > I could not complete this patch at that time. > > I hope that the current version of my patch is acceptable. > > > > Patch details: > > ************** > > 1. Assign a new field called 'Seq Value' for \ds command > > 2. All the sequence values are '1' initially > > 3. After executing the query, call AssignSeqValue() > > 4. This function assigns the respective sequence values back to the > > resultset > > > > > > Please review and comment on this patch. > > > > Thanks > > Dhanaraj > > > > Tom Lane wrote: > > > >> Dhanaraj M <Dhanaraj.M@Sun.COM> writes: > >> > >> > >>> However, it was not possible to display the seq. value using this. > >>> Hence, I made a small change in the currval() function, so that it > >>> retrieves the last_value > >>> even if the the value is not cached. > >>> > >> > >> > >> Breaking currval()'s semantics is not an acceptable solution for this. > >> > >> The best, fully backward compatible solution is for psql to issue > >> "SELECT last_value FROM <seq>" queries to get the values. This might > >> be a bit tricky to wedge into the structure of describe.c, but I don't > >> see any fundamental reason why it can't be done. > >> > >> regards, tom lane > >> > >> > > > ---------------------------(end of broadcast)--------------------------- > TIP 4: Have you searched our list archives? > > http://archives.postgresql.org -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +