public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v4 2/2] New command ALTER TABLE ALL IN TABLESPACE SET LOGGED/UNLOGGED 5+ messages / 4 participants [nested] [flat]
* [PATCH v4 2/2] New command ALTER TABLE ALL IN TABLESPACE SET LOGGED/UNLOGGED @ 2020-11-11 14:21 Kyotaro Horiguchi <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Kyotaro Horiguchi @ 2020-11-11 14:21 UTC (permalink / raw) To ease invoking ALTER TABLE SET LOGGED/UNLOGGED, this command changes relation persistence of all tables in the specified tablespace. --- src/backend/commands/tablecmds.c | 140 +++++++++++++++++++++++++++++++ src/backend/nodes/copyfuncs.c | 16 ++++ src/backend/nodes/equalfuncs.c | 15 ++++ src/backend/parser/gram.y | 20 +++++ src/backend/tcop/utility.c | 11 +++ src/include/commands/tablecmds.h | 2 + src/include/nodes/nodes.h | 1 + src/include/nodes/parsenodes.h | 9 ++ 8 files changed, 214 insertions(+) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 37a15d31ee..2f65abb19b 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -13696,6 +13696,146 @@ AlterTableMoveAll(AlterTableMoveAllStmt *stmt) return new_tablespaceoid; } +/* + * Alter Table ALL ... SET LOGGED/UNLOGGED + * + * Allows a user to change persistence of all objects in a given tablespace in + * the current database. Objects can be chosen based on the owner of the + * object also, to allow users to change persistene only their objects. The + * main permissions handling is done by the lower-level change persistence + * function. + * + * All to-be-modified objects are locked first. If NOWAIT is specified and the + * lock can't be acquired then we ereport(ERROR). + */ +void +AlterTableSetLoggedAll(AlterTableSetLoggedAllStmt *stmt) +{ + List *relations = NIL; + ListCell *l; + ScanKeyData key[1]; + Relation rel; + TableScanDesc scan; + HeapTuple tuple; + Oid tablespaceoid; + List *role_oids = roleSpecsToIds(NIL); + + /* Ensure we were not asked to change something we can't */ + if (stmt->objtype != OBJECT_TABLE) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("only tables can be specified"))); + + /* Get the tablespace OID */ + tablespaceoid = get_tablespace_oid(stmt->tablespacename, false); + + /* + * Now that the checks are done, check if we should set either to + * InvalidOid because it is our database's default tablespace. + */ + if (tablespaceoid == MyDatabaseTableSpace) + tablespaceoid = InvalidOid; + + /* + * Walk the list of objects in the tablespace to pick up them. This will + * only find objects in our database, of course. + */ + ScanKeyInit(&key[0], + Anum_pg_class_reltablespace, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(tablespaceoid)); + + rel = table_open(RelationRelationId, AccessShareLock); + scan = table_beginscan_catalog(rel, 1, key); + while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL) + { + Form_pg_class relForm = (Form_pg_class) GETSTRUCT(tuple); + Oid relOid = relForm->oid; + + /* + * Do not pick-up objects in pg_catalog as part of this, if an admin + * really wishes to do so, they can issue the individual ALTER + * commands directly. + * + * Also, explicitly avoid any shared tables, temp tables, or TOAST + * (TOAST will be changed with the main table). + */ + if (IsCatalogNamespace(relForm->relnamespace) || + relForm->relisshared || + isAnyTempNamespace(relForm->relnamespace) || + IsToastNamespace(relForm->relnamespace)) + continue; + + /* Only pick up the object type requested */ + if (relForm->relkind != RELKIND_RELATION) + continue; + + /* Check if we are only picking-up objects owned by certain roles */ + if (role_oids != NIL && !list_member_oid(role_oids, relForm->relowner)) + continue; + + /* + * Handle permissions-checking here since we are locking the tables + * and also to avoid doing a bunch of work only to fail part-way. Note + * that permissions will also be checked by AlterTableInternal(). + * + * Caller must be considered an owner on the table of which we're going + * to change persistence. + */ + if (!pg_class_ownercheck(relOid, GetUserId())) + aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(get_rel_relkind(relOid)), + NameStr(relForm->relname)); + + if (stmt->nowait && + !ConditionalLockRelationOid(relOid, AccessExclusiveLock)) + ereport(ERROR, + (errcode(ERRCODE_OBJECT_IN_USE), + errmsg("aborting because lock on relation \"%s.%s\" is not available", + get_namespace_name(relForm->relnamespace), + NameStr(relForm->relname)))); + else + LockRelationOid(relOid, AccessExclusiveLock); + + /* + * Add to our list of objects of which we're going to change + * persistence. + */ + relations = lappend_oid(relations, relOid); + } + + table_endscan(scan); + table_close(rel, AccessShareLock); + + if (relations == NIL) + ereport(NOTICE, + (errcode(ERRCODE_NO_DATA_FOUND), + errmsg("no matching relations in tablespace \"%s\" found", + tablespaceoid == InvalidOid ? "(database default)" : + get_tablespace_name(tablespaceoid)))); + + /* + * Everything is locked, loop through and change persistence of all of the + * relations. + */ + foreach(l, relations) + { + List *cmds = NIL; + AlterTableCmd *cmd = makeNode(AlterTableCmd); + + if (stmt->logged) + cmd->subtype = AT_SetLogged; + else + cmd->subtype = AT_SetUnLogged; + + cmds = lappend(cmds, cmd); + + EventTriggerAlterTableStart((Node *) stmt); + /* OID is set by AlterTableInternal */ + AlterTableInternal(lfirst_oid(l), cmds, false); + EventTriggerAlterTableEnd(); + } +} + static void index_copy_data(Relation rel, RelFileNode newrnode) { diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index ba3ccc712c..127da5151d 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -4138,6 +4138,19 @@ _copyAlterTableMoveAllStmt(const AlterTableMoveAllStmt *from) return newnode; } +static AlterTableSetLoggedAllStmt * +_copyAlterTableSetLoggedAllStmt(const AlterTableSetLoggedAllStmt *from) +{ + AlterTableSetLoggedAllStmt *newnode = makeNode(AlterTableSetLoggedAllStmt); + + COPY_STRING_FIELD(tablespacename); + COPY_SCALAR_FIELD(objtype); + COPY_SCALAR_FIELD(logged); + COPY_SCALAR_FIELD(nowait); + + return newnode; +} + static CreateExtensionStmt * _copyCreateExtensionStmt(const CreateExtensionStmt *from) { @@ -5441,6 +5454,9 @@ copyObjectImpl(const void *from) case T_AlterTableMoveAllStmt: retval = _copyAlterTableMoveAllStmt(from); break; + case T_AlterTableSetLoggedAllStmt: + retval = _copyAlterTableSetLoggedAllStmt(from); + break; case T_CreateExtensionStmt: retval = _copyCreateExtensionStmt(from); break; diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c index a2ef853dc2..4f13a1762b 100644 --- a/src/backend/nodes/equalfuncs.c +++ b/src/backend/nodes/equalfuncs.c @@ -1872,6 +1872,18 @@ _equalAlterTableMoveAllStmt(const AlterTableMoveAllStmt *a, return true; } +static bool +_equalAlterTableSetLoggedAllStmt(const AlterTableSetLoggedAllStmt *a, + const AlterTableSetLoggedAllStmt *b) +{ + COMPARE_STRING_FIELD(tablespacename); + COMPARE_SCALAR_FIELD(objtype); + COMPARE_SCALAR_FIELD(logged); + COMPARE_SCALAR_FIELD(nowait); + + return true; +} + static bool _equalCreateExtensionStmt(const CreateExtensionStmt *a, const CreateExtensionStmt *b) { @@ -3494,6 +3506,9 @@ equal(const void *a, const void *b) case T_AlterTableMoveAllStmt: retval = _equalAlterTableMoveAllStmt(a, b); break; + case T_AlterTableSetLoggedAllStmt: + retval = _equalAlterTableSetLoggedAllStmt(a, b); + break; case T_CreateExtensionStmt: retval = _equalCreateExtensionStmt(a, b); break; diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 31c95443a5..2222fd8fe3 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -1934,6 +1934,26 @@ AlterTableStmt: n->nowait = $13; $$ = (Node *)n; } + | ALTER TABLE ALL IN_P TABLESPACE name SET LOGGED opt_nowait + { + AlterTableSetLoggedAllStmt *n = + makeNode(AlterTableSetLoggedAllStmt); + n->tablespacename = $6; + n->objtype = OBJECT_TABLE; + n->logged = true; + n->nowait = $9; + $$ = (Node *)n; + } + | ALTER TABLE ALL IN_P TABLESPACE name SET UNLOGGED opt_nowait + { + AlterTableSetLoggedAllStmt *n = + makeNode(AlterTableSetLoggedAllStmt); + n->tablespacename = $6; + n->objtype = OBJECT_TABLE; + n->logged = false; + n->nowait = $9; + $$ = (Node *)n; + } | ALTER INDEX qualified_name alter_table_cmds { AlterTableStmt *n = makeNode(AlterTableStmt); diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index 53a511f1da..16606448bf 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -161,6 +161,7 @@ ClassifyUtilityCommandAsReadOnly(Node *parsetree) case T_AlterTSConfigurationStmt: case T_AlterTSDictionaryStmt: case T_AlterTableMoveAllStmt: + case T_AlterTableSetLoggedAllStmt: case T_AlterTableSpaceOptionsStmt: case T_AlterTableStmt: case T_AlterTypeStmt: @@ -1732,6 +1733,12 @@ ProcessUtilitySlow(ParseState *pstate, commandCollected = true; break; + case T_AlterTableSetLoggedAllStmt: + AlterTableSetLoggedAll((AlterTableSetLoggedAllStmt *) parsetree); + /* commands are stashed in AlterTableSetLoggedAll */ + commandCollected = true; + break; + case T_DropStmt: ExecDropStmt((DropStmt *) parsetree, isTopLevel); /* no commands stashed for DROP */ @@ -2619,6 +2626,10 @@ CreateCommandTag(Node *parsetree) tag = AlterObjectTypeCommandTag(((AlterTableMoveAllStmt *) parsetree)->objtype); break; + case T_AlterTableSetLoggedAllStmt: + tag = AlterObjectTypeCommandTag(((AlterTableSetLoggedAllStmt *) parsetree)->objtype); + break; + case T_AlterTableStmt: tag = AlterObjectTypeCommandTag(((AlterTableStmt *) parsetree)->objtype); break; diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h index 08c463d3c4..646928466d 100644 --- a/src/include/commands/tablecmds.h +++ b/src/include/commands/tablecmds.h @@ -42,6 +42,8 @@ extern void AlterTableInternal(Oid relid, List *cmds, bool recurse); extern Oid AlterTableMoveAll(AlterTableMoveAllStmt *stmt); +extern void AlterTableSetLoggedAll(AlterTableSetLoggedAllStmt *stmt); + extern ObjectAddress AlterTableNamespace(AlterObjectSchemaStmt *stmt, Oid *oldschema); diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h index caed683ba9..16d91d3e1d 100644 --- a/src/include/nodes/nodes.h +++ b/src/include/nodes/nodes.h @@ -424,6 +424,7 @@ typedef enum NodeTag T_AlterCollationStmt, T_CallStmt, T_AlterStatsStmt, + T_AlterTableSetLoggedAllStmt, /* * TAGS FOR PARSE TREE NODES (parsenodes.h) diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index dc2bb40926..c3eab6f1ab 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -2253,6 +2253,15 @@ typedef struct AlterTableMoveAllStmt bool nowait; } AlterTableMoveAllStmt; +typedef struct AlterTableSetLoggedAllStmt +{ + NodeTag type; + char *tablespacename; + ObjectType objtype; /* Object type to move */ + bool logged; + bool nowait; +} AlterTableSetLoggedAllStmt; + /* ---------------------- * Create/Alter Extension Statements * ---------------------- -- 2.27.0 ----Next_Part(Tue_Jan_12_18_58_08_2021_481)---- ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: json_query conditional wrapper bug @ 2024-09-04 10:16 Peter Eisentraut <[email protected]> 0 siblings, 2 replies; 5+ messages in thread From: Peter Eisentraut @ 2024-09-04 10:16 UTC (permalink / raw) To: pgsql-hackers On 28.08.24 11:21, Peter Eisentraut wrote: > These are ok: > > select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' without wrapper); > json_query > ------------ > 42 > > select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' with > unconditional wrapper); > json_query > ------------ > [42] > > But this appears to be wrong: > > select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' with conditional > wrapper); > json_query > ------------ > [42] > > This should return an unwrapped 42. If I make the code change illustrated in the attached patch, then I get the correct result here. And various regression test results change, which, to me, all look more correct after this patch. I don't know what the code I removed was supposed to accomplish, but it seems to be wrong somehow. In the current implementation, the WITH CONDITIONAL WRAPPER clause doesn't appear to work correctly in any case I could identify. From ea8a36584abf6cea0a05ac43ad8d101012b14313 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <[email protected]> Date: Wed, 4 Sep 2024 12:09:35 +0200 Subject: [PATCH] WIP: Fix JSON_QUERY WITH CONDITIONAL WRAPPER --- src/backend/utils/adt/jsonpath_exec.c | 5 +---- src/test/regress/sql/sqljson_queryfuncs.sql | 10 +++++----- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c index e3ee0093d4d..a55a34685f0 100644 --- a/src/backend/utils/adt/jsonpath_exec.c +++ b/src/backend/utils/adt/jsonpath_exec.c @@ -3957,10 +3957,7 @@ JsonPathQuery(Datum jb, JsonPath *jp, JsonWrapper wrapper, bool *empty, else if (wrapper == JSW_UNCONDITIONAL) wrap = true; else if (wrapper == JSW_CONDITIONAL) - wrap = count > 1 || - IsAJsonbScalar(singleton) || - (singleton->type == jbvBinary && - JsonContainerIsScalar(singleton->val.binary.data)); + wrap = count > 1; else { elog(ERROR, "unrecognized json wrapper %d", (int) wrapper); diff --git a/src/test/regress/sql/sqljson_queryfuncs.sql b/src/test/regress/sql/sqljson_queryfuncs.sql index 21ff7787a28..a7342416a9c 100644 --- a/src/test/regress/sql/sqljson_queryfuncs.sql +++ b/src/test/regress/sql/sqljson_queryfuncs.sql @@ -146,11 +146,11 @@ CREATE DOMAIN rgb AS rainbow CHECK (VALUE IN ('red', 'green', 'blue')); SELECT JSON_VALUE(NULL::jsonb, '$'); SELECT - JSON_QUERY(js, '$'), - JSON_QUERY(js, '$' WITHOUT WRAPPER), - JSON_QUERY(js, '$' WITH CONDITIONAL WRAPPER), - JSON_QUERY(js, '$' WITH UNCONDITIONAL ARRAY WRAPPER), - JSON_QUERY(js, '$' WITH ARRAY WRAPPER) + JSON_QUERY(js, '$') AS "unspec", + JSON_QUERY(js, '$' WITHOUT WRAPPER) AS "without", + JSON_QUERY(js, '$' WITH CONDITIONAL WRAPPER) AS "with cond", + JSON_QUERY(js, '$' WITH UNCONDITIONAL ARRAY WRAPPER) AS "with uncond", + JSON_QUERY(js, '$' WITH ARRAY WRAPPER) AS "with" FROM (VALUES (jsonb 'null'), -- 2.46.0 Attachments: [text/plain] 0001-WIP-Fix-JSON_QUERY-WITH-CONDITIONAL-WRAPPER.patch (1.9K, ../../[email protected]/2-0001-WIP-Fix-JSON_QUERY-WITH-CONDITIONAL-WRAPPER.patch) download | inline diff: From ea8a36584abf6cea0a05ac43ad8d101012b14313 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <[email protected]> Date: Wed, 4 Sep 2024 12:09:35 +0200 Subject: [PATCH] WIP: Fix JSON_QUERY WITH CONDITIONAL WRAPPER --- src/backend/utils/adt/jsonpath_exec.c | 5 +---- src/test/regress/sql/sqljson_queryfuncs.sql | 10 +++++----- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c index e3ee0093d4d..a55a34685f0 100644 --- a/src/backend/utils/adt/jsonpath_exec.c +++ b/src/backend/utils/adt/jsonpath_exec.c @@ -3957,10 +3957,7 @@ JsonPathQuery(Datum jb, JsonPath *jp, JsonWrapper wrapper, bool *empty, else if (wrapper == JSW_UNCONDITIONAL) wrap = true; else if (wrapper == JSW_CONDITIONAL) - wrap = count > 1 || - IsAJsonbScalar(singleton) || - (singleton->type == jbvBinary && - JsonContainerIsScalar(singleton->val.binary.data)); + wrap = count > 1; else { elog(ERROR, "unrecognized json wrapper %d", (int) wrapper); diff --git a/src/test/regress/sql/sqljson_queryfuncs.sql b/src/test/regress/sql/sqljson_queryfuncs.sql index 21ff7787a28..a7342416a9c 100644 --- a/src/test/regress/sql/sqljson_queryfuncs.sql +++ b/src/test/regress/sql/sqljson_queryfuncs.sql @@ -146,11 +146,11 @@ CREATE DOMAIN rgb AS rainbow CHECK (VALUE IN ('red', 'green', 'blue')); SELECT JSON_VALUE(NULL::jsonb, '$'); SELECT - JSON_QUERY(js, '$'), - JSON_QUERY(js, '$' WITHOUT WRAPPER), - JSON_QUERY(js, '$' WITH CONDITIONAL WRAPPER), - JSON_QUERY(js, '$' WITH UNCONDITIONAL ARRAY WRAPPER), - JSON_QUERY(js, '$' WITH ARRAY WRAPPER) + JSON_QUERY(js, '$') AS "unspec", + JSON_QUERY(js, '$' WITHOUT WRAPPER) AS "without", + JSON_QUERY(js, '$' WITH CONDITIONAL WRAPPER) AS "with cond", + JSON_QUERY(js, '$' WITH UNCONDITIONAL ARRAY WRAPPER) AS "with uncond", + JSON_QUERY(js, '$' WITH ARRAY WRAPPER) AS "with" FROM (VALUES (jsonb 'null'), -- 2.46.0 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: json_query conditional wrapper bug @ 2024-09-04 20:10 Andrew Dunstan <[email protected]> parent: Peter Eisentraut <[email protected]> 1 sibling, 0 replies; 5+ messages in thread From: Andrew Dunstan @ 2024-09-04 20:10 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; pgsql-hackers On 2024-09-04 We 6:16 AM, Peter Eisentraut wrote: > On 28.08.24 11:21, Peter Eisentraut wrote: >> These are ok: >> >> select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' without >> wrapper); >> json_query >> ------------ >> 42 >> >> select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' with >> unconditional wrapper); >> json_query >> ------------ >> [42] >> >> But this appears to be wrong: >> >> select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' with >> conditional wrapper); >> json_query >> ------------ >> [42] >> >> This should return an unwrapped 42. > > If I make the code change illustrated in the attached patch, then I > get the correct result here. And various regression test results > change, which, to me, all look more correct after this patch. I don't > know what the code I removed was supposed to accomplish, but it seems > to be wrong somehow. In the current implementation, the WITH > CONDITIONAL WRAPPER clause doesn't appear to work correctly in any > case I could identify. Agree the code definitely looks wrong. If anything the test should probably be reversed: wrap = count > 1 || !( IsAJsonbScalar(singleton) || (singleton->type == jbvBinary && JsonContainerIsScalar(singleton->val.binary.data))); i.e. in the count = 1 case wrap unless it's a scalar or a binary wrapping a scalar. The code could do with a comment about the logic. I know we're very close to release but we should fix this as it's a new feature. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: json_query conditional wrapper bug @ 2024-09-10 08:00 Amit Langote <[email protected]> parent: Peter Eisentraut <[email protected]> 1 sibling, 1 reply; 5+ messages in thread From: Amit Langote @ 2024-09-10 08:00 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: pgsql-hackers Sorry for missing this report and thanks Andrew for the offlist heads up. On Wed, Sep 4, 2024 at 7:16 PM Peter Eisentraut <[email protected]> wrote: > On 28.08.24 11:21, Peter Eisentraut wrote: > > These are ok: > > > > select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' without wrapper); > > json_query > > ------------ > > 42 > > > > select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' with > > unconditional wrapper); > > json_query > > ------------ > > [42] > > > > But this appears to be wrong: > > > > select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' with conditional > > wrapper); > > json_query > > ------------ > > [42] > > > > This should return an unwrapped 42. > > If I make the code change illustrated in the attached patch, then I get > the correct result here. And various regression test results change, > which, to me, all look more correct after this patch. I don't know what > the code I removed was supposed to accomplish, but it seems to be wrong > somehow. In the current implementation, the WITH CONDITIONAL WRAPPER > clause doesn't appear to work correctly in any case I could identify. Agreed that this looks wrong. I've wondered why the condition was like that but left it as-is, because I thought at one point that that's needed to ensure that the returned single scalar SQL/JSON item is valid jsonb. I've updated your patch to include updated test outputs and a nearby code comment expanded. Do you intend to commit it or do you prefer that I do? -- Thanks, Amit Langote Attachments: [application/octet-stream] v2-0001-WIP-Fix-JSON_QUERY-WITH-CONDITIONAL-WRAPPER.patch (8.1K, ../../CA+HiwqFARj1L-HPa+ESbK3ZvUTYt0MpFdrE2vjVjhF2oxXSK-g@mail.gmail.com/2-v2-0001-WIP-Fix-JSON_QUERY-WITH-CONDITIONAL-WRAPPER.patch) download | inline diff: From 106f39fded2bcdfd9a7ff47bb56f35c806b94d24 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <[email protected]> Date: Wed, 4 Sep 2024 12:09:35 +0200 Subject: [PATCH v2] WIP: Fix JSON_QUERY WITH CONDITIONAL WRAPPER --- src/backend/utils/adt/jsonpath_exec.c | 24 ++++++++--- .../regress/expected/sqljson_queryfuncs.out | 42 +++++++++---------- src/test/regress/sql/sqljson_queryfuncs.sql | 10 ++--- 3 files changed, 45 insertions(+), 31 deletions(-) diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c index e3ee0093d4..e569c7efb8 100644 --- a/src/backend/utils/adt/jsonpath_exec.c +++ b/src/backend/utils/adt/jsonpath_exec.c @@ -3947,7 +3947,24 @@ JsonPathQuery(Datum jb, JsonPath *jp, JsonWrapper wrapper, bool *empty, return (Datum) 0; } - /* WRAP or not? */ + /* + * Determine whether to wrap the result in a JSON array or not. + * + * First, count the number of SQL/JSON items in the returned + * JsonValueList. If the list is empty (singleton == NULL), no wrapping is + * necessary. + * + * If the wrapper mode is JSW_NONE or JSW_UNSPEC, wrapping is explicitly + * disabled. This enforces a WITHOUT WRAPPER clause, which is also the + * default when no WRAPPER clause is specified. + * + * If the mode is JSW_UNCONDITIONAL, wrapping is enforced regardless of + * the number of SQL/JSON items, enforcing a WITH WRAPPER or WITH + * UNCONDITIONAL WRAPPER clause. + * + * For JSW_CONDITIONAL, wrapping occurs only if there is more than one + * SQL/JSON item in the list, enforcing a WITH CONDITIONAL WRAPPER clause. + */ count = JsonValueListLength(&found); singleton = count > 0 ? JsonValueListHead(&found) : NULL; if (singleton == NULL) @@ -3957,10 +3974,7 @@ JsonPathQuery(Datum jb, JsonPath *jp, JsonWrapper wrapper, bool *empty, else if (wrapper == JSW_UNCONDITIONAL) wrap = true; else if (wrapper == JSW_CONDITIONAL) - wrap = count > 1 || - IsAJsonbScalar(singleton) || - (singleton->type == jbvBinary && - JsonContainerIsScalar(singleton->val.binary.data)); + wrap = count > 1; else { elog(ERROR, "unrecognized json wrapper %d", (int) wrapper); diff --git a/src/test/regress/expected/sqljson_queryfuncs.out b/src/test/regress/expected/sqljson_queryfuncs.out index 73d7d2117e..378cd9f4cc 100644 --- a/src/test/regress/expected/sqljson_queryfuncs.out +++ b/src/test/regress/expected/sqljson_queryfuncs.out @@ -541,11 +541,11 @@ SELECT JSON_VALUE(NULL::jsonb, '$'); (1 row) SELECT - JSON_QUERY(js, '$'), - JSON_QUERY(js, '$' WITHOUT WRAPPER), - JSON_QUERY(js, '$' WITH CONDITIONAL WRAPPER), - JSON_QUERY(js, '$' WITH UNCONDITIONAL ARRAY WRAPPER), - JSON_QUERY(js, '$' WITH ARRAY WRAPPER) + JSON_QUERY(js, '$') AS "unspec", + JSON_QUERY(js, '$' WITHOUT WRAPPER) AS "without", + JSON_QUERY(js, '$' WITH CONDITIONAL WRAPPER) AS "with cond", + JSON_QUERY(js, '$' WITH UNCONDITIONAL ARRAY WRAPPER) AS "with uncond", + JSON_QUERY(js, '$' WITH ARRAY WRAPPER) AS "with" FROM (VALUES (jsonb 'null'), @@ -555,12 +555,12 @@ FROM ('[1, null, "2"]'), ('{"a": 1, "b": [2]}') ) foo(js); - json_query | json_query | json_query | json_query | json_query + unspec | without | with cond | with uncond | with --------------------+--------------------+--------------------+----------------------+---------------------- - null | null | [null] | [null] | [null] - 12.3 | 12.3 | [12.3] | [12.3] | [12.3] - true | true | [true] | [true] | [true] - "aaa" | "aaa" | ["aaa"] | ["aaa"] | ["aaa"] + null | null | null | [null] | [null] + 12.3 | 12.3 | 12.3 | [12.3] | [12.3] + true | true | true | [true] | [true] + "aaa" | "aaa" | "aaa" | ["aaa"] | ["aaa"] [1, null, "2"] | [1, null, "2"] | [1, null, "2"] | [[1, null, "2"]] | [[1, null, "2"]] {"a": 1, "b": [2]} | {"a": 1, "b": [2]} | {"a": 1, "b": [2]} | [{"a": 1, "b": [2]}] | [{"a": 1, "b": [2]}] (6 rows) @@ -587,10 +587,10 @@ FROM --------------------+--------------------+---------------------+----------------------+---------------------- | | | | | | | | - null | null | [null] | [null] | [null] - 12.3 | 12.3 | [12.3] | [12.3] | [12.3] - true | true | [true] | [true] | [true] - "aaa" | "aaa" | ["aaa"] | ["aaa"] | ["aaa"] + null | null | null | [null] | [null] + 12.3 | 12.3 | 12.3 | [12.3] | [12.3] + true | true | true | [true] | [true] + "aaa" | "aaa" | "aaa" | ["aaa"] | ["aaa"] [1, 2, 3] | [1, 2, 3] | [1, 2, 3] | [[1, 2, 3]] | [[1, 2, 3]] {"a": 1, "b": [2]} | {"a": 1, "b": [2]} | {"a": 1, "b": [2]} | [{"a": 1, "b": [2]}] | [{"a": 1, "b": [2]}] | | [1, "2", null, [3]] | [1, "2", null, [3]] | [1, "2", null, [3]] @@ -681,7 +681,7 @@ LINE 1: SELECT JSON_QUERY(jsonb '[1]', '$' WITH CONDITIONAL WRAPPER ... SELECT JSON_QUERY(jsonb '["1"]', '$[*]' WITH CONDITIONAL WRAPPER KEEP QUOTES); json_query ------------ - ["1"] + "1" (1 row) SELECT JSON_QUERY(jsonb '["1"]', '$[*]' WITH UNCONDITIONAL WRAPPER KEEP QUOTES); @@ -940,30 +940,30 @@ FROM x | y | list ---+---+-------------- 0 | 0 | [] - 0 | 1 | [1] + 0 | 1 | 1 0 | 2 | [1, 2] 0 | 3 | [1, 2, 3] 0 | 4 | [1, 2, 3, 4] 1 | 0 | [] - 1 | 1 | [1] + 1 | 1 | 1 1 | 2 | [1, 2] 1 | 3 | [1, 2, 3] 1 | 4 | [1, 2, 3, 4] 2 | 0 | [] 2 | 1 | [] - 2 | 2 | [2] + 2 | 2 | 2 2 | 3 | [2, 3] 2 | 4 | [2, 3, 4] 3 | 0 | [] 3 | 1 | [] 3 | 2 | [] - 3 | 3 | [3] + 3 | 3 | 3 3 | 4 | [3, 4] 4 | 0 | [] 4 | 1 | [] 4 | 2 | [] 4 | 3 | [] - 4 | 4 | [4] + 4 | 4 | 4 (25 rows) -- record type returning with quotes behavior. @@ -1147,7 +1147,7 @@ INSERT INTO test_jsonb_constraints VALUES ('{"a": 7}', 1); ERROR: new row for relation "test_jsonb_constraints" violates check constraint "test_jsonb_constraint5" DETAIL: Failing row contains ({"a": 7}, 1, [1, 2]). INSERT INTO test_jsonb_constraints VALUES ('{"a": 10}', 1); -ERROR: new row for relation "test_jsonb_constraints" violates check constraint "test_jsonb_constraint4" +ERROR: new row for relation "test_jsonb_constraints" violates check constraint "test_jsonb_constraint5" DETAIL: Failing row contains ({"a": 10}, 1, [1, 2]). DROP TABLE test_jsonb_constraints; -- Test mutabilily of query functions diff --git a/src/test/regress/sql/sqljson_queryfuncs.sql b/src/test/regress/sql/sqljson_queryfuncs.sql index 21ff7787a2..a7342416a9 100644 --- a/src/test/regress/sql/sqljson_queryfuncs.sql +++ b/src/test/regress/sql/sqljson_queryfuncs.sql @@ -146,11 +146,11 @@ select json_value('{"a": "1.234"}', '$.a' returning int error on error); SELECT JSON_VALUE(NULL::jsonb, '$'); SELECT - JSON_QUERY(js, '$'), - JSON_QUERY(js, '$' WITHOUT WRAPPER), - JSON_QUERY(js, '$' WITH CONDITIONAL WRAPPER), - JSON_QUERY(js, '$' WITH UNCONDITIONAL ARRAY WRAPPER), - JSON_QUERY(js, '$' WITH ARRAY WRAPPER) + JSON_QUERY(js, '$') AS "unspec", + JSON_QUERY(js, '$' WITHOUT WRAPPER) AS "without", + JSON_QUERY(js, '$' WITH CONDITIONAL WRAPPER) AS "with cond", + JSON_QUERY(js, '$' WITH UNCONDITIONAL ARRAY WRAPPER) AS "with uncond", + JSON_QUERY(js, '$' WITH ARRAY WRAPPER) AS "with" FROM (VALUES (jsonb 'null'), -- 2.43.0 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: json_query conditional wrapper bug @ 2024-09-10 20:15 Peter Eisentraut <[email protected]> parent: Amit Langote <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Peter Eisentraut @ 2024-09-10 20:15 UTC (permalink / raw) To: Amit Langote <[email protected]>; +Cc: pgsql-hackers On 10.09.24 10:00, Amit Langote wrote: > Sorry for missing this report and thanks Andrew for the offlist heads up. > > On Wed, Sep 4, 2024 at 7:16 PM Peter Eisentraut <[email protected]> wrote: >> On 28.08.24 11:21, Peter Eisentraut wrote: >>> These are ok: >>> >>> select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' without wrapper); >>> json_query >>> ------------ >>> 42 >>> >>> select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' with >>> unconditional wrapper); >>> json_query >>> ------------ >>> [42] >>> >>> But this appears to be wrong: >>> >>> select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' with conditional >>> wrapper); >>> json_query >>> ------------ >>> [42] >>> >>> This should return an unwrapped 42. >> >> If I make the code change illustrated in the attached patch, then I get >> the correct result here. And various regression test results change, >> which, to me, all look more correct after this patch. I don't know what >> the code I removed was supposed to accomplish, but it seems to be wrong >> somehow. In the current implementation, the WITH CONDITIONAL WRAPPER >> clause doesn't appear to work correctly in any case I could identify. > > Agreed that this looks wrong. > > I've wondered why the condition was like that but left it as-is, > because I thought at one point that that's needed to ensure that the > returned single scalar SQL/JSON item is valid jsonb. > > I've updated your patch to include updated test outputs and a nearby > code comment expanded. Do you intend to commit it or do you prefer > that I do? This change looks unrelated: -ERROR: new row for relation "test_jsonb_constraints" violates check constraint "test_jsonb_constraint4" +ERROR: new row for relation "test_jsonb_constraints" violates check constraint "test_jsonb_constraint5" Is this some randomness in the way these constraints are evaluated? ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2024-09-10 20:15 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-11-11 14:21 [PATCH v4 2/2] New command ALTER TABLE ALL IN TABLESPACE SET LOGGED/UNLOGGED Kyotaro Horiguchi <[email protected]> 2024-09-04 10:16 Re: json_query conditional wrapper bug Peter Eisentraut <[email protected]> 2024-09-04 20:10 ` Re: json_query conditional wrapper bug Andrew Dunstan <[email protected]> 2024-09-10 08:00 ` Re: json_query conditional wrapper bug Amit Langote <[email protected]> 2024-09-10 20:15 ` Re: json_query conditional wrapper bug Peter Eisentraut <[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