public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v48 3/3] Replace assuming a composite object on a scalar 3+ messages / 3 participants [nested] [flat]
* [PATCH v48 3/3] Replace assuming a composite object on a scalar @ 2021-01-20 15:53 Dmitrii Dolgov <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Dmitrii Dolgov @ 2021-01-20 15:53 UTC (permalink / raw) For jsonb subscripting assignment it could happen that the provided path assumes an object or an array at some level, but the source jsonb has a scalar value there. Originally the update operation will be skipped and no message will be shown that nothing happened. Return an error to indicate such situations. --- doc/src/sgml/json.sgml | 19 +++++++++++++++++++ src/backend/utils/adt/jsonfuncs.c | 27 +++++++++++++++++++++++++++ src/test/regress/expected/jsonb.out | 27 +++++++++++++++++++++++++++ src/test/regress/sql/jsonb.sql | 17 +++++++++++++++++ 4 files changed, 90 insertions(+) diff --git a/doc/src/sgml/json.sgml b/doc/src/sgml/json.sgml index 07bd19f974..deeb9e66e0 100644 --- a/doc/src/sgml/json.sgml +++ b/doc/src/sgml/json.sgml @@ -614,8 +614,23 @@ SELECT jdoc->'guid', jdoc->'name' FROM api WHERE jdoc @> '{"tags": ["qu The result of a subscripting expression is always of the jsonb data type. </para> + <para> + <command>UPDATE</command> statements may use subscripting in the + <literal>SET</literal> clause to modify <type>jsonb</type> values. Object + values being traversed must exist as specified by the subscript path. For + instance, the path <literal>val['a']['b']['c']</literal> assumes that + <literal>val</literal>, <literal>val['a']</literal>, and <literal>val['a']['b']</literal> + are all objects in every record being updated (<literal>val['a']['b']</literal> + may or may not contain a field named <literal>c</literal>, as long as it's an + object). If any individual <literal>val</literal>, <literal>val['a']</literal>, + or <literal>val['a']['b']</literal> is a non-object such as a string, a number, + or <literal>NULL</literal>, an error is raised even if other values do conform. + Array values are not subject to this restriction, as detailed below. + </para> + <para> An example of subscripting syntax: + <programlisting> -- Extract object value by key @@ -631,6 +646,10 @@ SELECT ('[1, "2", null]'::jsonb)[1]; -- value must be of the jsonb type as well UPDATE table_name SET jsonb_field['key'] = '1'; +-- This will raise an error if any record's jsonb_field['a']['b'] is something +-- other than an object. For example, the value {"a": 1} has no 'b' key. +UPDATE table_name SET jsonb_field['a']['b']['c'] = '1'; + -- Filter records using a WHERE clause with subscripting. Since the result of -- subscripting is jsonb, the value we compare it against must also be jsonb. -- The double quotes make "value" also a valid jsonb string. diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c index f14f6c3191..9138b7950e 100644 --- a/src/backend/utils/adt/jsonfuncs.c +++ b/src/backend/utils/adt/jsonfuncs.c @@ -4926,6 +4926,20 @@ setPath(JsonbIterator **it, Datum *path_elems, switch (r) { case WJB_BEGIN_ARRAY: + /* + * If instructed complain about attempts to replace whithin a raw + * scalar value. This happens even when current level is equal to + * path_len, because the last path key should also correspond to an + * object or an array, not raw scalar. + */ + if ((op_type & JB_PATH_FILL_GAPS) && (level <= path_len - 1) && + v.val.array.rawScalar) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("cannot replace existing key"), + errdetail("The path assumes key is a composite object, " + "but it is a scalar value."))); + (void) pushJsonbValue(st, r, NULL); setPathArray(it, path_elems, path_nulls, path_len, st, level, newval, v.val.array.nElems, op_type); @@ -4943,6 +4957,19 @@ setPath(JsonbIterator **it, Datum *path_elems, break; case WJB_ELEM: case WJB_VALUE: + /* + * If instructed complain about attempts to replace whithin a + * scalar value. This happens even when current level is equal to + * path_len, because the last path key should also correspond to an + * object or an array, not an element or value. + */ + if ((op_type & JB_PATH_FILL_GAPS) && (level <= path_len - 1)) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("cannot replace existing key"), + errdetail("The path assumes key is a composite object, " + "but it is a scalar value."))); + res = pushJsonbValue(st, r, &v); break; default: diff --git a/src/test/regress/expected/jsonb.out b/src/test/regress/expected/jsonb.out index 5b5510c4fd..cf0ce0e44c 100644 --- a/src/test/regress/expected/jsonb.out +++ b/src/test/regress/expected/jsonb.out @@ -5134,6 +5134,33 @@ select * from test_jsonb_subscript; 1 | {"a": [null, {"c": [null, null, 1]}]} (1 row) +-- trying replace assuming a composite object, but it's an element or a value +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '{"a": 1}'); +update test_jsonb_subscript set test_json['a']['b'] = '1'; +ERROR: cannot replace existing key +DETAIL: The path assumes key is a composite object, but it is a scalar value. +update test_jsonb_subscript set test_json['a']['b']['c'] = '1'; +ERROR: cannot replace existing key +DETAIL: The path assumes key is a composite object, but it is a scalar value. +update test_jsonb_subscript set test_json['a'][0] = '1'; +ERROR: cannot replace existing key +DETAIL: The path assumes key is a composite object, but it is a scalar value. +update test_jsonb_subscript set test_json['a'][0]['c'] = '1'; +ERROR: cannot replace existing key +DETAIL: The path assumes key is a composite object, but it is a scalar value. +update test_jsonb_subscript set test_json['a'][0][0] = '1'; +ERROR: cannot replace existing key +DETAIL: The path assumes key is a composite object, but it is a scalar value. +-- trying replace assuming a composite object, but it's a raw scalar +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, 'null'); +update test_jsonb_subscript set test_json[0] = '1'; +ERROR: cannot replace existing key +DETAIL: The path assumes key is a composite object, but it is a scalar value. +update test_jsonb_subscript set test_json[0][0] = '1'; +ERROR: cannot replace existing key +DETAIL: The path assumes key is a composite object, but it is a scalar value. -- jsonb to tsvector select to_tsvector('{"a": "aaa bbb ddd ccc", "b": ["eee fff ggg"], "c": {"d": "hhh iii"}}'::jsonb); to_tsvector diff --git a/src/test/regress/sql/jsonb.sql b/src/test/regress/sql/jsonb.sql index 0320db0ea4..1a9d21741f 100644 --- a/src/test/regress/sql/jsonb.sql +++ b/src/test/regress/sql/jsonb.sql @@ -1371,6 +1371,23 @@ insert into test_jsonb_subscript values (1, '{"a": []}'); update test_jsonb_subscript set test_json['a'][1]['c'][2] = '1'; select * from test_jsonb_subscript; +-- trying replace assuming a composite object, but it's an element or a value + +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, '{"a": 1}'); +update test_jsonb_subscript set test_json['a']['b'] = '1'; +update test_jsonb_subscript set test_json['a']['b']['c'] = '1'; +update test_jsonb_subscript set test_json['a'][0] = '1'; +update test_jsonb_subscript set test_json['a'][0]['c'] = '1'; +update test_jsonb_subscript set test_json['a'][0][0] = '1'; + +-- trying replace assuming a composite object, but it's a raw scalar + +delete from test_jsonb_subscript; +insert into test_jsonb_subscript values (1, 'null'); +update test_jsonb_subscript set test_json[0] = '1'; +update test_jsonb_subscript set test_json[0][0] = '1'; + -- jsonb to tsvector select to_tsvector('{"a": "aaa bbb ddd ccc", "b": ["eee fff ggg"], "c": {"d": "hhh iii"}}'::jsonb); -- 2.21.0 --m4t3wpskod5t4yye-- ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: wal_compression=zstd @ 2022-03-04 07:19 Michael Paquier <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Michael Paquier @ 2022-03-04 07:19 UTC (permalink / raw) To: Justin Pryzby <[email protected]>; +Cc: Andrey Borodin <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; pgsql-hackers; Thomas Munro <[email protected]>; Peter Geoghegan <[email protected]>; Andres Freund <[email protected]>; Robert Haas <[email protected]> On Tue, Feb 22, 2022 at 05:19:48PM -0600, Justin Pryzby wrote: > I am not claiming that zstd is generally better for WAL. Rather, if we're > going to support alternate compression methods, it's nice to give a couple > options (in addition to pglz). Some use cases would certainly suffer from > slower WAL. But providing the option will benefit some others. Note that a > superuser can set wal_compression for a given session - this would probably > benefit bulk-loading in an otherwise OLTP environment. Well, I cannot say which one is better either as it depends on your workload, mostly, but we know for sure that both have benefits, so I don't mind adding it now. What you are proposing is built on top of the existing code, making it a very simple change. > As writen, this patch uses zstd level=1 (whereas the ZSTD's default compress > level is 6). Why? ZSTD using this default has its reasons, no? And it would be consistent to do the same for ZSTD as for the other two methods. - The supported methods are <literal>pglz</literal> and - <literal>lz4</literal> (if <productname>PostgreSQL</productname> was - compiled with <option>--with-lz4</option>). The default value is - <literal>off</literal>. Only superusers can change this setting. + The supported methods are <literal>pglz</literal>, and + (if configured when <productname>PostgreSQL</productname>was built) + <literal>lz4</literal> and zstd. + The default value is <literal>off</literal>. + Only superusers can change this setting. This is making the docs less verbose. I think that you should keep the mention to respectively --with-lz4 and --with-zstd after each value. <para> Build with <productname>ZSTD</productname> compression support. + This enables use of <productname>ZSTD</productname> for + compression of WAL data. This addition is not necessary, see d7a9786. Not related to your patch, but we have more reasons to add an check in the block of BKPIMAGE_COMPRESSED() in RestoreBlockImage() of xlogreader.c to make sure that only one bit is set for the compression type. We could use a pg_popcount() == 1 for that, returning report_invalid_record() when we see some corrupted data. As a whole, 0001 looks pretty good to me after a detailed read (not tested, though). > Only 0001 should be reviewed for pg15 - the others are optional/future work. That's wiser for v15. FWIW, I have the impression that we don't need most of what's proposed in 0002~. /* compression methods supported */ -#define BKPIMAGE_COMPRESS_PGLZ 0x04 -#define BKPIMAGE_COMPRESS_LZ4 0x08 -#define BKPIMAGE_COMPRESS_ZSTD 0x10 - +#define BKPIMAGE_COMPRESS_METHOD1 0x04 /* bits to encode compression method */ +#define BKPIMAGE_COMPRESS_METHOD2 0x08 /* 0=none, 1=pglz, 2=lz4, 3=zstd */ As of 0002. We still have some room for this data and this makes the code harder to follow, so I'd live this part of the code as it is proposed in 0001. 0003, defaulting to zstd, and 0004 to extend compression to support a level would require a lot of benchmarking to be justified. I have argued against making the code more complicated for such things in the past, with reloptions. The footprint on the code is much smaller, here, still.. 0007, for ZLIB, does not make sense once one can choose between LZ4 and ZSTD. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../YiG9hOS%[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: wal_compression=zstd @ 2022-03-04 11:44 Justin Pryzby <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Justin Pryzby @ 2022-03-04 11:44 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Andrey Borodin <[email protected]>; Peter Eisentraut <[email protected]>; Heikki Linnakangas <[email protected]>; pgsql-hackers; Thomas Munro <[email protected]>; Peter Geoghegan <[email protected]>; Andres Freund <[email protected]>; Robert Haas <[email protected]> On Fri, Mar 04, 2022 at 04:19:32PM +0900, Michael Paquier wrote: > On Tue, Feb 22, 2022 at 05:19:48PM -0600, Justin Pryzby wrote: > > > As writen, this patch uses zstd level=1 (whereas the ZSTD's default compress > > level is 6). > > Why? ZSTD using this default has its reasons, no? And it would be > consistent to do the same for ZSTD as for the other two methods. In my 1-off test, it gets 610/633 = 96% of the benefit at 209/273 = 77% of the cost. postgres=# SET wal_compression= 'zstd-6'; postgres=# \set QUIET \\ \timing on \\ SET max_parallel_maintenance_workers=0; SELECT pg_stat_reset_shared('wal'); begin; CREATE INDEX ON t(a); rollback; SELECT * FROM pg_stat_wal; Duración: 2729,017 ms (00:02,729) wal_bytes | 6102403 postgres=# SET wal_compression= 'zstd-1'; postgres=# \set QUIET \\ \timing on \\ SET max_parallel_maintenance_workers=0; SELECT pg_stat_reset_shared('wal'); begin; CREATE INDEX ON t(a); rollback; SELECT * FROM pg_stat_wal; Duración: 2090,459 ms (00:02,090) wal_bytes | 6330269 It may be relevant that we're only compressing 8k [0]. It would probably pay off to preserve a compression "dictionary" to be re-used across FPIs. > - The supported methods are <literal>pglz</literal> and > - <literal>lz4</literal> (if <productname>PostgreSQL</productname> was > - compiled with <option>--with-lz4</option>). The default value is > - <literal>off</literal>. Only superusers can change this setting. > + The supported methods are <literal>pglz</literal>, and > + (if configured when <productname>PostgreSQL</productname>was built) > + <literal>lz4</literal> and zstd. > + The default value is <literal>off</literal>. > + Only superusers can change this setting. > > This is making the docs less verbose. I think that you should keep > the mention to respectively --with-lz4 and --with-zstd after each > value. I changed this relative to your latest zstd patch in July since it reads better to me. YMMV. On Tue, Feb 22, 2022 at 05:19:48PM -0600, Justin Pryzby wrote: >> 0001 adds support for zstd >> 0002 makes more efficient our use of bits in the WAL header >> 0003 makes it the default compression, to exercise on CI (windows will fail). >> 0004 adds support for zstd-level >> 0005-6 are needed to allow recovery tests to pass when wal compression is enabled; >> 0007 (not included) also adds support for zlib. I'm of the impression nobody >> cares about this, otherwise it would've been included 5-10 years ago. > 0003, defaulting to zstd, would require a lot of benchmarking to be > justified. Maybe you didn't see that I wrote that it was for CI ? (In any case, it's impossible to do that without first taking care of 005-6). > and 0004 to extend compression to support a level > I have argued against making the code more complicated for such things in the > past, with reloptions. I suppose that you're referring to reloptions for toast compression, which was about controlling LZ4 compression level. https://www.postgresql.org/message-id/flat/CAFiTN-vMHU_yakMgNo90SUih_6LtvmqZ5hpQb2iTgQtVyOjyFA@mail.... I think you're right that it's not interesting to control the compression level of LZ4 - but there's no reason to say the same thing of zstd. We're already debating which is the "right" level to use (1 or 6). I don't think there is a "right" level - some people will want to trade more CPU for disk writes and some people won't. ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2022-03-04 11:44 UTC | newest] Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-01-20 15:53 [PATCH v48 3/3] Replace assuming a composite object on a scalar Dmitrii Dolgov <[email protected]> 2022-03-04 07:19 Re: wal_compression=zstd Michael Paquier <[email protected]> 2022-03-04 11:44 ` Re: wal_compression=zstd Justin Pryzby <[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