Horton

Thread

Commits

  1. pg_attribute_no_sanitize_alignment() macro

  2. Tweak compiler version cutoff for no_sanitize("alignment") support.

  3. Implementation of subscripting for jsonb

  1. pgsql: Implementation of subscripting for jsonb

    Alexander Korotkov <akorotkov@postgresql.org> — 2021-01-31T20:54:27Z

    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: https://postgr.es/m/CA%2Bq6zcV8qvGcDXurwwgUbwACV86Th7G80pnubg42e-p9gsSf%3Dg%40mail.gmail.com
Discussion: https://postgr.es/m/CA%2Bq6zcX3mdxGCgdThzuySwH-ApyHHM-G4oB1R0fn0j2hZqqkLQ%40mail.gmail.com
Discussion: https://postgr.es/m/CA%2Bq6zcVDuGBv%3DM0FqBYX8DPebS3F_0KQ6OVFobGJPM507_SZ_w%40mail.gmail.com
Discussion: https://postgr.es/m/CA%2Bq6zcVovR%2BXY4mfk-7oNk-rF91gH0PebnNfuUjuuDsyHjOcVA%40mail.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(-)

  2. Re: pgsql: Implementation of subscripting for jsonb

    Heikki Linnakangas <hlinnaka@iki.fi> — 2021-02-01T06:55:52Z

    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

  3. Re: pgsql: Implementation of subscripting for jsonb

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-02-01T07:05:10Z

    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

  4. Re: pgsql: Implementation of subscripting for jsonb

    Alexander Korotkov <aekorotkov@gmail.com> — 2021-02-01T12:41:59Z

    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

  5. Re: pgsql: Implementation of subscripting for jsonb

    Alexander Korotkov <aekorotkov@gmail.com> — 2021-02-01T13:00:21Z

    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

  6. Detecting pointer misalignment (was Re: pgsql: Implementation of subscripting for jsonb)

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-02-08T00:47:37Z

    [ 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

  7. Re: Detecting pointer misalignment (was Re: pgsql: Implementation of subscripting for jsonb)

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-02-08T01:20:00Z

    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

  8. Re: Detecting pointer misalignment (was Re: pgsql: Implementation of subscripting for jsonb)

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-02-08T16:49:41Z

    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

  9. Re: Detecting pointer misalignment (was Re: pgsql: Implementation of subscripting for jsonb)

    Alexander Korotkov <aekorotkov@gmail.com> — 2021-02-09T00:34:27Z

    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

  10. Re: Detecting pointer misalignment (was Re: pgsql: Implementation of subscripting for jsonb)

    Alexander Korotkov <aekorotkov@gmail.com> — 2021-02-09T00:35:01Z

    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

  11. Re: Detecting pointer misalignment (was Re: pgsql: Implementation of subscripting for jsonb)

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-02-11T18:46:07Z

    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

  12. Re: Detecting pointer misalignment (was Re: pgsql: Implementation of subscripting for jsonb)

    Thomas Munro <thomas.munro@gmail.com> — 2021-02-11T21:03:55Z

    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.

  13. Re: Detecting pointer misalignment (was Re: pgsql: Implementation of subscripting for jsonb)

    Alexander Korotkov <aekorotkov@gmail.com> — 2021-02-12T01:36:02Z

    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

  14. Re: Detecting pointer misalignment (was Re: pgsql: Implementation of subscripting for jsonb)

    Alexander Korotkov <aekorotkov@gmail.com> — 2021-02-12T14:29:48Z

    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

  15. Re: Detecting pointer misalignment (was Re: pgsql: Implementation of subscripting for jsonb)

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-02-12T17:19:43Z

    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

  16. Re: Detecting pointer misalignment (was Re: pgsql: Implementation of subscripting for jsonb)

    Alexander Korotkov <aekorotkov@gmail.com> — 2021-02-12T22:29:43Z

    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

  17. Re: Detecting pointer misalignment (was Re: pgsql: Implementation of subscripting for jsonb)

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-02-12T22:35:46Z

    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

  18. Re: Detecting pointer misalignment (was Re: pgsql: Implementation of subscripting for jsonb)

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-02-12T22:55:00Z

    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

  19. Re: Detecting pointer misalignment (was Re: pgsql: Implementation of subscripting for jsonb)

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-02-13T22:59:09Z

    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

  20. Re: Detecting pointer misalignment (was Re: pgsql: Implementation of subscripting for jsonb)

    Alexander Korotkov <aekorotkov@gmail.com> — 2021-02-14T00:42:28Z

    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