public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v13 1/2] Reusable decimalLength functions
13+ messages / 5 participants
[nested] [flat]
* [PATCH v13 1/2] Reusable decimalLength functions
@ 2023-02-17 09:17 Dmitrii Dolgov <[email protected]>
0 siblings, 0 replies; 13+ messages in thread
From: Dmitrii Dolgov @ 2023-02-17 09:17 UTC (permalink / raw)
Move out decimalLength functions to reuse in the following patch.
---
src/backend/utils/adt/numutils.c | 50 +-----------------------
src/include/utils/numutils.h | 67 ++++++++++++++++++++++++++++++++
2 files changed, 68 insertions(+), 49 deletions(-)
create mode 100644 src/include/utils/numutils.h
diff --git a/src/backend/utils/adt/numutils.c b/src/backend/utils/adt/numutils.c
index 471fbb7ee6..df7418cce7 100644
--- a/src/backend/utils/adt/numutils.c
+++ b/src/backend/utils/adt/numutils.c
@@ -18,9 +18,8 @@
#include <limits.h>
#include <ctype.h>
-#include "common/int.h"
#include "utils/builtins.h"
-#include "port/pg_bitutils.h"
+#include "utils/numutils.h"
/*
* A table of all two-digit numbers. This is used to speed up decimal digit
@@ -38,53 +37,6 @@ static const char DIGIT_TABLE[200] =
"80" "81" "82" "83" "84" "85" "86" "87" "88" "89"
"90" "91" "92" "93" "94" "95" "96" "97" "98" "99";
-/*
- * Adapted from http://graphics.stanford.edu/~seander/bithacks.html#IntegerLog10
- */
-static inline int
-decimalLength32(const uint32 v)
-{
- int t;
- static const uint32 PowersOfTen[] = {
- 1, 10, 100,
- 1000, 10000, 100000,
- 1000000, 10000000, 100000000,
- 1000000000
- };
-
- /*
- * Compute base-10 logarithm by dividing the base-2 logarithm by a
- * good-enough approximation of the base-2 logarithm of 10
- */
- t = (pg_leftmost_one_pos32(v) + 1) * 1233 / 4096;
- return t + (v >= PowersOfTen[t]);
-}
-
-static inline int
-decimalLength64(const uint64 v)
-{
- int t;
- static const uint64 PowersOfTen[] = {
- UINT64CONST(1), UINT64CONST(10),
- UINT64CONST(100), UINT64CONST(1000),
- UINT64CONST(10000), UINT64CONST(100000),
- UINT64CONST(1000000), UINT64CONST(10000000),
- UINT64CONST(100000000), UINT64CONST(1000000000),
- UINT64CONST(10000000000), UINT64CONST(100000000000),
- UINT64CONST(1000000000000), UINT64CONST(10000000000000),
- UINT64CONST(100000000000000), UINT64CONST(1000000000000000),
- UINT64CONST(10000000000000000), UINT64CONST(100000000000000000),
- UINT64CONST(1000000000000000000), UINT64CONST(10000000000000000000)
- };
-
- /*
- * Compute base-10 logarithm by dividing the base-2 logarithm by a
- * good-enough approximation of the base-2 logarithm of 10
- */
- t = (pg_leftmost_one_pos64(v) + 1) * 1233 / 4096;
- return t + (v >= PowersOfTen[t]);
-}
-
static const int8 hexlookup[128] = {
-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
diff --git a/src/include/utils/numutils.h b/src/include/utils/numutils.h
new file mode 100644
index 0000000000..876e64f2df
--- /dev/null
+++ b/src/include/utils/numutils.h
@@ -0,0 +1,67 @@
+/*-------------------------------------------------------------------------
+ *
+ * numutils.h
+ * Decimal length functions for numutils.c
+ *
+ *
+ * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/utils/numutils.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef NUMUTILS_H
+#define NUMUTILS_H
+
+#include "common/int.h"
+#include "port/pg_bitutils.h"
+
+/*
+ * Adapted from http://graphics.stanford.edu/~seander/bithacks.html#IntegerLog10
+ */
+static inline int
+decimalLength32(const uint32 v)
+{
+ int t;
+ static const uint32 PowersOfTen[] = {
+ 1, 10, 100,
+ 1000, 10000, 100000,
+ 1000000, 10000000, 100000000,
+ 1000000000
+ };
+
+ /*
+ * Compute base-10 logarithm by dividing the base-2 logarithm by a
+ * good-enough approximation of the base-2 logarithm of 10
+ */
+ t = (pg_leftmost_one_pos32(v) + 1) * 1233 / 4096;
+ return t + (v >= PowersOfTen[t]);
+}
+
+static inline int
+decimalLength64(const uint64 v)
+{
+ int t;
+ static const uint64 PowersOfTen[] = {
+ UINT64CONST(1), UINT64CONST(10),
+ UINT64CONST(100), UINT64CONST(1000),
+ UINT64CONST(10000), UINT64CONST(100000),
+ UINT64CONST(1000000), UINT64CONST(10000000),
+ UINT64CONST(100000000), UINT64CONST(1000000000),
+ UINT64CONST(10000000000), UINT64CONST(100000000000),
+ UINT64CONST(1000000000000), UINT64CONST(10000000000000),
+ UINT64CONST(100000000000000), UINT64CONST(1000000000000000),
+ UINT64CONST(10000000000000000), UINT64CONST(100000000000000000),
+ UINT64CONST(1000000000000000000), UINT64CONST(10000000000000000000)
+ };
+
+ /*
+ * Compute base-10 logarithm by dividing the base-2 logarithm by a
+ * good-enough approximation of the base-2 logarithm of 10
+ */
+ t = (pg_leftmost_one_pos64(v) + 1) * 1233 / 4096;
+ return t + (v >= PowersOfTen[t]);
+}
+
+#endif /* NUMUTILS_H */
--
2.32.0
--ydohaqx6er4mg5aj
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v13-0002-Prevent-jumbling-of-every-element-in-ArrayExpr.patch"
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [PATCH] hstore: Fix parsing on Mac OS X: isspace() is locale specific
@ 2023-06-18 08:32 Michael Paquier <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Michael Paquier @ 2023-06-18 08:32 UTC (permalink / raw)
To: Evan Jones <[email protected]>; +Cc: pgsql-hackers
On Sun, Jun 18, 2023 at 10:50:16AM +0900, Michael Paquier wrote:
> The difference between scanner_isspace() and array_isspace() is that
> the former matches with what scan.l stores as rules for whitespace
> characters, but the latter works on values. For hstore, we want the
> latter, with something that works on values. To keep the change
> locale to hstore, I think that we should just introduce an
> hstore_isspace() which is a copy of array_isspace. That's a
> duplication, sure, but I think that we may want to think harder about
> \v in the flex scanner, and that's just a few extra lines for
> something that has not changed in 13 years for arrays. That's also
> easier to think about for stable branches. If you can send a patch,
> that helps a lot, for sure!
At the end, no need to do that. I have been able to hack the
attached, that shows the difference of treatment for \v when running
in macOS. Evan, what do you think?
--
Michael
Attachments:
[text/x-diff] 0001-More-fixes-for-hstore-and-locales.patch (4.3K, ../../[email protected]/2-0001-More-fixes-for-hstore-and-locales.patch)
download | inline diff:
From 7ede07940011d08f8c0cf05d57b3782f367c0adf Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Sun, 18 Jun 2023 17:29:37 +0900
Subject: [PATCH] More fixes for hstore and locales
This is not really complete without the treatment of \v like array
values.
---
contrib/hstore/expected/hstore_utf8.out | 31 +++++++++++++++++++++++++
contrib/hstore/hstore_io.c | 30 ++++++++++++++++++++----
contrib/hstore/sql/hstore_utf8.sql | 7 ++++++
3 files changed, 63 insertions(+), 5 deletions(-)
diff --git a/contrib/hstore/expected/hstore_utf8.out b/contrib/hstore/expected/hstore_utf8.out
index 4405824413..bbc885a181 100644
--- a/contrib/hstore/expected/hstore_utf8.out
+++ b/contrib/hstore/expected/hstore_utf8.out
@@ -34,3 +34,34 @@ SELECT 'keyąfoo=>valueą'::hstore;
"keyąfoo"=>"valueą"
(1 row)
+-- More patterns that may depend on isspace() and locales, all discarded.
+SELECT E'key\u000A=>value\u000A'::hstore; -- \n
+ hstore
+----------------
+ "key"=>"value"
+(1 row)
+
+SELECT E'key\u0009=>value\u0009'::hstore; -- \t
+ hstore
+----------------
+ "key"=>"value"
+(1 row)
+
+SELECT E'key\u000D=>value\u000D'::hstore; -- \r
+ hstore
+----------------
+ "key"=>"value"
+(1 row)
+
+SELECT E'key\u000B=>value\u000B'::hstore; -- \v
+ hstore
+----------------
+ "key"=>"value"
+(1 row)
+
+SELECT E'key\u000C=>value\u000C'::hstore; -- \f
+ hstore
+----------------
+ "key"=>"value"
+(1 row)
+
diff --git a/contrib/hstore/hstore_io.c b/contrib/hstore/hstore_io.c
index 999ddad76d..f33b241d54 100644
--- a/contrib/hstore/hstore_io.c
+++ b/contrib/hstore/hstore_io.c
@@ -13,7 +13,6 @@
#include "lib/stringinfo.h"
#include "libpq/pqformat.h"
#include "nodes/miscnodes.h"
-#include "parser/scansup.h"
#include "utils/builtins.h"
#include "utils/json.h"
#include "utils/jsonb.h"
@@ -41,6 +40,7 @@ typedef struct
int plen;
} HSParser;
+static bool hstore_isspace(char ch);
static bool hstoreCheckKeyLength(size_t len, HSParser *state);
static bool hstoreCheckValLength(size_t len, HSParser *state);
@@ -82,6 +82,26 @@ prseof(HSParser *state)
return false;
}
+/*
+ * hstore_isspace() --- a non-locale-dependent isspace()
+ *
+ * We used to use isspace() for parsing hstore values, but that has
+ * undesirable results: a hstore value might be silently interpreted
+ * differently depending on the locale setting. Now we just hard-wire
+ * the traditional ASCII definition of isspace().
+ */
+static bool
+hstore_isspace(char ch)
+{
+ if (ch == ' ' ||
+ ch == '\t' ||
+ ch == '\n' ||
+ ch == '\r' ||
+ ch == '\v' ||
+ ch == '\f')
+ return true;
+ return false;
+}
#define GV_WAITVAL 0
#define GV_INVAL 1
@@ -119,7 +139,7 @@ get_val(HSParser *state, bool ignoreeq, bool *escaped)
{
st = GV_WAITESCIN;
}
- else if (!scanner_isspace((unsigned char) *(state->ptr)))
+ else if (!hstore_isspace((unsigned char) *(state->ptr)))
{
*(state->cur) = *(state->ptr);
state->cur++;
@@ -142,7 +162,7 @@ get_val(HSParser *state, bool ignoreeq, bool *escaped)
state->ptr--;
return true;
}
- else if (scanner_isspace((unsigned char) *(state->ptr)))
+ else if (hstore_isspace((unsigned char) *(state->ptr)))
{
return true;
}
@@ -256,7 +276,7 @@ parse_hstore(HSParser *state)
{
PRSEOF;
}
- else if (!scanner_isspace((unsigned char) *(state->ptr)))
+ else if (!hstore_isspace((unsigned char) *(state->ptr)))
{
PRSSYNTAXERROR;
}
@@ -310,7 +330,7 @@ parse_hstore(HSParser *state)
{
return true;
}
- else if (!scanner_isspace((unsigned char) *(state->ptr)))
+ else if (!hstore_isspace((unsigned char) *(state->ptr)))
{
PRSSYNTAXERROR;
}
diff --git a/contrib/hstore/sql/hstore_utf8.sql b/contrib/hstore/sql/hstore_utf8.sql
index face878324..38c9481ee6 100644
--- a/contrib/hstore/sql/hstore_utf8.sql
+++ b/contrib/hstore/sql/hstore_utf8.sql
@@ -17,3 +17,10 @@ SELECT E'key\u0105=>value\u0105'::hstore;
SELECT 'keyą=>valueą'::hstore;
SELECT 'ą=>ą'::hstore;
SELECT 'keyąfoo=>valueą'::hstore;
+
+-- More patterns that may depend on isspace() and locales, all discarded.
+SELECT E'key\u000A=>value\u000A'::hstore; -- \n
+SELECT E'key\u0009=>value\u0009'::hstore; -- \t
+SELECT E'key\u000D=>value\u000D'::hstore; -- \r
+SELECT E'key\u000B=>value\u000B'::hstore; -- \v
+SELECT E'key\u000C=>value\u000C'::hstore; -- \f
--
2.40.1
[application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc)
download
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [PATCH] hstore: Fix parsing on Mac OS X: isspace() is locale specific
@ 2023-06-18 16:38 Tom Lane <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Tom Lane @ 2023-06-18 16:38 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Evan Jones <[email protected]>; pgsql-hackers
Michael Paquier <[email protected]> writes:
> At the end, no need to do that. I have been able to hack the
> attached, that shows the difference of treatment for \v when running
> in macOS. Evan, what do you think?
FWIW, I think the status quo is fine. Having hstore do something that
is neither its historical behavior nor aligned with the core parser
doesn't seem like a great idea. I don't buy this argument that
somebody might be depending on the handling of \v in particular. It's
not any stronger than the argument that they might be depending on,
say, recognizing no-break space (0xA0) in LATIN1, which the old code
did (probably, depending on platform) and scanner_isspace will not.
If anything, the answer for these concerns is that d522b05c8
should not have been back-patched. But I'm okay with where we are.
regards, tom lane
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [PATCH] hstore: Fix parsing on Mac OS X: isspace() is locale specific
@ 2023-06-18 23:28 Michael Paquier <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Michael Paquier @ 2023-06-18 23:28 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Evan Jones <[email protected]>; pgsql-hackers
On Sun, Jun 18, 2023 at 12:38:12PM -0400, Tom Lane wrote:
> FWIW, I think the status quo is fine. Having hstore do something that
> is neither its historical behavior nor aligned with the core parser
> doesn't seem like a great idea.
Okay. Fine by me.
> I don't buy this argument that
> somebody might be depending on the handling of \v in particular. It's
> not any stronger than the argument that they might be depending on,
> say, recognizing no-break space (0xA0) in LATIN1, which the old code
> did (probably, depending on platform) and scanner_isspace will not.
Another thing that I was wondering, though.. Do you think that there
would be an argument in being stricter in the hstore code regarding
the handling of multi-byte characters with some checks based on
IS_HIGHBIT_SET() when parsing the keys and values?
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../ZI+TK2kch0ZZ%[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [PATCH] hstore: Fix parsing on Mac OS X: isspace() is locale specific
@ 2023-06-19 01:10 Tom Lane <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Tom Lane @ 2023-06-19 01:10 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Evan Jones <[email protected]>; pgsql-hackers
Michael Paquier <[email protected]> writes:
> Another thing that I was wondering, though.. Do you think that there
> would be an argument in being stricter in the hstore code regarding
> the handling of multi-byte characters with some checks based on
> IS_HIGHBIT_SET() when parsing the keys and values?
What have you got in mind? We should already have validated encoding
correctness before the text ever gets to hstore_in, and I'm not clear
what additional checks would be useful.
regards, tom lane
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [PATCH] hstore: Fix parsing on Mac OS X: isspace() is locale specific
@ 2023-06-20 06:02 Michael Paquier <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Michael Paquier @ 2023-06-20 06:02 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Evan Jones <[email protected]>; pgsql-hackers
On Sun, Jun 18, 2023 at 09:10:59PM -0400, Tom Lane wrote:
> What have you got in mind? We should already have validated encoding
> correctness before the text ever gets to hstore_in, and I'm not clear
> what additional checks would be useful.
I was staring at the hstore parsing code and got the impression that
multi-byte character handling could be improved, but looking closer it
seems that I got that wrong. Apologies for the noise.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [PATCH] hstore: Fix parsing on Mac OS X: isspace() is locale specific
@ 2023-06-20 13:04 Evan Jones <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Evan Jones @ 2023-06-20 13:04 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers
Thanks for the detailed discussion. To confirm that I've understood
everything:
* Michael's proposed patch to add hstore_isspace() would be a potential
fix: it resolves my original bug, and does not change the behavior of '\v'.
* We believe the change to '\v' is not a problem, and may be an improvement
because it now follows the "core" Postgres parser.
In conclusion: we don't need to make an additional change. Thank you all
for investigating!
My one last suggestion: We *could* revert the backpatching if we are
concerned about this change, but I'm not personally sure that is necessary.
As we discussed, this is an unusual corner case in an "extension" type that
many won't even have enabled.
Evan
On Tue, Jun 20, 2023 at 2:02 AM Michael Paquier <[email protected]> wrote:
> On Sun, Jun 18, 2023 at 09:10:59PM -0400, Tom Lane wrote:
> > What have you got in mind? We should already have validated encoding
> > correctness before the text ever gets to hstore_in, and I'm not clear
> > what additional checks would be useful.
>
> I was staring at the hstore parsing code and got the impression that
> multi-byte character handling could be improved, but looking closer it
> seems that I got that wrong. Apologies for the noise.
> --
> Michael
>
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [PATCH] hstore: Fix parsing on Mac OS X: isspace() is locale specific
@ 2023-06-21 00:02 Michael Paquier <[email protected]>
parent: Evan Jones <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Michael Paquier @ 2023-06-21 00:02 UTC (permalink / raw)
To: Evan Jones <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers
On Tue, Jun 20, 2023 at 09:04:26AM -0400, Evan Jones wrote:
> My one last suggestion: We *could* revert the backpatching if we are
> concerned about this change, but I'm not personally sure that is necessary.
> As we discussed, this is an unusual corner case in an "extension" type that
> many won't even have enabled.
As a whole, I'd like to think that this is an improvement even for
stable branches with these weird isspace() handlings, so I'm OK with
the current status in all the branches. There's an argument about \v,
IMO, but I won't fight hard for it either even if it would be more
consistent with the way array values are handled.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../ZJI+G7B%2FqblD6%[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [PATCH] hstore: Fix parsing on Mac OS X: isspace() is locale specific
@ 2023-06-21 03:39 Tom Lane <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Tom Lane @ 2023-06-21 03:39 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Evan Jones <[email protected]>; pgsql-hackers
Michael Paquier <[email protected]> writes:
> As a whole, I'd like to think that this is an improvement even for
> stable branches with these weird isspace() handlings, so I'm OK with
> the current status in all the branches.
Sounds like we're all content with that.
> There's an argument about \v,
> IMO, but I won't fight hard for it either even if it would be more
> consistent with the way array values are handled.
I'd be okay with adding \v to the set of whitespace characters in
scan.l and scanner_isspace (and other affected places) for v17.
Don't want to back-patch it though.
regards, tom lane
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [PATCH] hstore: Fix parsing on Mac OS X: isspace() is locale specific
@ 2023-06-21 06:49 Michael Paquier <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Michael Paquier @ 2023-06-21 06:49 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Evan Jones <[email protected]>; pgsql-hackers
On Tue, Jun 20, 2023 at 11:39:31PM -0400, Tom Lane wrote:
> I'd be okay with adding \v to the set of whitespace characters in
> scan.l and scanner_isspace (and other affected places) for v17.
> Don't want to back-patch it though.
Okay. No idea where this will lead, but for now I have sent a patch
that adds \v to the parser paths where it would be needed, as far as I
checked:
https://www.postgresql.org/message-id/[email protected]
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [PATCH] hstore: Fix parsing on Mac OS X: isspace() is locale specific
@ 2023-10-10 03:17 Thomas Munro <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Thomas Munro @ 2023-10-10 03:17 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Tom Lane <[email protected]>; Evan Jones <[email protected]>; pgsql-hackers
FTR I ran into a benign case of the phenomenon in this thread when
dealing with row types. In rowtypes.c, we double-quote stuff
containing spaces, but we detect them by passing individual bytes of
UTF-8 sequences to isspace(). Like macOS, Windows thinks that 0xa0 is
a space when you do that, so for example the Korean character '점'
(code point C810, UTF-8 sequence EC A0 90) gets quotes on Windows but
not on Linux. That confused a migration/diff tool while comparing
Windows and Linux database servers using that representation. Not a
big deal, I guess no one ever promised that the format was stable
across platforms, and I don't immediately see a way for anything more
serious to go wrong (though I may lack imagination). It does seem a
bit weird to be using locale-aware tokenising for a machine-readable
format, and then making sure its behaviour is undefined by feeding it
chopped up bytes.
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [PATCH] hstore: Fix parsing on Mac OS X: isspace() is locale specific
@ 2023-10-10 14:51 Evan Jones <[email protected]>
parent: Thomas Munro <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Evan Jones @ 2023-10-10 14:51 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; +Cc: Michael Paquier <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers
Thanks for bringing this up! I just looked at the uses if isspace() in that
file. It looks like it is the usual thing: it is allowing leading or
trailing whitespace when parsing values, or for this "needs quoting" logic
on output. The fix would be the same: this *should* be
using scanner_isspace. This has the same disadvantage: it would change
Postgres's results for some inputs that contain these non-ASCII "space"
characters.
Here is a quick demonstration of this issue, showing that the quoting
behavior is different between these two. Mac OS X with the "default" locale
includes quotes because ą includes 0x85 in its UTF-8 encoding:
postgres=# SELECT ROW('keyą');
row
----------
("keyą")
(1 row)
On Mac OS X with the LANG=C environment variable set, it does not include
quotes:
postgres=# SELECT ROW('keyą');
row
--------
(keyą)
(1 row)
On Mon, Oct 9, 2023 at 11:18 PM Thomas Munro <[email protected]> wrote:
> FTR I ran into a benign case of the phenomenon in this thread when
> dealing with row types. In rowtypes.c, we double-quote stuff
> containing spaces, but we detect them by passing individual bytes of
> UTF-8 sequences to isspace(). Like macOS, Windows thinks that 0xa0 is
> a space when you do that, so for example the Korean character '점'
> (code point C810, UTF-8 sequence EC A0 90) gets quotes on Windows but
> not on Linux. That confused a migration/diff tool while comparing
> Windows and Linux database servers using that representation. Not a
> big deal, I guess no one ever promised that the format was stable
> across platforms, and I don't immediately see a way for anything more
> serious to go wrong (though I may lack imagination). It does seem a
> bit weird to be using locale-aware tokenising for a machine-readable
> format, and then making sure its behaviour is undefined by feeding it
> chopped up bytes.
>
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [PATCH] hstore: Fix parsing on Mac OS X: isspace() is locale specific
@ 2023-10-10 23:34 Michael Paquier <[email protected]>
parent: Evan Jones <[email protected]>
0 siblings, 0 replies; 13+ messages in thread
From: Michael Paquier @ 2023-10-10 23:34 UTC (permalink / raw)
To: Evan Jones <[email protected]>; +Cc: Thomas Munro <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers
On Tue, Oct 10, 2023 at 10:51:10AM -0400, Evan Jones wrote:
> Here is a quick demonstration of this issue, showing that the quoting
> behavior is different between these two. Mac OS X with the "default" locale
> includes quotes because ą includes 0x85 in its UTF-8 encoding:
Ugh. rowtypes.c has reminded me as well of gistfuncs.c in pageinspect
where included columns are printed in a ROW-like fashion. And it also
uses isspace() when we check if double quotes are needed or not. So
the use of the quotes would equally depend on what macos thinks is
a correct space in this case.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 13+ messages in thread
end of thread, other threads:[~2023-10-10 23:34 UTC | newest]
Thread overview: 13+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-02-17 09:17 [PATCH v13 1/2] Reusable decimalLength functions Dmitrii Dolgov <[email protected]>
2023-06-18 08:32 Re: [PATCH] hstore: Fix parsing on Mac OS X: isspace() is locale specific Michael Paquier <[email protected]>
2023-06-18 16:38 ` Re: [PATCH] hstore: Fix parsing on Mac OS X: isspace() is locale specific Tom Lane <[email protected]>
2023-06-18 23:28 ` Re: [PATCH] hstore: Fix parsing on Mac OS X: isspace() is locale specific Michael Paquier <[email protected]>
2023-06-19 01:10 ` Re: [PATCH] hstore: Fix parsing on Mac OS X: isspace() is locale specific Tom Lane <[email protected]>
2023-06-20 06:02 ` Re: [PATCH] hstore: Fix parsing on Mac OS X: isspace() is locale specific Michael Paquier <[email protected]>
2023-06-20 13:04 ` Re: [PATCH] hstore: Fix parsing on Mac OS X: isspace() is locale specific Evan Jones <[email protected]>
2023-06-21 00:02 ` Re: [PATCH] hstore: Fix parsing on Mac OS X: isspace() is locale specific Michael Paquier <[email protected]>
2023-06-21 03:39 ` Re: [PATCH] hstore: Fix parsing on Mac OS X: isspace() is locale specific Tom Lane <[email protected]>
2023-06-21 06:49 ` Re: [PATCH] hstore: Fix parsing on Mac OS X: isspace() is locale specific Michael Paquier <[email protected]>
2023-10-10 03:17 ` Re: [PATCH] hstore: Fix parsing on Mac OS X: isspace() is locale specific Thomas Munro <[email protected]>
2023-10-10 14:51 ` Re: [PATCH] hstore: Fix parsing on Mac OS X: isspace() is locale specific Evan Jones <[email protected]>
2023-10-10 23:34 ` Re: [PATCH] hstore: Fix parsing on Mac OS X: isspace() is locale specific Michael Paquier <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox