Re: [PATCHES] [HACKERS] Cannot dump/restore text value \N
On Sun, 05 Oct 2003 19:12:50 -0400, Tom Lane [EMAIL PROTECTED]
wrote:
it seems we have to compare the null representation string to the
pre-debackslashing input.
Here is a patch that does this and adds a few regression tests.
(This is probably fairly easy to make happen
in CVS tip, but it might be pretty painful in 7.3.)
There haven't been too much changes in this area between 7.3 and 7.4.
A patch against 7.3.4 will follow ...
Servus
Manfred
diff -ruN ../base/src/backend/commands/copy.c src/backend/commands/copy.c
--- ../base/src/backend/commands/copy.c 2003-08-28 15:52:34.0 +0200
+++ src/backend/commands/copy.c 2003-10-08 10:43:02.0 +0200
@@ -90,7 +90,8 @@
char *delim, char *null_print);
static void CopyFrom(Relation rel, List *attnumlist, bool binary, bool oids,
char *delim, char *null_print);
-static char *CopyReadAttribute(const char *delim, CopyReadResult *result);
+static char *CopyReadAttribute(const char *delim, const char *nullst,
+ CopyReadResult *result, bool *isnull);
static Datum CopyReadBinaryAttribute(int column_no, FmgrInfo *flinfo,
Oid typelem, bool *isnull);
static void CopyAttributeOut(char *string, char *delim);
@@ -1361,7 +1362,7 @@
if (file_has_oids)
{
- string = CopyReadAttribute(delim, result);
+ string = CopyReadAttribute(delim, null_print, result,
isnull);
if (result == END_OF_FILE *string == '\0')
{
@@ -1370,7 +1371,7 @@
break;
}
- if (strcmp(string, null_print) == 0)
+ if (isnull)
ereport(ERROR,
(errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
errmsg(null OID in COPY
data)));
@@ -1403,7 +1404,7 @@
errmsg(missing data for
column \%s\,
NameStr(attr[m]-attname;
- string = CopyReadAttribute(delim, result);
+ string = CopyReadAttribute(delim, null_print, result,
isnull);
if (result == END_OF_FILE *string == '\0'
cur == attnumlist !file_has_oids)
@@ -1413,7 +1414,7 @@
break; /* out of per-attr loop */
}
- if (strcmp(string, null_print) == 0)
+ if (isnull)
{
/* we read an SQL NULL, no need to do anything
*/
}
@@ -1442,7 +1443,7 @@
{
if (attnumlist == NIL !file_has_oids)
{
- string = CopyReadAttribute(delim, result);
+ string = CopyReadAttribute(delim, null_print,
result, isnull);
if (result == NORMAL_ATTR || *string != '\0')
ereport(ERROR,
(errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
@@ -1650,14 +1651,13 @@
* END_OF_FILE:EOF indicator
* In all cases, the string read up to the terminator is returned.
*
- * Note: This function does not care about SQL NULL values -- it
- * is the caller's responsibility to check if the returned string
- * matches what the user specified for the SQL NULL value.
- *
* delim is the column delimiter string.
+ * nullst says how NULL values are represented.
+ * *isnull is set true if a null attribute, else false.
*/
static char *
-CopyReadAttribute(const char *delim, CopyReadResult *result)
+CopyReadAttribute(const char *delim, const char *nullst,
+ CopyReadResult *result, bool *isnull)
{
int c;
int delimc = (unsigned char) delim[0];
@@ -1665,6 +1665,17 @@
unsigned char s[2];
char *cvt;
int j;
+ boolmatchnull = true;
+ int matchlen = 0;
+
+#define CHECK_MATCH(c) \
+ do { \
+ if (matchnull) \
+ if (c == nullst[matchlen]) \
+ ++matchlen; \
+ else \
+ matchnull = false; \
+ } while (0)
s[1] = 0;
@@ -1733,6 +1744,7 @@
}
Re: [PATCHES] [HACKERS] Cannot dump/restore text value \N
On Wed, 08 Oct 2003 11:31:30 +0200, I wrote:
There haven't been too much changes in this area between 7.3 and 7.4.
Here is the patch for 7.3.4 ...
Bruce, I noticed that the original patch submission didn't contain
anything useful as a cvs log message:
Make COPY FROM a bit more compatible with COPY TO regarding
backslashes, especially \N.
Servus
Manfred
diff -ruN ../base/src/backend/commands/copy.c src/backend/commands/copy.c
--- ../base/src/backend/commands/copy.c 2003-04-26 00:14:33.0 +0200
+++ src/backend/commands/copy.c 2003-10-08 16:30:14.0 +0200
@@ -62,7 +62,8 @@
FILE *fp, char *delim, char *null_print);
static Oid GetInputFunction(Oid type);
static Oid GetTypeElement(Oid type);
-static char *CopyReadAttribute(FILE *fp, const char *delim, CopyReadResult *result);
+static char *CopyReadAttribute(FILE *fp, const char *delim, const char *nullst,
+ CopyReadResult *result, bool *isnull);
static void CopyAttributeOut(FILE *fp, char *string, char *delim);
static List *CopyGetAttnums(Relation rel, List *attnamelist);
@@ -931,6 +932,7 @@
{
boolskip_tuple;
Oid loaded_oid = InvalidOid;
+ boolisnull;
CHECK_FOR_INTERRUPTS();
@@ -954,7 +956,7 @@
if (file_has_oids)
{
- string = CopyReadAttribute(fp, delim, result);
+ string = CopyReadAttribute(fp, delim, null_print,
result, isnull);
if (result == END_OF_FILE *string == '\0')
{
@@ -963,7 +965,7 @@
break;
}
- if (strcmp(string, null_print) == 0)
+ if (isnull)
elog(ERROR, NULL Oid);
else
{
@@ -990,7 +992,7 @@
elog(ERROR, Missing data for column \%s\,
NameStr(attr[m]-attname));
- string = CopyReadAttribute(fp, delim, result);
+ string = CopyReadAttribute(fp, delim, null_print,
result, isnull);
if (result == END_OF_FILE *string == '\0'
cur == attnumlist !file_has_oids)
@@ -1000,7 +1002,7 @@
break; /* out of per-attr loop */
}
- if (strcmp(string, null_print) == 0)
+ if (isnull)
{
/* we read an SQL NULL, no need to do anything
*/
}
@@ -1029,7 +1031,7 @@
{
if (attnumlist == NIL !file_has_oids)
{
- string = CopyReadAttribute(fp, delim, result);
+ string = CopyReadAttribute(fp, delim,
null_print, result, isnull);
if (result == NORMAL_ATTR || *string != '\0')
elog(ERROR, Extra data after last
expected column);
if (result == END_OF_FILE)
@@ -1158,8 +1160,6 @@
*/
for (i = 0; i num_defaults; i++)
{
- boolisnull;
-
values[defmap[i]] = ExecEvalExpr(defexprs[i], econtext,
isnull, NULL);
if (!isnull)
@@ -1175,7 +1175,6 @@
{
Node *node = constraintexprs[i];
Const *con;
- boolisnull;
if (node == NULL)
continue; /* no constraint for this attr
*/
@@ -1316,15 +1315,14 @@
* END_OF_FILE:EOF indication
* In all cases, the string read up to the terminator is returned.
*
- * Note: This function does not care about SQL NULL values -- it
- * is the caller's responsibility to check if the returned string
- * matches what the user specified for the SQL NULL value.
- *
* delim is the column delimiter string.
+ * nullst says how NULL values are represented.
+ * *isnull is set true if a null attribute, else false.
*/
static char *
-CopyReadAttribute(FILE *fp, const char *delim, CopyReadResult *result)
+CopyReadAttribute(FILE *fp, const char *delim, const char *nullst,
+
Re: [PATCHES] [HACKERS] Cannot dump/restore text value \N
Manfred Koizar wrote: On Wed, 08 Oct 2003 11:31:30 +0200, I wrote: There haven't been too much changes in this area between 7.3 and 7.4. Here is the patch for 7.3.4 ... Bruce, I noticed that the original patch submission didn't contain anything useful as a cvs log message: Make COPY FROM a bit more compatible with COPY TO regarding backslashes, especially \N. Oh, good point. Thanks. Can someone explain what was broken? Was it only for non-standard NULL strings? Would it silently fail? I saw your example and it seemed strange we had not seen a bug report before. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (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 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
[PATCHES] Spanish translations of pg_dump and pg_resetxlog
pg_dump-es.pot.bz2 Description: Spanish translation of pg_dump pg_resetxlog-es.pot.bz2 Description: Spanish translation of pg_resetxlog Regards, Manuel. ---(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] [HACKERS] pg_restore -d doesn't display output
Bruce Momjian writes: I have patched pg_restore to output the script contents if you restore with -v: I don't think it's a good idea to overload the -v switch with it. A separate switch seems OK, but I don't quite get the point of this functionality, actually. -- Peter Eisentraut [EMAIL PROTECTED] ---(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
[PATCHES] akward wording in autovacuum README
Change some awkward wording in the pg_autovacuum README file. I really
only read this because of Niel :-)
Robert Treat
--
Build A Brighter Lamp :: Linux Apache {middleware} PostgreSQL
Index: README.pg_autovacuum
===
RCS file: /projects/cvsroot/pgsql-server/contrib/pg_autovacuum/README.pg_autovacuum,v
retrieving revision 1.3
diff -c -r1.3 README.pg_autovacuum
*** README.pg_autovacuum 13 Sep 2003 16:26:17 - 1.3
--- README.pg_autovacuum 8 Oct 2003 18:04:38 -
***
*** 35,41
pg_autovacuum requires that the statistics system be enabled and
reporting row level stats. The overhead of the stats system has been
! shown to be significant costly under certain workloads. For instance,
a tight loop of queries performing select 1 was found to run nearly
30% slower when stats were enabled. However, in practice, with more
realistic workloads, the stats system overhead is usually nominal.
--- 35,41
pg_autovacuum requires that the statistics system be enabled and
reporting row level stats. The overhead of the stats system has been
! shown to have a significant cost under certain workloads. For instance,
a tight loop of queries performing select 1 was found to run nearly
30% slower when stats were enabled. However, in practice, with more
realistic workloads, the stats system overhead is usually nominal.
---(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] akward wording in autovacuum README
Patch applied. Thanks.
---
Robert Treat wrote:
Change some awkward wording in the pg_autovacuum README file. I really
only read this because of Niel :-)
Robert Treat
--
Build A Brighter Lamp :: Linux Apache {middleware} PostgreSQL
[ Attachment, skipping... ]
---(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
--
Bruce Momjian| http://candle.pha.pa.us
[EMAIL PROTECTED] | (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 9: the planner will ignore your desire to choose an index scan if your
joining column's datatypes do not match
Re: [PATCHES] [HACKERS] Cannot dump/restore text value \N
On Wed, 8 Oct 2003 11:33:24 -0400 (EDT), Bruce Momjian
[EMAIL PROTECTED] wrote:
Can someone explain what was broken?
COPY FROM removed backslashes before comparing the input to the
external null representation. (It had a hard-wired special code path
that allowed \N to be recognized.) The text \N was (and still is)
correctly exported as \\N, but \\N was imported as NULL.
Was it only for non-standard NULL strings?
There were problems in both cases.
Standard NULL representation:
fred=# CREATE TABLE a (c1 text, c2 text);
CREATE TABLE
fred=# INSERT INTO a VALUES ('\\N', null);
INSERT 577147 1
fred=# SELECT * FROM a;
c1 | c2
+
\N |
(1 row)
fred=# COPY a TO stdout;
\\N \N
fred=# COPY a FROM stdin;
Enter data to be copied followed by a newline.
End with a backslash and a period on a line by itself.
\\N \N
\.
fred=# SELECT * FROM a;
c1 | c2
+
\N |
|
(2 rows)
User defined NULL string:
fred=# CREATE TABLE a (c1 text, c2 text);
CREATE TABLE
fred=# INSERT INTO a VALUES ('\\X', null);
INSERT 577140 1
fred=# SELECT * FROM a;
c1 | c2
+
\X |
(1 row)
fred=# COPY a TO stdout WITH NULL AS '\\X';
\\X \X
fred=# COPY a FROM stdin WITH NULL AS '\\X';
Enter data to be copied followed by a newline.
End with a backslash and a period on a line by itself.
\\X \X
\.
fred=# SELECT * FROM a;
c1 | c2
+
\X |
| X
(2 rows)
Would it silently fail?
It would silently insert wrong data, unless a constraint (NOT NULL)
prevented it.
I saw your example and it seemed
strange we had not seen a bug report before.
Because nobody was crazy enough to store \N in his database ...
Tom has already fixed this issue for cvs head. My 7.4 patch wouldn't
apply anyway (built it against Beta 3). You might want to apply the
7.3.4 version, though.
Should I send a new patch with only the regression tests?
Servus
Manfred
---(end of broadcast)---
TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] Spanish translations of pg_dump and pg_resetxlog
Installed. Next time, please run msgfmt -o /dev/null -c -v file.po, or use the equivalent command in your PO editor, to check your file for errors. -- Peter Eisentraut [EMAIL PROTECTED] ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] fix log_min_duration_statement logic error
Bruce Momjian writes: Imagine someone always having log_statement on and doing some sort of aggregate query counting, say like grep '^LOG: query:' | wc -l. Now that someone (or maybe the admin on the night shift, to make it more dramatic) also turns on log_min_statement_duration (or whatever it's spelled), say to find the slowest queries: grep '^LOG: duration:' | sort -xyz | head -10. In order to guarantee the consistency of both results, you'd have to log each query twice in this case. I guess. Can we agree to this? * If log_statement is on, then we log query: %s (Should it be statement: %s?) %s has newlines suitable escaped; exactly how is independent of this discussion. * If log_duration is on, then we log duration: x.xxx ms The user can use log_pid to link it to the statement. (If the user doesn't like this, he can use log_min_duration_statement=0.) * If log_min_duration_statement is triggered, then we log duration: x.xxx ms; query: %s * If log_statement is on and log_min_duration_statement is triggered, then we (pick one): (a) log both as per above, or (b) log only log_min_duration_statement * If log_duration is on and log_min_duration_statement is triggered, then we (pick one): (a) log both as per above, or (b) log only log_min_duration_statement -- Peter Eisentraut [EMAIL PROTECTED] ---(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] fix log_min_duration_statement logic error
Peter Eisentraut wrote: Bruce Momjian writes: Imagine someone always having log_statement on and doing some sort of aggregate query counting, say like grep '^LOG: query:' | wc -l. Now that someone (or maybe the admin on the night shift, to make it more dramatic) also turns on log_min_statement_duration (or whatever it's spelled), say to find the slowest queries: grep '^LOG: duration:' | sort -xyz | head -10. In order to guarantee the consistency of both results, you'd have to log each query twice in this case. I guess. Can we agree to this? * If log_statement is on, then we log query: %s (Should it be statement: %s?) %s has newlines suitable escaped; exactly Yes, it should be 'statement'. how is independent of this discussion. People didn't like the newlines changed because it makes large queries hard to read (all on one line), and we had to double-escape backslashes, meaning the logged query had different backslashing from the original. * If log_duration is on, then we log duration: x.xxx ms The user can use log_pid to link it to the statement. (If the user doesn't like this, he can use log_min_duration_statement=0.) Exactly. log_min_duration_statement=0 delays the print of the statement until it completes, but it prints them together. * If log_min_duration_statement is triggered, then we log duration: x.xxx ms; query: %s * If log_statement is on and log_min_duration_statement is triggered, then we (pick one): (a) log both as per above, or I think we have to log both because perhaps they want the query printed before it completes. (b) log only log_min_duration_statement * If log_duration is on and log_min_duration_statement is triggered, then we (pick one): (a) log both as per above, or (b) log only log_min_duration_statement Good point. Right now we do only: duration: x.xxx ms; query: %s This seems wrong because it isn't consistent with log_statement. Glad you pointed that out. The only argument for printing one one is that there isn't any value to printing the duration separately, because it isn't printing before the query. However, for consistency, I think you are right we should print both. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (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 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [PATCHES] fix log_min_duration_statement logic error
pgman wrote: OK, patch attached and applied. Changes are: query: now statement: log_duration also now prints when log_min_duration_statement prints Oh, sample is: LOG: statement: select 1; LOG: duration: 0.329 ms LOG: duration: 0.329 ms statement: select 1; -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (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 6: Have you searched our list archives? http://archives.postgresql.org
