pg.ddx.io pgsql-hackers@postgresql.org mailing list archive help / color / mirror / Atom feed From: Nathan Bossart <nathandbossart@gmail.com>
To: jian he <jian.universality@gmail.com>
Cc: Joseph Koshakow <koshy44@gmail.com>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Matthew Kim <matthewkmkim@gmail.com>
Cc: Alexander Lakhin <exclusion@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Andres Freund <andres@anarazel.de>
Subject: Re: Remove dependence on integer wrapping
Date: Thu, 15 Aug 2024 16:34:30 -0500
Message-ID: <Zr50ZpSyrBx0EHfR@nathan> (raw )
In-Reply-To: <Zr5M3KxOZlEZRZIN@nathan >
References: <CAAvxfHfuxiMG4_YB5oBmJtK7mQx6FRKcUhNueXGwKT6_r=4W_Q@mail.gmail.com >
<CACJufxFUKTQx8wUpxtRPT31D+a7fNxAL_A_-S4XaR-oZkKEgxA@mail.gmail.com >
<ZrvUOkL19KHEH2sb@nathan >
<ZrwfgNyDXy99NUOS@nathan >
<875ad3ec-f216-487e-baad-cdbaf32c1fcf@iki.fi >
<ZrznaJTIOPgSjEJ7@nathan >
<CAAvxfHc2Smk92cJX4aUju1AdW_RpNN8csEj5rvf29b-YFHRmfQ@mail.gmail.com >
<Zrz0Zv4PVFxieEii@nathan >
<CACJufxH-uLR5SnnmVbGYj7Oy90zF_pYV3G_HxfbJ6Lu3XKGXxQ@mail.gmail.com >
<Zr5M3KxOZlEZRZIN@nathan >
I've committed 0001. Now to 0002...
- if (-element > nelements)
+ if (element == PG_INT32_MIN || -element > nelements)
This seems like a good opportunity to use our new pg_abs_s32() function,
and godbolt.org [0] seems to indicate that it might produce better code,
too (at least to my eye). I've attached an updated version of the patch
with this change. Barring additional feedback, I plan to commit this one
shortly.
[0] https://godbolt.org/z/57P4vvGYf
--
nathan
From 62677b351cf18dee37dc0d9253bd694fe6fbf26a Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <koshy44@gmail.com>
Date: Sat, 6 Jul 2024 15:41:09 -0400
Subject: [PATCH v23 1/1] Remove dependence on integer wrapping for jsonb
This commit updates various jsonb operators and functions to no longer
rely on integer wrapping for correctness. Not all compilers support
-fwrapv, so it's best not to rely on it.
---
src/backend/utils/adt/jsonfuncs.c | 7 ++++---
src/test/regress/expected/jsonb.out | 12 ++++++++++++
src/test/regress/sql/jsonb.sql | 2 ++
3 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index 5ecb9fffae..1f8ea51e6a 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -18,6 +18,7 @@
#include "access/htup_details.h"
#include "catalog/pg_type.h"
+#include "common/int.h"
#include "common/jsonapi.h"
#include "common/string.h"
#include "fmgr.h"
@@ -946,7 +947,7 @@ jsonb_array_element(PG_FUNCTION_ARGS)
{
uint32 nelements = JB_ROOT_COUNT(jb);
- if (-element > nelements)
+ if (pg_abs_s32(element) > nelements)
PG_RETURN_NULL();
else
element += nelements;
@@ -5426,7 +5427,7 @@ setPathArray(JsonbIterator **it, Datum *path_elems, bool *path_nulls,
if (idx < 0)
{
- if (-idx > nelems)
+ if (pg_abs_s32(idx) > nelems)
{
/*
* If asked to keep elements position consistent, it's not allowed
@@ -5438,7 +5439,7 @@ setPathArray(JsonbIterator **it, Datum *path_elems, bool *path_nulls,
errmsg("path element at position %d is out of range: %d",
level + 1, idx)));
else
- idx = INT_MIN;
+ idx = PG_INT32_MIN;
}
else
idx = nelems + idx;
diff --git a/src/test/regress/expected/jsonb.out b/src/test/regress/expected/jsonb.out
index e66d760189..a9d93052fc 100644
--- a/src/test/regress/expected/jsonb.out
+++ b/src/test/regress/expected/jsonb.out
@@ -680,6 +680,18 @@ select '"foo"'::jsonb -> 'z';
(1 row)
+select '[]'::jsonb -> -2147483648;
+ ?column?
+----------
+
+(1 row)
+
+select jsonb_delete_path('{"a":[]}', '{"a",-2147483648}');
+ jsonb_delete_path
+-------------------
+ {"a": []}
+(1 row)
+
select '{"a": [{"b": "c"}, {"b": "cc"}]}'::jsonb ->> null::text;
?column?
----------
diff --git a/src/test/regress/sql/jsonb.sql b/src/test/regress/sql/jsonb.sql
index 97bc2242a1..6a18577ead 100644
--- a/src/test/regress/sql/jsonb.sql
+++ b/src/test/regress/sql/jsonb.sql
@@ -204,6 +204,8 @@ select '[{"b": "c"}, {"b": "cc"}]'::jsonb -> 'z';
select '{"a": "c", "b": null}'::jsonb -> 'b';
select '"foo"'::jsonb -> 1;
select '"foo"'::jsonb -> 'z';
+select '[]'::jsonb -> -2147483648;
+select jsonb_delete_path('{"a":[]}', '{"a",-2147483648}');
select '{"a": [{"b": "c"}, {"b": "cc"}]}'::jsonb ->> null::text;
select '{"a": [{"b": "c"}, {"b": "cc"}]}'::jsonb ->> null::int;
--
2.39.3 (Apple Git-146)
Attachments:
[text/plain] v23-0001-Remove-dependence-on-integer-wrapping-for-jsonb.patch (2.9K, ../Zr50ZpSyrBx0EHfR@nathan/2-v23-0001-Remove-dependence-on-integer-wrapping-for-jsonb.patch)
download | inline diff:
From 62677b351cf18dee37dc0d9253bd694fe6fbf26a Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <koshy44@gmail.com>
Date: Sat, 6 Jul 2024 15:41:09 -0400
Subject: [PATCH v23 1/1] Remove dependence on integer wrapping for jsonb
This commit updates various jsonb operators and functions to no longer
rely on integer wrapping for correctness. Not all compilers support
-fwrapv, so it's best not to rely on it.
---
src/backend/utils/adt/jsonfuncs.c | 7 ++++---
src/test/regress/expected/jsonb.out | 12 ++++++++++++
src/test/regress/sql/jsonb.sql | 2 ++
3 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index 5ecb9fffae..1f8ea51e6a 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -18,6 +18,7 @@
#include "access/htup_details.h"
#include "catalog/pg_type.h"
+#include "common/int.h"
#include "common/jsonapi.h"
#include "common/string.h"
#include "fmgr.h"
@@ -946,7 +947,7 @@ jsonb_array_element(PG_FUNCTION_ARGS)
{
uint32 nelements = JB_ROOT_COUNT(jb);
- if (-element > nelements)
+ if (pg_abs_s32(element) > nelements)
PG_RETURN_NULL();
else
element += nelements;
@@ -5426,7 +5427,7 @@ setPathArray(JsonbIterator **it, Datum *path_elems, bool *path_nulls,
if (idx < 0)
{
- if (-idx > nelems)
+ if (pg_abs_s32(idx) > nelems)
{
/*
* If asked to keep elements position consistent, it's not allowed
@@ -5438,7 +5439,7 @@ setPathArray(JsonbIterator **it, Datum *path_elems, bool *path_nulls,
errmsg("path element at position %d is out of range: %d",
level + 1, idx)));
else
- idx = INT_MIN;
+ idx = PG_INT32_MIN;
}
else
idx = nelems + idx;
diff --git a/src/test/regress/expected/jsonb.out b/src/test/regress/expected/jsonb.out
index e66d760189..a9d93052fc 100644
--- a/src/test/regress/expected/jsonb.out
+++ b/src/test/regress/expected/jsonb.out
@@ -680,6 +680,18 @@ select '"foo"'::jsonb -> 'z';
(1 row)
+select '[]'::jsonb -> -2147483648;
+ ?column?
+----------
+
+(1 row)
+
+select jsonb_delete_path('{"a":[]}', '{"a",-2147483648}');
+ jsonb_delete_path
+-------------------
+ {"a": []}
+(1 row)
+
select '{"a": [{"b": "c"}, {"b": "cc"}]}'::jsonb ->> null::text;
?column?
----------
diff --git a/src/test/regress/sql/jsonb.sql b/src/test/regress/sql/jsonb.sql
index 97bc2242a1..6a18577ead 100644
--- a/src/test/regress/sql/jsonb.sql
+++ b/src/test/regress/sql/jsonb.sql
@@ -204,6 +204,8 @@ select '[{"b": "c"}, {"b": "cc"}]'::jsonb -> 'z';
select '{"a": "c", "b": null}'::jsonb -> 'b';
select '"foo"'::jsonb -> 1;
select '"foo"'::jsonb -> 'z';
+select '[]'::jsonb -> -2147483648;
+select jsonb_delete_path('{"a":[]}', '{"a",-2147483648}');
select '{"a": [{"b": "c"}, {"b": "cc"}]}'::jsonb ->> null::text;
select '{"a": [{"b": "c"}, {"b": "cc"}]}'::jsonb ->> null::int;
--
2.39.3 (Apple Git-146)
view thread (85+ messages) latest in thread
Message-ID: <Zr50ZpSyrBx0EHfR@nathan>
Permalink: ../Zr50ZpSyrBx0EHfR@nathan/
Also on: postgresql.org/message-id/Zr50ZpSyrBx0EHfR@nathan
copy link · copy postgr.es · copy git am
reply Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, jian.universality@gmail.com, koshy44@gmail.com, hlinnaka@iki.fi, matthewkmkim@gmail.com, exclusion@gmail.com, tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org, andres@anarazel.de
Subject: Re: Remove dependence on integer wrapping
In-Reply-To: <Zr50ZpSyrBx0EHfR@nathan>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox