Re: [PATCHES] patch: garbage error strings in libpq
Tom Lane wrote: [EMAIL PROTECTED] writes: Another approach would have been to make libpq_gettext() preserve errno. That seems like a far easier, cleaner, and more robust fix than this. Provided that either: (a) the C standard has added a sequence point between the arguments in a function call, which AFAIK wasn't there before, or the sequence point was there all along (and the compiler implements it); (b) the compiler is sufficiently naive; (c) you get lucky with instruction scheduling on your particular architecture. This is why I called this approach was tempting, but didn't go for it. I felt it was better to really fix the instances I found first, then see what patterns emerge and refactor. Like maybe a wrapper for printfPQExpBuffer() that takes a PGconn *, an untranslated format string, and varargs; this in turn can do the libpq_gettext(). That would cover all uses of printfPQExpBuffer() in libpq--except for one of the out-of-memory errors where no translation is done, which may have been unintentional (and this bug is again duplicated in the code). Moreover I don't believe that this approach works either, as the result of strerror() is not guaranteed still usable after another strerror call (ie, it can use a static buffer repeatedly), so you'd still have the problem if libpq_gettext invokes strerror. I suppose that a really robust solution would involve libpq_gettext saving errno, restoring errno, and invoking strerror() again ... Check again. The calls to strerror() are routed through pqStrerror() which copies the error message to the buffer, or in the case of GNU strerror_r(), at least ensures it is in some reusable location. Jeroen ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send unregister YourEmailAddressHere to [EMAIL PROTECTED])
Re: [PATCHES] Error handling fix in interfaces/libpq/fe-secure.c
Tom Lane wrote: The gettext function does not modify the value of the global errno variable. This is necessary to make it possible to write something like printf (gettext (Operation failed: %m\n)); which is pretty much what I expected to find. Ergo, this entire discussion is wrong, and whatever bug you are concerned about will not be solved this way. Tom, I didn't know that gettext() preserved errno--but I still believe you're wrong. The problem is not gettext() but libpq_gettext(). The latter calls the former, but it may go through initialization first--which would still pollute errno on the first call. And that first call may well be the failed to connect message that so many people have been seeing replaced with garbage data. Given that gettext() doesn't pollute errno, you'd see this problem occur only if the first error message you got from libpq included an errno-based string. What you may actually be running into is the problem that there are two different definitions of strerror_r() out there, the SUS spec and the GNU spec, and pre-8.0 we failed to distinguish these. The 7.4 coding will yield garbage messages in some cases when GNU strerror_r is in use. Actually I'm fairly sure that people have been seeing the problem with 8.0. In fact I had so much trouble getting a portable, reliable result for my strerror_r check in the libpqxx configure script that I ended up checking for both versions, and then verifying that at least one of the checks failed. I use function pointer assignments now, but I may end up adding the repeated declaration method of checking back in as well. My situation is a bit different from that of postgres since the base language is C++. On a side note, is there any risk of a packager building libpq against a GNU-style strerror_r() and the user who downloads the binary then loading it against a SUS-style strerror_r() or vice versa? Jeroen ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] patch: garbage error strings in libpq
[EMAIL PROTECTED] wrote: (a) the C standard has added a sequence point between the arguments in a function call, which AFAIK wasn't there before, or the sequence point was there all along (and the compiler implements it); Per C99 6.5.2.2.10, a sequence point occurs between the evaluation of the arguments to a function and the call of the function itself. Therefore a sequence point occurs before the call to libpq_gettext(). So ISTM having libpq_gettext() preserve errno should work. -Neil ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] Disable page writes when fsync off, add GUC
Bruce Momjian wrote: Bruce Momjian wrote: This also adds a full_page_writes GUC to turn off page writes to WAL. Some people might not want full_page_writes. Fsync linkage removed, patch attached and applied. ... + When this option is on, the productnamePostgreSQL/ server + writes full pages to WAL when they first modified after a checkpoint + so full recovery is possible. I believe this should be when they _are_ first modified after. Perhaps you should also mention power failure, not only an operating system crash as disaster scenario, even if the latter includes the former. Best Regards, Michael Paesold ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] INSERT ... RETURNING
Tom Lane wrote: - should work for UPDATE and DELETE too And probably INSERT ... SELECT as well. -Neil ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] patch: garbage error strings in libpq
Neil Conway wrote: Per C99 6.5.2.2.10, a sequence point occurs between the evaluation of the arguments to a function and the call of the function itself. Therefore a sequence point occurs before the call to libpq_gettext(). So ISTM having libpq_gettext() preserve errno should work. In C99, at least. But that's not the dialect postgres is written in; even gcc 4.0 leaves C99 support turned off by default. Does anyone know what the situation is in C89, or whatever the applicable standard is? Jeroen ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] patch: garbage error strings in libpq
[EMAIL PROTECTED] wrote: Does anyone know what the situation is in C89, or whatever the applicable standard is? [ *looks* ] The text is the same in both versions: http://dev.unicals.com/papers/c89-draft.html#3.3.2.2 The order of evaluation of the function designator, the arguments, and subexpressions within the arguments is unspecified, but there is a sequence point before the actual call. (On reading this more closely, I suppose you could make the argument that a function call that takes place in the argument list of another function call is a subexpression within the [outer function's] arguments, so the order of evaluation prior to the call of the outer function would be undefined. But I don't think that's the right reading of the standard.) -Neil ---(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
Re: [HACKERS] [PATCHES] Dbsize backend integration
-Original Message- From: Bruce Momjian [mailto:[EMAIL PROTECTED] Sent: 06 July 2005 04:11 To: Tom Lane Cc: Dave Page; Christopher Kings-Lynne; Robert Treat; Dawid Kuroczko; Andreas Pflug; PostgreSQL-patches; PostgreSQL-development Subject: Re: [HACKERS] [PATCHES] Dbsize backend integration Tom Lane wrote: pg_relation_size plus pg_complete_relation_size is fine. Ship it... OK. Updated version attached. Regards, Dave. dbsize.c Description: dbsize.c dbsize.patch Description: dbsize.patch ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] patch: garbage error strings in libpq
[EMAIL PROTECTED] wrote: That is pretty much what I remember hearing at the time. A well-known way to trigger undefined behaviour is x++=x++; because there is no sequence point between the two side effects. Try it: gcc will give you a stern warning. Given that there is no sequence point between argument expressions, as per the paragraph you quote, the same must go for c(x++,x++). So then it becomes dubious that there is suddenly a guarantee for c(a(),b())! Right; my interpretation is that the sequence point before function call rule applies recursively. So in c(a(...), b(...)), there are in fact three sequence points, which precede the calls of a, b, and c. Shouldn't that be sufficient to ensure that the evaluation of libpq_gettext() is not interleaved with the evaluation of the other arguments to the printf()? -Neil ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] patch: garbage error strings in libpq
Neil Conway [EMAIL PROTECTED] writes: Right; my interpretation is that the sequence point before function call rule applies recursively. So in c(a(...), b(...)), there are in fact three sequence points, which precede the calls of a, b, and c. Shouldn't that be sufficient to ensure that the evaluation of libpq_gettext() is not interleaved with the evaluation of the other arguments to the printf()? I think this is all irrelevant language-lawyering; jtv spotted the true problem which is that we do not protect errno during the *first* call of libpq_gettext. regards, tom lane ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] patch: garbage error strings in libpq
Tom Lane wrote: I think this is all irrelevant language-lawyering; jtv spotted the true problem which is that we do not protect errno during the *first* call of libpq_gettext. I think you're missing the point. Obviously the current code is wrong, the debate is over the best way to fix it. Jeroen's interpretation of the spec suggests that merely having libpq_gettext() preserve errno is not sufficient. I'm not convinced this his interpretation is correct, but it is a question worth resolving. -Neil ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] Disable page writes when fsync off, add GUC
Michael Paesold wrote: Bruce Momjian wrote: Bruce Momjian wrote: This also adds a full_page_writes GUC to turn off page writes to WAL. Some people might not want full_page_writes. Fsync linkage removed, patch attached and applied. ... + When this option is on, the productnamePostgreSQL/ server + writes full pages to WAL when they first modified after a checkpoint + so full recovery is possible. I believe this should be when they _are_ first modified after. Perhaps you should also mention power failure, not only an operating system crash as disaster scenario, even if the latter includes the former. Thanks. Done. -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 Index: doc/src/sgml/runtime.sgml === RCS file: /cvsroot/pgsql/doc/src/sgml/runtime.sgml,v retrieving revision 1.336 diff -c -c -r1.336 runtime.sgml *** doc/src/sgml/runtime.sgml 5 Jul 2005 23:18:09 - 1.336 --- doc/src/sgml/runtime.sgml 6 Jul 2005 14:40:15 - *** *** 1705,1715 para When this option is on, the productnamePostgreSQL/ server ! writes full pages to WAL when they first modified after a checkpoint ! so full recovery is possible. Turning this option off might lead ! to a corrupt system after an operating system crash because ! uncorrected partial pages might contain inconsistent or corrupt ! data. The risks are less but similar to varnamefsync/. /para para --- 1705,1716 para When this option is on, the productnamePostgreSQL/ server ! writes full pages to WAL when they are first modified after a ! checkpoint so full recovery is possible. Turning this option off ! might lead to a corrupt system after an operating system crash ! or power failure because uncorrected partial pages might contain ! inconsistent or corrupt data. The risks are less but similar to ! varnamefsync/. /para para ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] User's exception plpgsql
Pavel Stehule wrote: Per small recent discussion I corrected patch user's exception. Attached is a revised patch. I haven't looked at the documentation changes yet (more work is needed I believe) or some of the error message text. I was originally hoping to make exception variables a little more full-featured -- it seems silly to DECLARE something that cannot be initialized with the value of another expression, for example. I can also see how it would be useful to evaluate an expression variable (e.g. to print it out for debugging purposes). It would be possible extend the operations allowed upon exception variables, thinking about this further, I wonder if there is any point introducing the concept of an exception variable in the first place. What does it buy us over simply using a string? In other words, if we allowed the syntax: RAISE LEVEL [ opt_sqlstate ] 'fmt' [, expr ... ] where `opt_sqlstate' is either empty, a T_WORD we find in the table of predefined condition names, or an expression that evaluates to a text value. The text value must be of a certain form (e.g. 5 characters in length, begins with a U and so on). It might be slightly more difficult to parse this (especially if we allow 'fmt' to be an expression yielding a string, not just a string literal), but I don't think it is ambiguous and can be sorted out via yylex(). -Neil Index: doc/src/sgml/plpgsql.sgml === RCS file: /Users/neilc/local/cvs/pgsql/doc/src/sgml/plpgsql.sgml,v retrieving revision 1.75 diff -c -r1.75 plpgsql.sgml *** doc/src/sgml/plpgsql.sgml 2 Jul 2005 08:59:47 - 1.75 --- doc/src/sgml/plpgsql.sgml 6 Jul 2005 13:26:22 - *** *** 2117,2123 para The replaceablecondition/replaceable names can be any of those shown in xref linkend=errcodes-appendix. A category name matches ! any error within its category. The special condition name literalOTHERS/ matches every error type except literalQUERY_CANCELED/. (It is possible, but often unwise, to trap --- 2117,2125 para The replaceablecondition/replaceable names can be any of those shown in xref linkend=errcodes-appendix. A category name matches ! any error within its category. You can use exception variable as ! condition name. Exception variable is declared with type ! literalEXCEPTION/literal The special condition name literalOTHERS/ matches every error type except literalQUERY_CANCELED/. (It is possible, but often unwise, to trap *** *** 2571,2577 raise errors. synopsis ! RAISE replaceable class=parameterlevel/replaceable 'replaceable class=parameterformat/replaceable' optional, replaceable class=parameterexpression/replaceable optional, .../optional/optional; /synopsis Possible levels are literalDEBUG/literal, --- 2573,2580 raise errors. synopsis ! RAISE replaceable class=parameterlevel/replaceable ! optionalsystem exception|exception variable/optional 'replaceable class=parameterformat/replaceable' optional, replaceable class=parameterexpression/replaceable optional, .../optional/optional; /synopsis Possible levels are literalDEBUG/literal, *** *** 2588,2593 --- 2591,2600 variables. See xref linkend=runtime-config for more information. /para + +para +You can specify any system exception or any user exception. +/para para Inside the format string, literal%/literal is replaced by the Index: src/include/utils/elog.h === RCS file: /Users/neilc/local/cvs/pgsql/src/include/utils/elog.h,v retrieving revision 1.79 diff -c -r1.79 elog.h *** src/include/utils/elog.h10 Jun 2005 16:23:10 - 1.79 --- src/include/utils/elog.h6 Jul 2005 13:26:22 - *** *** 61,66 --- 61,72 (PGSIXBIT(ch1) + (PGSIXBIT(ch2) 6) + (PGSIXBIT(ch3) 12) + \ (PGSIXBIT(ch4) 18) + (PGSIXBIT(ch5) 24)) + #define MAKE_SQLSTATE_STR(str) \ + ( \ + AssertMacro(strlen(str) == 5), \ + MAKE_SQLSTATE(str[0], str[1], str[2], str[3], str[4]) \ + ) + /* These macros depend on the fact that '0' becomes a zero in SIXBIT */ #define ERRCODE_TO_CATEGORY(ec) ((ec) ((1 12) - 1)) #define ERRCODE_IS_CATEGORY(ec) (((ec) ~((1 12) - 1)) == 0) Index: src/pl/plpgsql/src/gram.y === RCS file: /Users/neilc/local/cvs/pgsql/src/pl/plpgsql/src/gram.y,v retrieving revision 1.80 diff -c -r1.80 gram.y *** src/pl/plpgsql/src/gram.y 2 Jul 2005 17:01:59 - 1.80 --- src/pl/plpgsql/src/gram.y 6 Jul 2005 13:38:35 - *** *** 103,108 --- 103,109 PLpgSQL_exception_block *exception_block; PLpgSQL_nsitem
[PATCHES] plperl SRF sanity check fix
The attached patch moves a plperl sanity check into the correct
position. Performing the check in the existing position allows the call
to go through to perl first, possibly resulting in a SEGV.
cheers
andrew
Index: plperl.c
===
RCS file: /projects/cvsroot/pgsql/src/pl/plperl/plperl.c,v
retrieving revision 1.79
diff -c -r1.79 plperl.c
*** plperl.c 3 Jul 2005 21:56:16 - 1.79
--- plperl.c 6 Jul 2005 15:46:52 -
***
*** 850,855
--- 850,867
prodesc-tuple_store = 0;
prodesc-tuple_desc = 0;
+ rsi = (ReturnSetInfo *)fcinfo-resultinfo;
+
+ if (prodesc-fn_retisset (!rsi || !IsA(rsi, ReturnSetInfo) ||
+ (rsi-allowedModes SFRM_Materialize) == 0 ||
+ rsi-expectedDesc == NULL))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg(set-valued function called in context that
+ cannot accept a set)));
+ }
+
perlret = plperl_call_perl_func(prodesc, fcinfo);
/
***
*** 861,879
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, SPI_finish() failed);
! rsi = (ReturnSetInfo *)fcinfo-resultinfo;
!
! if (prodesc-fn_retisset) {
! if (!rsi || !IsA(rsi, ReturnSetInfo) ||
! (rsi-allowedModes SFRM_Materialize) == 0 ||
! rsi-expectedDesc == NULL)
! {
! ereport(ERROR,
! (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! errmsg(set-valued function called in context that
! cannot accept a set)));
! }
!
/* If the Perl function returned an arrayref, we pretend that it
* called return_next() for each element of the array, to handle
* old SRFs that didn't know about return_next(). Any other sort
--- 873,880
if (SPI_finish() != SPI_OK_FINISH)
elog(ERROR, SPI_finish() failed);
! if (prodesc-fn_retisset)
! {
/* If the Perl function returned an arrayref, we pretend that it
* called return_next() for each element of the array, to handle
* old SRFs that didn't know about return_next(). Any other sort
---(end of broadcast)---
TIP 5: Have you checked our extensive FAQ?
http://www.postgresql.org/docs/faq
[PATCHES] ecpg: fix ECPGstore_input()
This patch fixes the following issues in ECPGstore_input():
- strlen() was invoked on the NULL pointer for the first iteration of
the loop (line 875, 923, 966, 1009)
- `nval' is freed for every iteration of the loop at 864, but only
initialized once outside the loop, resulting in potential multiple
free()'s, as well as the use of a freed variable in subsequent iterations
- `str' was leaked for every subsequent iteration of the loop (line 871,
920, 963, 1006)
- the return value of PGTYPESinterval_to_asc() is leaked at line 920 and
937; the return value of PGTYPESdate_to_asc() is leaked at line 963 and
980; the return value of PGTYPEStimestamp_to_asc() is leaked at line
1006 and 1023.
- malloc failure is in general not handled well; the function simply
returns without bothering to clean up allocated resources, and many
return values are not checked for errors.
Also, in create_statement(), `*stmt' was dereferenced before being
initialized.
Per the Coverity report run by EnterpriseDB. Thanks to Eric Astor at EDB
for an initial version of this patch -- the attached version has been
improved by myself.
Barring any objections, I'd like to apply this to CVS in a day or two (I
want to test it first, which I haven't yet done).
-Neil
Index: src/interfaces/ecpg/ecpglib/execute.c
===
RCS file: /var/lib/cvs/pgsql/src/interfaces/ecpg/ecpglib/execute.c,v
retrieving revision 1.41
diff -c -r1.41 execute.c
*** src/interfaces/ecpg/ecpglib/execute.c 2 Jul 2005 17:01:53 - 1.41
--- src/interfaces/ecpg/ecpglib/execute.c 4 Jul 2005 05:52:51 -
***
*** 143,149
static bool
create_statement(int lineno, int compat, int force_indicator, struct connection * connection, struct statement ** stmt, char *query, va_list ap)
{
! struct variable **list = ((*stmt)-inlist);
enum ECPGttype type;
if (!(*stmt = (struct statement *) ECPGalloc(sizeof(struct statement), lineno)))
--- 143,149
static bool
create_statement(int lineno, int compat, int force_indicator, struct connection * connection, struct statement ** stmt, char *query, va_list ap)
{
! struct variable **list;
enum ECPGttype type;
if (!(*stmt = (struct statement *) ECPGalloc(sizeof(struct statement), lineno)))
***
*** 856,1039
case ECPGt_numeric:
{
char *str = NULL;
int slen;
numeric*nval = PGTYPESnumeric_new();
if (var-arrsize 1)
{
for (element = 0; element var-arrsize; element++)
{
if (var-type == ECPGt_numeric)
! PGTYPESnumeric_copy((numeric *) ((var + var-offset * element)-value), nval);
else
! PGTYPESnumeric_from_decimal((decimal *) ((var + var-offset * element)-value), nval);
str = PGTYPESnumeric_to_asc(nval, nval-dscale);
! PGTYPESnumeric_free(nval);
slen = strlen(str);
! if (!(mallocedval = ECPGrealloc(mallocedval, strlen(mallocedval) + slen + sizeof(array [] ), lineno)))
! return false;
! if (!element)
strcpy(mallocedval, array [);
strncpy(mallocedval + strlen(mallocedval), str, slen + 1);
strcpy(mallocedval + strlen(mallocedval), ,);
}
strcpy(mallocedval + strlen(mallocedval) - 1, ]);
}
else
{
if (var-type == ECPGt_numeric)
! PGTYPESnumeric_copy((numeric *) (var-value), nval);
else
! PGTYPESnumeric_from_decimal((decimal *) (var-value), nval);
str = PGTYPESnumeric_to_asc(nval, nval-dscale);
!
! PGTYPESnumeric_free(nval);
slen = strlen(str);
if (!(mallocedval = ECPGalloc(slen + 1, lineno)))
! return false;
! strncpy(mallocedval, str, slen);
mallocedval[slen] = '\0';
}
*tobeinserted_p = mallocedval;
*malloced_p = true;
! free(str);
}
- break;
case ECPGt_interval:
{
char *str = NULL;
int slen;
if (var-arrsize 1)
{
for (element = 0; element var-arrsize; element++)
{
! str = quote_postgres(PGTYPESinterval_to_asc((interval *) ((var + var-offset * element)-value)), lineno);
slen = strlen(str);
! if (!(mallocedval = ECPGrealloc(mallocedval, strlen(mallocedval) + slen + sizeof(array [],interval ), lineno)))
return false;
! if (!element)
strcpy(mallocedval, array [);
strcpy(mallocedval + strlen(mallocedval), interval );
strncpy(mallocedval + strlen(mallocedval), str, slen + 1);
strcpy(mallocedval + strlen(mallocedval), ,);
}
strcpy(mallocedval + strlen(mallocedval) - 1, ]);
}
else
{
! str = quote_postgres(PGTYPESinterval_to_asc((interval *) (var-value)), lineno);
slen = strlen(str);
if (!(mallocedval = ECPGalloc(slen + sizeof(interval ) + 1,
Re: [PATCHES] User's exception plpgsql
Neil Conway [EMAIL PROTECTED] writes:
I wonder if there is any point introducing the concept of an
exception variable in the first place. What does it buy us over simply
using a string?
Not a lot really, except for keeping things similar to the Oracle way of
doing it ... but that's a nontrivial consideration.
RAISE LEVEL [ opt_sqlstate ] 'fmt' [, expr ... ]
It might be slightly more difficult to parse this (especially if we
allow 'fmt' to be an expression yielding a string, not just a string
literal), but I don't think it is ambiguous and can be sorted out via
yylex().
I think it is a bad idea, if not actually impossible, to have an
expression for sqlstate with no separating syntax before the 'fmt';
especially not if you'd like to also allow an expression for the 'fmt'.
At one point we had talked about
RAISE LEVEL [ opt_sqlstate, ] 'fmt' [, expr ... ]
The hard part here is that there isn't any very easy way to tell whether
you have a sqlstate, a fmt, and N exprs, or a fmt and N+1 exprs. The
saving grace of the declared-exception approach for this is that you
can tell by the datatype of the first argument expression which case you
have: if the expression yields text, it's a fmt, if it yields exception
(which we assume is an actual datatype) then it's a sqlstate.
We could handle undeclared exceptions in such a design by having a
function that converts text to an exception value:
RAISE LEVEL SQLSTATE('12345'), 'format here', ...
and maybe the short-term cheesy thing to do is special-case exactly this
syntax:
RAISE LEVEL [ SQLSTATE(text_expr), ] text_expr [, ... ]
which would give us the minimum functionality with a clear path to
expansion later.
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
[PATCHES] Bad link Makefile patch
I built the v8.0.3 product from postgresql-8.0.3-1PGDG.src.rpm on RedHat9 (I'm thinking the source RPM for RH9 should not have exactly the same name as the FC3 version, since they are different files). When I tried to roll it into an RPM CD builder transaction, I got 'RPM dependency errors': CRITICAL ERROR: Unable to resolve dependency libecpg.so.3 for postgresql-libs and CRITICAL ERROR: Unable to resolve dependency libpq.so.3 for postgresql-libs. I was only including the base rpm, -server, and -lib in the RPM transaction set. The culprit was a nasty bit at $TOP/src/Makefile.shlib:243. This piece insures that for the link step of libecpg.so.5.0 and libecpg_compat.so.2.0, /usr/lib is searched for libpq and libecpg.so _before_ the locally built copy is searched. At least for libecpg.so.5.0 and libecpg_compat.so.2.0, the attached patch fixes the problem. Not sure if there would be other instances in -contrib, etc. Hope this helps and that I'm not redundant with your other fans. Regards, Mark userlib_link.patch Description: Binary data ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] User's exception plpgsql
Tom Lane wrote: I think it is a bad idea, if not actually impossible, to have an expression for sqlstate with no separating syntax before the 'fmt'; especially not if you'd like to also allow an expression for the 'fmt'. I don't actually see much of a need to allow 'fmt' to be an expression, especially now that RAISE parameters can be expressions. The ratio of constant printf() format strings to variable format strings is probably 100:1, for good reason... The hard part here is that there isn't any very easy way to tell whether you have a sqlstate, a fmt, and N exprs, or a fmt and N+1 exprs. The saving grace of the declared-exception approach for this is that you can tell by the datatype of the first argument expression which case you have I really don't like the idea of introducing a new concept into the language (exception variables) to resolve some ambiguous syntax. It would be another matter if exception variables actually provided something that strings do not... Another solution might be varying the syntax slightly, such as: RAISE [ opt_sqlstate ] LEVEL 'fmt' [ , expr ... ] -Neil ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] User's exception plpgsql
exception variable in the first place. What does it buy us over simply using a string? In other words, if we allowed the syntax: RAISE LEVEL [ opt_sqlstate ] 'fmt' [, expr ... ] where `opt_sqlstate' is either empty, a T_WORD we find in the table of predefined condition names, or an expression that evaluates to a text value. The text value must be of a certain form (e.g. 5 characters in length, begins with a U and so on). I unlike this syntax. Yes, it's easy and clear, but not readable. Exception variables are better and an way for future. SQL state can be only one value wich can hold exception variable. And more it's more in oracle style (I don't wont to copy all Oracle ware into PostgreSQL) Pavel p.s. I have patch for rethrow exception which isn't related to user's exception (but need's finished plpgsql code). Syntax is easy, I hope RAISE; ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] User's exception plpgsql
Neil Conway [EMAIL PROTECTED] writes: Tom Lane wrote: I think it is a bad idea, if not actually impossible, to have an expression for sqlstate with no separating syntax before the 'fmt'; especially not if you'd like to also allow an expression for the 'fmt'. I don't actually see much of a need to allow 'fmt' to be an expression, Well, in any case we have a problem if there's no comma. Consider RAISE NOTICE '12' !! '345', ... Is !! an infix operator (using both strings as arguments) or a postfix operator (in which case '345' is the format)? Another solution might be varying the syntax slightly, such as: RAISE [ opt_sqlstate ] LEVEL 'fmt' [ , expr ... ] This would require promoting all the options for LEVEL into fully reserved words. You really can't get around the fact that you need something pretty identifiable to terminate the expression. It might work to require parentheses: RAISE LEVEL ( sqlstate expression ), 'fmt' [, ...] The comma after the right paren is optional from a formal point of view, but I'd still consider it better design to use one than not. (For one reason, it would make it much easier to catch mismatched-parens problems.) regards, tom lane ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send unregister YourEmailAddressHere to [EMAIL PROTECTED])
Re: [PATCHES] User's exception plpgsql
This would require promoting all the options for LEVEL into fully
reserved words. You really can't get around the fact that you need
something pretty identifiable to terminate the expression.
It might work to require parentheses:
RAISE LEVEL ( sqlstate expression ), 'fmt' [, ...]
? what sense has sqlstate expression? like any expression returns sqlstate
type?
SQLSTATE('')|user exception|system exception
Pavel
---(end of broadcast)---
TIP 2: you can get off all lists at once with the unregister command
(send unregister YourEmailAddressHere to [EMAIL PROTECTED])
Re: [PATCHES] User's exception plpgsql
Pavel Stehule [EMAIL PROTECTED] writes: if I use registered sqlstate, plpgsql knows text message. No, it does not. I already pointed out that tying a single error message to a SQLSTATE is unreasonable, because that's not how the SQL committee intended SQLSTATEs to be used. I haven't looked at this patch yet, but if it's doing things that way it is wrong. 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
Re: [PATCHES] User's exception plpgsql
On Wed, 6 Jul 2005, Tom Lane wrote: Pavel Stehule [EMAIL PROTECTED] writes: if I use registered sqlstate, plpgsql knows text message. No, it does not. I already pointed out that tying a single error message to a SQLSTATE is unreasonable, because that's not how the SQL committee intended SQLSTATEs to be used. I haven't looked at this patch yet, but if it's doing things that way it is wrong. no, raise stmt still needs message text (patch) regards, tom lane What I wont. Maybe I going in wrong direction. Please, correct me. User's exception needs and will needs message text. I don't wont to introduce wrong programming style. But if I use exception variable and have to use its, then there is not only SQLSTATE but there exist name of exception too. But I wont to simplify using system's exception. The system knows all what need: name, text, sqlstate. And in mostly time I don't wont to substitute text of system message. But if I wont to show it, I have to copy it. example: raise exception div_by_zero; -- I wont to use system message, why not? but now, I have to do raise exception div_by_zero, 'division by zero ...' Regards Pavel ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] User's exception plpgsql
I really don't like the idea of introducing a new concept into the
language (exception variables) to resolve some ambiguous syntax. It
would be another matter if exception variables actually provided
something that strings do not...
In this time e.variables does it - only holds sqlstate and name.
You see only raise stmt. But there is part of begin exception block too.
without e.v. you have to catch users exception only via OTHERS or you have
to change syntax.
EXCEPTION WHEN SQLSTATE('') THEN
e.v. solve this problem. And I hope so can hold others info in future
Pavel
---(end of broadcast)---
TIP 2: you can get off all lists at once with the unregister command
(send unregister YourEmailAddressHere to [EMAIL PROTECTED])
[PATCHES] More to Bad link Makefile patch
So, my co-workers chided me mercilessly to get the -contrib RPM working as well; so the full patch is now attached. BTW, I did search the archive and this problem did not stick out; but it could be my crummy reference skills. And, no, I didn't read all 3500 emails since the v8.0.3 release. Also, there's some good chance that the binary RPMs on the postgresql.org FTP site have the same flaw that I encountered as described below. Regards, Mark On Wed, 6 Jul 2005 09:08:50 -0700 Mark Deric [EMAIL PROTECTED] wrote: I built the v8.0.3 product from postgresql-8.0.3-1PGDG.src.rpm on RedHat9 (I'm thinking the source RPM for RH9 should not have exactly the same name as the FC3 version, since they are different files). When I tried to roll it into an RPM CD builder transaction, I got 'RPM dependency errors': CRITICAL ERROR: Unable to resolve dependency libecpg.so.3 for postgresql-libs and CRITICAL ERROR: Unable to resolve dependency libpq.so.3 for postgresql-libs. I was only including the base rpm, -server, and -lib in the RPM transaction set. The culprit was a nasty bit at $TOP/src/Makefile.shlib:243. This piece insures that for the link step of libecpg.so.5.0 and libecpg_compat.so.2.0, /usr/lib is searched for libpq and libecpg.so _before_ the locally built copy is searched. At least for libecpg.so.5.0 and libecpg_compat.so.2.0, the attached patch fixes the problem. Not sure if there would be other instances in-contrib, etc. Hope this helps and that I'm not redundant with your other fans. Regards, Mark userlib_link.patch Description: Binary data ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] TODO Item - Return compressed length of TOAST datatypes
Mark Kirkwood wrote:
I did a few cleanups on the last patch. Please examine this one instead.
The changes are:
1. Add documentation for pg_datum_length builtin.
2. Correct some typos in the code comments.
3. Move the code in toastfuncs.c to varlena.c as it is probably the
correct place.
4. Use ereport instead of elog.
5 Quiet compiler warning in pg_datum_length.
I have modified your patch to simplify the logic, and renamed it to
pg_column_size(), to be consistent with our soon-to-be-added
pg_relation/tablespace/database functions from dbsize.
Here is a sample usage:
test= CREATE TABLE test (x INT, y TEXT);
CREATE TABLE
test= INSERT INTO test VALUES (4, repeat('x', 1));
INSERT 0 1
test= INSERT INTO test VALUES (4, repeat('x', 10));
INSERT 0 1
test= SELECT pg_column_size(x), pg_column_size(y) FROM test;
pg_column_size | pg_column_size
+
4 |121
4 | 1152
(2 rows)
Interesting the 10-times larger column is 10-times larger in storage.
Do we have some limit on how many repeated values we can record?
Patch attached and applied.
--
Bruce Momjian| http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup.| Newtown Square, Pennsylvania 19073
Index: doc/src/sgml/func.sgml
===
RCS file: /cvsroot/pgsql/doc/src/sgml/func.sgml,v
retrieving revision 1.262
diff -c -c -r1.262 func.sgml
*** doc/src/sgml/func.sgml 29 Jun 2005 01:52:56 - 1.262
--- doc/src/sgml/func.sgml 6 Jul 2005 18:55:34 -
***
*** 2187,2192
--- 2187,2200
/row
row
+
entryliteralfunctionpg_column_size/function(parameterstring/parameter)/literal/entry
+entrytypeinteger/type/entry
+entryNumber of bytes required to store the value, which might be
compressed/entry
+entryliteralpg_column_size('jo\\000se'::bytea)/literal/entry
+entryliteral5/literal/entry
+ /row
+
+ row
entryliteralfunctionposition/function(parametersubstring/parameter
in parameterstring/parameter)/literal/entry
entrytypeinteger/type/entry
entryLocation of specified substring/entry
Index: src/backend/access/heap/tuptoaster.c
===
RCS file: /cvsroot/pgsql/src/backend/access/heap/tuptoaster.c,v
retrieving revision 1.49
diff -c -c -r1.49 tuptoaster.c
*** src/backend/access/heap/tuptoaster.c21 Mar 2005 01:23:58 -
1.49
--- src/backend/access/heap/tuptoaster.c6 Jul 2005 18:55:35 -
***
*** 1436,1438
--- 1436,1480
return result;
}
+
+ /* --
+ * toast_datum_size
+ *
+ *Show the (possibly compressed) size of a datum
+ * --
+ */
+ Size
+ toast_datum_size(Datum value)
+ {
+
+ varattrib *attr = (varattrib *) DatumGetPointer(value);
+ Sizeresult;
+
+ if (VARATT_IS_EXTERNAL(attr))
+ {
+ /*
+* Attribute is stored externally - If it is compressed too,
+* then we need to get the external datum and calculate its
size,
+* otherwise we just use the external rawsize.
+*/
+ if (VARATT_IS_COMPRESSED(attr))
+ {
+ varattrib *attrext =
toast_fetch_datum(attr);
+ result = VARSIZE(attrext);
+ pfree(attrext);
+ }
+ else
+ result = attr-va_content.va_external.va_rawsize;
+ }
+ else
+ {
+ /*
+* Attribute is stored inline either compressed or not, just
+* calculate the size of the datum in either case.
+*/
+ result = VARSIZE(attr);
+ }
+
+ return result;
+
+ }
Index: src/backend/utils/adt/varlena.c
===
RCS file: /cvsroot/pgsql/src/backend/utils/adt/varlena.c,v
retrieving revision 1.124
diff -c -c -r1.124 varlena.c
*** src/backend/utils/adt/varlena.c 4 Jul 2005 18:56:44 - 1.124
--- src/backend/utils/adt/varlena.c 6 Jul 2005 18:55:36 -
***
*** 28,33
--- 28,34
#include utils/builtins.h
#include utils/lsyscache.h
#include utils/pg_locale.h
+ #include utils/syscache.h
typedef struct varlena unknown;
***
*** 2348,2350
--- 2349,2395
result_text = PG_STR_GET_TEXT(hexsum);
PG_RETURN_TEXT_P(result_text);
}
+
+ /*
+ * Return the length of a datum, possibly compressed
+
Re: [PATCHES] More to Bad link Makefile patch
Mark Deric [EMAIL PROTECTED] writes: So, my co-workers chided me mercilessly to get the -contrib RPM working as well; so the full patch is now attached. I think this overlaps or may be redundant with the patch that Peter E. recently proposed; have you been following the thread about buildfarm failures? http://archives.postgresql.org/pgsql-hackers/2005-07/msg00178.php regards, tom lane ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] User's exception plpgsql
Tom Lane wrote: RAISE NOTICE '12' !! '345', ... Is !! an infix operator (using both strings as arguments) or a postfix operator (in which case '345' is the format)? Ah, I see. I would be content to allow opt_sqlstate to be either a string literal, a T_WORD (predefined error condition), or a TEXT variable. If users need to throw a sqlstate that is derived from a SQL expression, they can always assign to a TEXT variable and then specify that variable to RAISE. RAISE [ opt_sqlstate ] LEVEL 'fmt' [ , expr ... ] This syntax might be slightly better anyway, as allowing two string literals without any intervening tokens is a bit ugly. We would still need to restrict opt_sqlstate as described above, though. -Neil ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
[PATCHES] Mistake in latest plperl patch
Per Andrew (he is having email problems): http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=viperdt=2005-07-07%2001:10:01 Joshua D. Drkae ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] TODO Item - Return compressed length of TOAST datatypes
Bruce Momjian wrote:
+ /*
+ * Return the length of a datum, possibly compressed
+ */
+ Datum
+ pg_column_size(PG_FUNCTION_ARGS)
+ {
+ Datum value = PG_GETARG_DATUM(0);
+ int result;
+
+ /* fn_extra stores the fixed column length, or -1 for varlena. */
+ if (fcinfo-flinfo-fn_extra == NULL) /* first call? */
+ {
+ /* On the first call lookup the datatype of the supplied
argument */
[...]
Is this optimization worth the code complexity?
+ tp = SearchSysCache(TYPEOID,
+
ObjectIdGetDatum(argtypeid),
+ 0, 0, 0);
+ if (!HeapTupleIsValid(tp))
+ {
+ /* Oid not in pg_type, should never happen. */
+ ereport(ERROR,
+ (errcode(ERRCODE_INTERNAL_ERROR),
+errmsg(invalid typid: %u,
argtypeid)));
+ }
elog(ERROR) is usually used for can't happen errors; also, the usual
error message in this scenario is cache lookup failed [...]. Perhaps
better to use get_typlen() here, anyway.
-Neil
---(end of broadcast)---
TIP 6: Have you searched our list archives?
http://archives.postgresql.org
Re: [PATCHES] TODO Item - Return compressed length of TOAST datatypes
Neil Conway wrote:
Bruce Momjian wrote:
+ /* + * Return the length of a datum, possibly compressed
+ */
+ Datum
+ pg_column_size(PG_FUNCTION_ARGS)
+ {
+ Datumvalue = PG_GETARG_DATUM(0);
+ intresult;
+ + /*fn_extra stores the fixed column length, or -1 for
varlena. */
+ if (fcinfo-flinfo-fn_extra == NULL)/* first call? */
+ {
+ /* On the first call lookup the datatype of the supplied
argument */
[...]
Is this optimization worth the code complexity?
I didn't performance test it, but the idea of hammering the catalogs for
each value to be processed seemed a bad thing.
+ tp = SearchSysCache(TYPEOID,
+ ObjectIdGetDatum(argtypeid),
+ 0, 0, 0);
+ if (!HeapTupleIsValid(tp))
+ {
+ /* Oid not in pg_type, should never happen. */
+ ereport(ERROR,
+ (errcode(ERRCODE_INTERNAL_ERROR),
+ errmsg(invalid typid: %u, argtypeid)));
+ }
elog(ERROR) is usually used for can't happen errors; also, the usual
error message in this scenario is cache lookup failed [...]. Perhaps
better to use get_typlen() here, anyway.
Ahem,..ah yes, get_typlen()! (ducks)- that is clearly much better!
I have attached a little change to varlena.c that uses it. I left the
ereport as it was, but am not fussed about it either way.
BTW - Bruce, I like the changes, makes the beast more friendly to use!
Best wishes
Mark
Index: src/backend/utils/adt/varlena.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/utils/adt/varlena.c,v
retrieving revision 1.125
diff -c -r1.125 varlena.c
*** src/backend/utils/adt/varlena.c 6 Jul 2005 19:02:52 - 1.125
--- src/backend/utils/adt/varlena.c 7 Jul 2005 02:52:50 -
***
*** 28,34
#include utils/builtins.h
#include utils/lsyscache.h
#include utils/pg_locale.h
- #include utils/syscache.h
typedef struct varlena unknown;
--- 28,33
***
*** 2364,2385
{
/* On the first call lookup the datatype of the supplied
argument */
Oid argtypeid =
get_fn_expr_argtype(fcinfo-flinfo, 0);
! HeapTuple tp;
! int typlen;
! tp = SearchSysCache(TYPEOID,
!
ObjectIdGetDatum(argtypeid),
! 0, 0, 0);
! if (!HeapTupleIsValid(tp))
{
/* Oid not in pg_type, should never happen. */
ereport(ERROR,
(errcode(ERRCODE_INTERNAL_ERROR),
!errmsg(invalid typid: %u,
argtypeid)));
}
!
! typlen = ((Form_pg_type)GETSTRUCT(tp))-typlen;
! ReleaseSysCache(tp);
fcinfo-flinfo-fn_extra =
MemoryContextAlloc(fcinfo-flinfo-fn_mcxt,
sizeof(int));
*(int *)fcinfo-flinfo-fn_extra = typlen;
--- 2363,2379
{
/* On the first call lookup the datatype of the supplied
argument */
Oid argtypeid =
get_fn_expr_argtype(fcinfo-flinfo, 0);
! int typlen=
get_typlen(argtypeid);
!
! if (typlen == 0)
{
/* Oid not in pg_type, should never happen. */
ereport(ERROR,
(errcode(ERRCODE_INTERNAL_ERROR),
!errmsg(cache lookup failed for type
%u, argtypeid)));
}
!
fcinfo-flinfo-fn_extra =
MemoryContextAlloc(fcinfo-flinfo-fn_mcxt,
sizeof(int));
*(int *)fcinfo-flinfo-fn_extra = typlen;
---(end of broadcast)---
TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] TODO Item - Return compressed length of TOAST datatypes
Mark Kirkwood wrote: I didn't performance test it, but the idea of hammering the catalogs for each value to be processed seemed a bad thing. Well, the syscache already sits in front of the catalogs themselves. I'd be curious to see what the performance difference actually is... -Neil ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] TODO Item - Return compressed length of TOAST datatypes
On Thu, Jul 07, 2005 at 03:01:46PM +1200, Mark Kirkwood wrote: Neil Conway wrote: elog(ERROR) is usually used for can't happen errors; also, the usual error message in this scenario is cache lookup failed [...]. Perhaps better to use get_typlen() here, anyway. I have attached a little change to varlena.c that uses it. I left the ereport as it was, but am not fussed about it either way. I am, because it gives useless messages to the translators to work on. elog parameters are not marked for translation, ereport are (errmsg and friends, really). So please don't do that. -- Alvaro Herrera (alvherre[a]alvh.no-ip.org) ¿Cómo puedes confiar en algo que pagas y que no ves, y no confiar en algo que te dan y te lo muestran? (Germán Poo) ---(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
Re: [PATCHES] TODO Item - Return compressed length of TOAST datatypes
Alvaro Herrera wrote:
On Thu, Jul 07, 2005 at 03:01:46PM +1200, Mark Kirkwood wrote:
Neil Conway wrote:
elog(ERROR) is usually used for can't happen errors.
I have attached a little change to varlena.c that uses it. I left the
ereport as it was, but am not fussed about it either way.
I am, because it gives useless messages to the translators to work on.
elog parameters are not marked for translation, ereport are (errmsg and
friends, really). So please don't do that.
Ok, didn't realize the difference! Revised patch attached that uses elog.
Neil, I will runs some tests to see if there is any performance saving
with the first-call-only business.
Cheers
Mark
Index: src/backend/utils/adt/varlena.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/utils/adt/varlena.c,v
retrieving revision 1.125
diff -c -r1.125 varlena.c
*** src/backend/utils/adt/varlena.c 6 Jul 2005 19:02:52 - 1.125
--- src/backend/utils/adt/varlena.c 7 Jul 2005 03:40:44 -
***
*** 28,34
#include utils/builtins.h
#include utils/lsyscache.h
#include utils/pg_locale.h
- #include utils/syscache.h
typedef struct varlena unknown;
--- 28,33
***
*** 2364,2385
{
/* On the first call lookup the datatype of the supplied
argument */
Oid argtypeid =
get_fn_expr_argtype(fcinfo-flinfo, 0);
! HeapTuple tp;
! int typlen;
! tp = SearchSysCache(TYPEOID,
!
ObjectIdGetDatum(argtypeid),
! 0, 0, 0);
! if (!HeapTupleIsValid(tp))
{
/* Oid not in pg_type, should never happen. */
! ereport(ERROR,
! (errcode(ERRCODE_INTERNAL_ERROR),
!errmsg(invalid typid: %u,
argtypeid)));
}
!
! typlen = ((Form_pg_type)GETSTRUCT(tp))-typlen;
! ReleaseSysCache(tp);
fcinfo-flinfo-fn_extra =
MemoryContextAlloc(fcinfo-flinfo-fn_mcxt,
sizeof(int));
*(int *)fcinfo-flinfo-fn_extra = typlen;
--- 2363,2377
{
/* On the first call lookup the datatype of the supplied
argument */
Oid argtypeid =
get_fn_expr_argtype(fcinfo-flinfo, 0);
! int typlen=
get_typlen(argtypeid);
!
! if (typlen == 0)
{
/* Oid not in pg_type, should never happen. */
! elog(ERROR, cache lookup failed for type %u,
argtypeid);
}
!
fcinfo-flinfo-fn_extra =
MemoryContextAlloc(fcinfo-flinfo-fn_mcxt,
sizeof(int));
*(int *)fcinfo-flinfo-fn_extra = typlen;
---(end of broadcast)---
TIP 5: Have you checked our extensive FAQ?
http://www.postgresql.org/docs/faq
Re: [PATCHES] TODO Item - Return compressed length of TOAST datatypes
Alvaro Herrera [EMAIL PROTECTED] writes: On Thu, Jul 07, 2005 at 03:01:46PM +1200, Mark Kirkwood wrote: I have attached a little change to varlena.c that uses it. I left the ereport as it was, but am not fussed about it either way. I am, because it gives useless messages to the translators to work on. Exactly. I had already made a private note to fix that patch --- inventing your own random wording and punctuation for an extremely standard error message is simply Not Acceptable. regards, tom lane ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send unregister YourEmailAddressHere to [EMAIL PROTECTED])
Re: [PATCHES] TODO Item - Return compressed length of TOAST datatypes
Neil Conway wrote: Mark Kirkwood wrote: I didn't performance test it, but the idea of hammering the catalogs for each value to be processed seemed a bad thing. Well, the syscache already sits in front of the catalogs themselves. I'd be curious to see what the performance difference actually is... I did some tests with a two tables, one small and one large, I am seeing a consistent difference favoring the first-call-only type checking: Table public.dim1 Column | Type | Modifiers ++--- d1key | integer| not null dat| character varying(100) | not null dattyp | character varying(20) | not null dfill | character varying(100) | not null INFO: dim1: scanned 24 of 24 pages, containing 1001 live rows and 0 dead rows; 1001 rows in sample, 1001 estimated total rows SELECT max(pg_column_size(dfill)) FROM dim1 Run 1st call only check? elapsed(ms) 1y5.5 2y3.1 3n11.3 4n4.1 Now try a bigger table: Table public.fact0 Column | Type | Modifiers ++--- d0key | integer| not null d1key | integer| not null d2key | integer| not null fval | integer| not null ffill | character varying(100) | not null INFO: fact0: scanned 3000 of 18182 pages, containing 165000 live rows and 0 dead rows; 3000 rows in sample, 110 estimated total rows SELECT max(pg_column_size(ffill)) FROM fact0 Run 1st call only check? elapsed(ms) 1y2497 2y2484 3y2496 4y2480 5n3572 6n3570 7n3558 8n3457 ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send unregister YourEmailAddressHere to [EMAIL PROTECTED])
Re: [PATCHES] TODO Item - Return compressed length of TOAST datatypes
Patch applied. Thanks.
---
Mark Kirkwood wrote:
Alvaro Herrera wrote:
On Thu, Jul 07, 2005 at 03:01:46PM +1200, Mark Kirkwood wrote:
Neil Conway wrote:
elog(ERROR) is usually used for can't happen errors.
I have attached a little change to varlena.c that uses it. I left the
ereport as it was, but am not fussed about it either way.
I am, because it gives useless messages to the translators to work on.
elog parameters are not marked for translation, ereport are (errmsg and
friends, really). So please don't do that.
Ok, didn't realize the difference! Revised patch attached that uses elog.
Neil, I will runs some tests to see if there is any performance saving
with the first-call-only business.
Cheers
Mark
Index: src/backend/utils/adt/varlena.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/utils/adt/varlena.c,v
retrieving revision 1.125
diff -c -r1.125 varlena.c
*** src/backend/utils/adt/varlena.c 6 Jul 2005 19:02:52 - 1.125
--- src/backend/utils/adt/varlena.c 7 Jul 2005 03:40:44 -
***
*** 28,34
#include utils/builtins.h
#include utils/lsyscache.h
#include utils/pg_locale.h
- #include utils/syscache.h
typedef struct varlena unknown;
--- 28,33
***
*** 2364,2385
{
/* On the first call lookup the datatype of the supplied
argument */
Oid argtypeid =
get_fn_expr_argtype(fcinfo-flinfo, 0);
! HeapTuple tp;
! int typlen;
! tp = SearchSysCache(TYPEOID,
!
ObjectIdGetDatum(argtypeid),
! 0, 0, 0);
! if (!HeapTupleIsValid(tp))
{
/* Oid not in pg_type, should never happen. */
! ereport(ERROR,
! (errcode(ERRCODE_INTERNAL_ERROR),
! errmsg(invalid typid: %u,
argtypeid)));
}
!
! typlen = ((Form_pg_type)GETSTRUCT(tp))-typlen;
! ReleaseSysCache(tp);
fcinfo-flinfo-fn_extra =
MemoryContextAlloc(fcinfo-flinfo-fn_mcxt,
sizeof(int));
*(int *)fcinfo-flinfo-fn_extra = typlen;
--- 2363,2377
{
/* On the first call lookup the datatype of the supplied
argument */
Oid argtypeid =
get_fn_expr_argtype(fcinfo-flinfo, 0);
! int typlen=
get_typlen(argtypeid);
!
! if (typlen == 0)
{
/* Oid not in pg_type, should never happen. */
! elog(ERROR, cache lookup failed for type %u,
argtypeid);
}
!
fcinfo-flinfo-fn_extra =
MemoryContextAlloc(fcinfo-flinfo-fn_mcxt,
sizeof(int));
*(int *)fcinfo-flinfo-fn_extra = typlen;
--
Bruce Momjian| http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup.| Newtown Square, Pennsylvania 19073
---(end of broadcast)---
TIP 8: explain analyze is your friend
Re: [PATCHES] TODO Item - Return compressed length of TOAST datatypes
Tom Lane wrote: Alvaro Herrera [EMAIL PROTECTED] writes: On Thu, Jul 07, 2005 at 03:01:46PM +1200, Mark Kirkwood wrote: I have attached a little change to varlena.c that uses it. I left the ereport as it was, but am not fussed about it either way. I am, because it gives useless messages to the translators to work on. Exactly. I had already made a private note to fix that patch --- inventing your own random wording and punctuation for an extremely standard error message is simply Not Acceptable. Apologies all, my not fussed about it was highly misleading - I had already changed the error message as per Neil's suggestion in the revised patch, but that was not obvious from my comment :-( regards Mark ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] [HACKERS] Mistake in latest plperl patch
Joshua D. Drake wrote: Per Andrew (he is having email problems): http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=viperdt=2005-07-07%2001:10:01 Thanks, fixed (I hope). -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
