pgsql: Implementation of subscripting for jsonb
Implementation of subscripting for jsonb
Subscripting for jsonb does not support slices, does not have a limit for the
number of subscripts, and an assignment expects a replace value to have jsonb
type. There is also one functional difference between assignment via
subscripting and assignment via jsonb_set(). When an original jsonb container
is NULL, the subscripting replaces it with an empty jsonb and proceeds with
an assignment.
For the sake of code reuse, we rearrange some parts of jsonb functionality
to allow the usage of the same functions for jsonb_set and assign subscripting
operation.
The original idea belongs to Oleg Bartunov.
Catversion is bumped.
Discussion: /messages/by-id/CA+q6zcV8qvGcDXurwwgUbwACV86Th7G80pnubg42e-p9gsSf=g@mail.gmail.com
Discussion: /messages/by-id/CA+q6zcX3mdxGCgdThzuySwH-ApyHHM-G4oB1R0fn0j2hZqqkLQ@mail.gmail.com
Discussion: /messages/by-id/CA+q6zcVDuGBv=M0FqBYX8DPebS3F_0KQ6OVFobGJPM507_SZ_w@mail.gmail.com
Discussion: /messages/by-id/CA+q6zcVovR+XY4mfk-7oNk-rF91gH0PebnNfuUjuuDsyHjOcVA@mail.gmail.com
Author: Dmitry Dolgov
Reviewed-by: Tom Lane, Arthur Zakirov, Pavel Stehule, Dian M Fay
Reviewed-by: Andrew Dunstan, Chapman Flack, Merlin Moncure, Peter Geoghegan
Reviewed-by: Alvaro Herrera, Jim Nasby, Josh Berkus, Victor Wagner
Reviewed-by: Aleksander Alekseev, Robert Haas, Oleg Bartunov
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/676887a3b0b8e3c0348ac3f82ab0d16e9a24bd43
Modified Files
--------------
doc/src/sgml/json.sgml | 51 +++++
src/backend/utils/adt/Makefile | 1 +
src/backend/utils/adt/jsonb_util.c | 72 ++++++-
src/backend/utils/adt/jsonbsubs.c | 412 ++++++++++++++++++++++++++++++++++++
src/backend/utils/adt/jsonfuncs.c | 188 ++++++++--------
src/include/catalog/catversion.h | 2 +-
src/include/catalog/pg_proc.dat | 4 +
src/include/catalog/pg_type.dat | 3 +-
src/include/utils/jsonb.h | 6 +-
src/test/regress/expected/jsonb.out | 272 +++++++++++++++++++++++-
src/test/regress/sql/jsonb.sql | 84 +++++++-
src/tools/pgindent/typedefs.list | 1 +
12 files changed, 988 insertions(+), 108 deletions(-)
On 31/01/2021 22:54, Alexander Korotkov wrote:
Implementation of subscripting for jsonb
The Itanium and sparc64 buildfarm members didn't like this, and are
crashing at "select ('123'::jsonb)['a'];". Unaligned memory access, perhaps?
- Heikki
Heikki Linnakangas <hlinnaka@iki.fi> writes:
On 31/01/2021 22:54, Alexander Korotkov wrote:
Implementation of subscripting for jsonb
The Itanium and sparc64 buildfarm members didn't like this, and are
crashing at "select ('123'::jsonb)['a'];". Unaligned memory access, perhaps?
I think I just identified the cause.
regards, tom lane
On Mon, Feb 1, 2021 at 10:06 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Heikki Linnakangas <hlinnaka@iki.fi> writes:
On 31/01/2021 22:54, Alexander Korotkov wrote:
Implementation of subscripting for jsonb
The Itanium and sparc64 buildfarm members didn't like this, and are
crashing at "select ('123'::jsonb)['a'];". Unaligned memory access, perhaps?I think I just identified the cause.
Thanks again for fixing this.
BTW, I managed to reproduce the issue by compiling with CFLAGS="-O0
-fsanitize=alignment -fsanitize-trap=alignment" and the patch
attached.
I can propose the following to catch such issues earlier. We could
finish (wrap attribute with macro and apply it to other places with
misalignment access if any) and apply the attached patch and make
commitfest.cputube.org check patches with CFLAGS="-O0
-fsanitize=alignment -fsanitize-trap=alignment". What do you think?
------
Regards,
Alexander Korotkov
Attachments:
pg_comp_crc32c_sse42_no_sanitize_alignment.patchapplication/octet-stream; name=pg_comp_crc32c_sse42_no_sanitize_alignment.patchDownload
diff --git a/src/port/pg_crc32c_sse42.c b/src/port/pg_crc32c_sse42.c
index 3b94a7388ab..cc5f8f9b487 100644
--- a/src/port/pg_crc32c_sse42.c
+++ b/src/port/pg_crc32c_sse42.c
@@ -18,6 +18,7 @@
#include "port/pg_crc32c.h"
+__attribute__((no_sanitize("alignment")))
pg_crc32c
pg_comp_crc32c_sse42(pg_crc32c crc, const void *data, size_t len)
{
On Mon, Feb 1, 2021 at 3:41 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
On Mon, Feb 1, 2021 at 10:06 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Heikki Linnakangas <hlinnaka@iki.fi> writes:
On 31/01/2021 22:54, Alexander Korotkov wrote:
Implementation of subscripting for jsonb
The Itanium and sparc64 buildfarm members didn't like this, and are
crashing at "select ('123'::jsonb)['a'];". Unaligned memory access, perhaps?I think I just identified the cause.
Thanks again for fixing this.
BTW, I managed to reproduce the issue by compiling with CFLAGS="-O0
-fsanitize=alignment -fsanitize-trap=alignment" and the patch
attached.I can propose the following to catch such issues earlier. We could
finish (wrap attribute with macro and apply it to other places with
misalignment access if any) and apply the attached patch and make
commitfest.cputube.org check patches with CFLAGS="-O0
-fsanitize=alignment -fsanitize-trap=alignment". What do you think?
The revised patch is attached. The attribute is wrapped into
pg_attribute_no_sanitize_alignment() macro. I've checked it works for
me with gcc-10 and clang-11.
------
Regards,
Alexander Korotkov
Attachments:
pg_attribute_no_sanitize_alignment.patchapplication/octet-stream; name=pg_attribute_no_sanitize_alignment.patchDownload
diff --git a/src/include/c.h b/src/include/c.h
index ae978830daf..47b97a9eb73 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -120,6 +120,13 @@
#define pg_attribute_unused()
#endif
+/* Macro for disabling GCC alignment sanitizer */
+#ifdef __GNUC__
+#define pg_attribute_no_sanitize_alignment() __attribute__((no_sanitize("alignment")))
+#else
+#define pg_attribute_no_sanitize_alignment()
+#endif
+
/*
* pg_nodiscard means the compiler should warn if the result of a function
* call is ignored. The name "nodiscard" is chosen in alignment with
diff --git a/src/port/pg_crc32c_sse42.c b/src/port/pg_crc32c_sse42.c
index 3b94a7388ab..10fc01e1f06 100644
--- a/src/port/pg_crc32c_sse42.c
+++ b/src/port/pg_crc32c_sse42.c
@@ -18,6 +18,7 @@
#include "port/pg_crc32c.h"
+pg_attribute_no_sanitize_alignment()
pg_crc32c
pg_comp_crc32c_sse42(pg_crc32c crc, const void *data, size_t len)
{
[ redirecting to -hackers ]
Alexander Korotkov <aekorotkov@gmail.com> writes:
BTW, I managed to reproduce the issue by compiling with CFLAGS="-O0
-fsanitize=alignment -fsanitize-trap=alignment" and the patch
attached.
I can propose the following to catch such issues earlier. We could
finish (wrap attribute with macro and apply it to other places with
misalignment access if any) and apply the attached patch and make
commitfest.cputube.org check patches with CFLAGS="-O0
-fsanitize=alignment -fsanitize-trap=alignment". What do you think?
The revised patch is attached. The attribute is wrapped into
pg_attribute_no_sanitize_alignment() macro. I've checked it works for
me with gcc-10 and clang-11.
I found some time to experiment with this today. It is really nice
to be able to detect these problems without using obsolete hardware.
However, I have a few issues:
* Why do you recommend -O0? Seems to me we want to test the code
as we'd normally use it, ie typically -O2.
* -fsanitize-trap=alignment seems to be a clang-ism; gcc won't take it.
However, after some experimenting I found that "-fno-sanitize-recover=all"
(or "-fno-sanitize-recover=alignment" if you prefer) produces roughly
equivalent results on gcc.
* Both clang and gcc seem to be happy with the same spelling of the
function attribute, which is fortunate. However, I seriously doubt
that bare "#ifdef __GNUC__" is going to be good enough. At the very
least there's going to need to be a compiler version test in there,
and we might end up needing to get the configure script involved.
* I think the right place to run such a check is in some buildfarm
animals. The cfbot only sees portions of what goes into our tree.
regards, tom lane
I wrote:
* Both clang and gcc seem to be happy with the same spelling of the
function attribute, which is fortunate. However, I seriously doubt
that bare "#ifdef __GNUC__" is going to be good enough. At the very
least there's going to need to be a compiler version test in there,
and we might end up needing to get the configure script involved.
After digging in gcc's release history, it seems they invented
"-fsanitize=alignment" in GCC 5, so we can make this work for gcc
by writing
#if __GNUC__ >= 5
(the likely() macro already uses a similar approach). Can't say
if that's close enough for clang too.
regards, tom lane
I wrote:
After digging in gcc's release history, it seems they invented
"-fsanitize=alignment" in GCC 5, so we can make this work for gcc
by writing
#if __GNUC__ >= 5
(the likely() macro already uses a similar approach). Can't say
if that's close enough for clang too.
Ugh, no it isn't: even pretty recent clang releases only define
__GNUC__ as 4. It looks like we need a separate test on clang's
version. I looked at their version history and sanitizers seem
to have come in around clang 7, so I propose the attached (where
I worked a bit harder on the comment, too).
regards, tom lane
Attachments:
pg_attribute_no_sanitize_alignment-2.patchtext/x-diff; charset=us-ascii; name=pg_attribute_no_sanitize_alignment-2.patchDownload
diff --git a/src/include/c.h b/src/include/c.h
index ae978830da..a86342093e 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -132,6 +132,18 @@
#define pg_nodiscard
#endif
+/*
+ * Place this macro before functions that should be allowed to make misaligned
+ * accesses. Think twice before using it on non-x86-specific code!
+ * Testing can be done with "-fsanitize=alignment -fsanitize-trap=alignment"
+ * on clang, or "-fsanitize=alignment -fno-sanitize-recover=alignment" on gcc.
+ */
+#if __clang_major__ >= 7 || __GNUC__ >= 5
+#define pg_attribute_no_sanitize_alignment() __attribute__((no_sanitize("alignment")))
+#else
+#define pg_attribute_no_sanitize_alignment()
+#endif
+
/*
* Append PG_USED_FOR_ASSERTS_ONLY to definitions of variables that are only
* used in assert-enabled builds, to avoid compiler warnings about unused
diff --git a/src/port/pg_crc32c_sse42.c b/src/port/pg_crc32c_sse42.c
index 3b94a7388a..10fc01e1f0 100644
--- a/src/port/pg_crc32c_sse42.c
+++ b/src/port/pg_crc32c_sse42.c
@@ -18,6 +18,7 @@
#include "port/pg_crc32c.h"
+pg_attribute_no_sanitize_alignment()
pg_crc32c
pg_comp_crc32c_sse42(pg_crc32c crc, const void *data, size_t len)
{
Hi, Tom!
Thank you for taking care of this.
On Mon, Feb 8, 2021 at 3:47 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
[ redirecting to -hackers ]
Alexander Korotkov <aekorotkov@gmail.com> writes:
BTW, I managed to reproduce the issue by compiling with CFLAGS="-O0
-fsanitize=alignment -fsanitize-trap=alignment" and the patch
attached.
I can propose the following to catch such issues earlier. We could
finish (wrap attribute with macro and apply it to other places with
misalignment access if any) and apply the attached patch and make
commitfest.cputube.org check patches with CFLAGS="-O0
-fsanitize=alignment -fsanitize-trap=alignment". What do you think?The revised patch is attached. The attribute is wrapped into
pg_attribute_no_sanitize_alignment() macro. I've checked it works for
me with gcc-10 and clang-11.I found some time to experiment with this today. It is really nice
to be able to detect these problems without using obsolete hardware.
However, I have a few issues:* Why do you recommend -O0? Seems to me we want to test the code
as we'd normally use it, ie typically -O2.
My idea was that with -O0 we can see some unaligned accesses, which
would be optimized away with -O2. I mean with -O2 we might completely
skip accessing some pointer, which would be accessed in -O0. However,
this situation is probably very rare.
* I think the right place to run such a check is in some buildfarm
animals. The cfbot only sees portions of what goes into our tree.
Could we have both cfbot + buildfarm animals?
------
Regards,
Alexander Korotkov
On Mon, Feb 8, 2021 at 7:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
I wrote:
After digging in gcc's release history, it seems they invented
"-fsanitize=alignment" in GCC 5, so we can make this work for gcc
by writing
#if __GNUC__ >= 5
(the likely() macro already uses a similar approach). Can't say
if that's close enough for clang too.Ugh, no it isn't: even pretty recent clang releases only define
__GNUC__ as 4. It looks like we need a separate test on clang's
version. I looked at their version history and sanitizers seem
to have come in around clang 7, so I propose the attached (where
I worked a bit harder on the comment, too).
Looks good to me. Thank you for revising!
------
Regards,
Alexander Korotkov
Alexander Korotkov <aekorotkov@gmail.com> writes:
On Mon, Feb 8, 2021 at 7:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Ugh, no it isn't: even pretty recent clang releases only define
__GNUC__ as 4. It looks like we need a separate test on clang's
version. I looked at their version history and sanitizers seem
to have come in around clang 7, so I propose the attached (where
I worked a bit harder on the comment, too).
Looks good to me. Thank you for revising!
Were you going to push this, or did you expect me to?
regards, tom lane
On Tue, Feb 9, 2021 at 1:34 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
Could we have both cfbot + buildfarm animals?
Hi Alexander,
For cfbot, yeah it does seem like a good idea to throw whatever code
sanitiser stuff we can into the automated tests, especially stuff that
isn't prone to false alarms. Can you please recommend an exact change
to apply to:
https://github.com/macdice/cfbot/blob/master/cirrus/.cirrus.yml
Note that FreeBSD and macOS are using clang (though you might think
the latter is using gcc from its configure output...), and Linux is
using gcc.
On Thu, Feb 11, 2021 at 9:46 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alexander Korotkov <aekorotkov@gmail.com> writes:
On Mon, Feb 8, 2021 at 7:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Ugh, no it isn't: even pretty recent clang releases only define
__GNUC__ as 4. It looks like we need a separate test on clang's
version. I looked at their version history and sanitizers seem
to have come in around clang 7, so I propose the attached (where
I worked a bit harder on the comment, too).Looks good to me. Thank you for revising!
Were you going to push this, or did you expect me to?
Thank you for noticing. I'll commit this today.
------
Regards,
Alexander Korotkov
Hi, Thomas!
On Fri, Feb 12, 2021 at 12:04 AM Thomas Munro <thomas.munro@gmail.com> wrote:
On Tue, Feb 9, 2021 at 1:34 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
Could we have both cfbot + buildfarm animals?
For cfbot, yeah it does seem like a good idea to throw whatever code
sanitiser stuff we can into the automated tests, especially stuff that
isn't prone to false alarms. Can you please recommend an exact change
to apply to:https://github.com/macdice/cfbot/blob/master/cirrus/.cirrus.yml
Note that FreeBSD and macOS are using clang (though you might think
the latter is using gcc from its configure output...), and Linux is
using gcc.
Thank you for the feedback!
I'll propose a pull-request at github.
------
Regards,
Alexander Korotkov
I've updated buildfarm member longfin to use "-fsanitize=alignment
-fsanitize-trap=alignment", and it just got through a run successfully
with that. It'd be good perhaps if some other buildfarm owners
followed suit (mumble JIT coverage mumble).
Looking around at other recent reports, it looks like we'll need to tweak
the compiler version cutoffs a bit. I see for instance that spurfowl,
with gcc (Ubuntu 5.4.0-6ubuntu1~16.04.11) 5.4.0 20160609, is whining:
pg_crc32c_sse42.c:24:1: warning: \342\200\230no_sanitize\342\200\231 attribute directive ignored [-Wattributes]
So maybe it'd better be __GNUC__ >= 6 not __GNUC__ >= 5. I think
we can wait a little bit for more reports before messing with that,
though.
Once this does settle, should we consider back-patching so that it's
possible to run alignment checks in the back branches too?
regards, tom lane
On Fri, Feb 12, 2021 at 8:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
I've updated buildfarm member longfin to use "-fsanitize=alignment
-fsanitize-trap=alignment", and it just got through a run successfully
with that. It'd be good perhaps if some other buildfarm owners
followed suit (mumble JIT coverage mumble).Looking around at other recent reports, it looks like we'll need to tweak
the compiler version cutoffs a bit. I see for instance that spurfowl,
with gcc (Ubuntu 5.4.0-6ubuntu1~16.04.11) 5.4.0 20160609, is whining:pg_crc32c_sse42.c:24:1: warning: \342\200\230no_sanitize\342\200\231 attribute directive ignored [-Wattributes]
So maybe it'd better be __GNUC__ >= 6 not __GNUC__ >= 5. I think
we can wait a little bit for more reports before messing with that,
though.
I've rechecked this in the documentation. no_sanitize attribute seems
to appear since gcc 8.0. Much later than alignment sanitizer itself.
https://gcc.gnu.org/gcc-8/changes.html
"A new attribute no_sanitize can be applied to functions to instruct
the compiler not to do sanitization of the options provided as
arguments to the attribute. Acceptable values for no_sanitize match
those acceptable by the -fsanitize command-line option."
Yes, let's wait for more feedback from buildfarm and fix the version
requirement.
Once this does settle, should we consider back-patching so that it's
possible to run alignment checks in the back branches too?
+1
------
Regards,
Alexander Korotkov
I wrote:
Looking around at other recent reports, it looks like we'll need to tweak
the compiler version cutoffs a bit. I see for instance that spurfowl,
with gcc (Ubuntu 5.4.0-6ubuntu1~16.04.11) 5.4.0 20160609, is whining:
...
So maybe it'd better be __GNUC__ >= 6 not __GNUC__ >= 5. I think
we can wait a little bit for more reports before messing with that,
though.
Further reports show that gcc 6.x and 7.x also produce warnings,
so I moved the cutoff up to 8. Hopefully that's good enough.
We could write a configure test instead, but I'd just as soon not
expend configure cycles on this.
regards, tom lane
Alexander Korotkov <aekorotkov@gmail.com> writes:
On Fri, Feb 12, 2021 at 8:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
So maybe it'd better be __GNUC__ >= 6 not __GNUC__ >= 5. I think
we can wait a little bit for more reports before messing with that,
though.
I've rechecked this in the documentation. no_sanitize attribute seems
to appear since gcc 8.0. Much later than alignment sanitizer itself.
Yeah, I'd just come to that conclusion from scraping the buildfarm
logs. Good to see it confirmed in the manual though.
Once this does settle, should we consider back-patching so that it's
possible to run alignment checks in the back branches too?
+1
Let's make sure we have a clean set of builds and then do that.
regards, tom lane
I wrote:
Alexander Korotkov <aekorotkov@gmail.com> writes:
On Fri, Feb 12, 2021 at 8:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Once this does settle, should we consider back-patching so that it's
possible to run alignment checks in the back branches too?
+1
Let's make sure we have a clean set of builds and then do that.
The buildfarm seems to be happy --- the active members that haven't
reported in should be unaffected by this patch, either because their
compiler versions are too old or because they're not x86 architecture.
So I went ahead and back-patched, and have adjusted longfin to apply
the -fsanitize switch in all branches.
(I've checked that 9.6 passes check-world this way, but not the
intermediate branches, so it's possible something will fail...)
regards, tom lane
On Sun, Feb 14, 2021 at 1:59 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
I wrote:
Alexander Korotkov <aekorotkov@gmail.com> writes:
On Fri, Feb 12, 2021 at 8:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Once this does settle, should we consider back-patching so that it's
possible to run alignment checks in the back branches too?+1
Let's make sure we have a clean set of builds and then do that.
The buildfarm seems to be happy --- the active members that haven't
reported in should be unaffected by this patch, either because their
compiler versions are too old or because they're not x86 architecture.
So I went ahead and back-patched, and have adjusted longfin to apply
the -fsanitize switch in all branches.
Perfect, thank you very much!
------
Regards,
Alexander Korotkov