Re: Underscore in positional parameters?
On 2024-05-20 05:02 +0200, Tom Lane wrote:
> Erik Wienhold writes:
> > On 2024-05-20 03:26 +0200, jian he wrote:
> >> /* Check parameter number is in range */
> >> if (paramno <= 0 || paramno > MaxAllocSize / sizeof(Oid))
> >> ereport(ERROR, ...
>
> > Yes, it makes sense to show the upper bound. How about a hint such as
> > "Valid parameters range from $%d to $%d."?
>
> I kind of feel like this upper bound is ridiculous. In what scenario
> is parameter 25000 not a mistake, if not indeed somebody trying
> to break the system?
>
> The "Bind" protocol message only allows an int16 parameter count,
> so rejecting parameter numbers above 32K would make sense to me.
Agree. I was already wondering upthread why someone would use that many
parameters.
Out of curiosity, I checked if there might be an even lower limit. And
indeed, max_stack_depth puts a limit due to some recursive evaluation:
ERROR: stack depth limit exceeded
HINT: Increase the configuration parameter "max_stack_depth" (currently
2048kB), after ensuring the platform's stack depth limit is adequate.
Attached is the stacktrace for EXECUTE on HEAD (I snipped most of the
recursive frames).
Running \bind, PREPARE, and EXECUTE with following number of parameters
works as expected, although the number varies between releases which is
not ideal IMO. The commands hit the stack depth limit for #Params+1.
VersionCommand #Params
- --- ---
HEAD (18cbed13d5) \bind4365
HEAD (18cbed13d5) PREPARE 8182
HEAD (18cbed13d5) EXECUTE 4363
16.2 \bind3968
16.2 PREPARE 6889
16.2 EXECUTE 3966
Those are already pretty large numbers in my view (compared to the 100
parameters that we accept at most for functions). And I guess nobody
complained about those limits yet, or they just increased
max_stack_depth.
The Python script to generate the test scripts:
import sys
n_params = 1 << 16
if len(sys.argv) > 1:
n_params = min(n_params, int(sys.argv[1]))
params = '+'.join(f'${i+1}::int' for i in range(n_params))
bind_vals = ' '.join('1' for _ in range(n_params))
exec_vals = ','.join('1' for _ in range(n_params))
print(fr"SELECT {params} \bind {bind_vals} \g")
print(f"PREPARE p AS SELECT {params};")
print(f"EXECUTE p ({exec_vals});")
--
Erik
Breakpoint 1, errfinish (filename=0x6441fddaec2e "postgres.c", lineno=3535,
funcname=0x6441fdef87a0 <__func__.14> "check_stack_depth") at elog.c:479
479 ErrorData *edata = [errordata_stack_depth];
#0 errfinish (filename=0x6441fddaec2e "postgres.c", lineno=3535,
funcname=0x6441fdef87a0 <__func__.14> "check_stack_depth") at elog.c:479
#1 0x6441fd898173 in check_stack_depth () at postgres.c:3535
#2 0x6441fda5a9a2 in ExecInitExprRec (node=node@entry=0x6442373be5b0,
state=state@entry=0x644236db7e90, resv=resv@entry=0x644236db7e98,
resnull=resnull@entry=0x644236db7e95) at execExpr.c:899
#3 0x6441fda5dc12 in ExecInitExpr (node=node@entry=0x6442373be5b0,
parent=parent@entry=0x0) at execExpr.c:153
#4 0x6441fdb52f58 in evaluate_expr (expr=0x6442373be5b0,
result_type=result_type@entry=23, result_typmod=result_typmod@entry=-1,
result_collation=result_collation@entry=0) at clauses.c:4987
#5 0x6441fdb53181 in evaluate_function (func_tuple=0x71b8821811c0,
funcid=177, result_type=23, result_typmod=-1, result_collid=0, input_collid=0,
args=0x6442373be520, funcvariadic=false, context=0x7ffd36b93cc0) at
clauses.c:4503
#6 simplify_function (funcid=177, result_type=23,
result_typmod=result_typmod@entry=-1, result_collid=0, input_collid=0,
args_p=args_p@entry=0x7ffd36994940, funcvariadic=false, process_args=true,
allow_non_const=true, context=0x7ffd36b93cc0) at clauses.c:4092
#7 0x6441fdb54624 in eval_const_expressions_mutator (node=,
context=0x7ffd36b93cc0) at clauses.c:2639
#8 0x6441fdacdf3a in expression_tree_mutator_impl (node=0x644236d87398,
mutator=mutator@entry=0x6441fdb53a10 ,
context=context@entry=0x7ffd36b93cc0) at nodeFuncs.c:3554
#9 0x6441fdb53781 in simplify_function (funcid=177, result_type=23,
result_typmod=result_typmod@entry=-1, result_collid=0, input_collid=0,
args_p=args_p@entry=0x7ffd36994b20, funcvariadic=false, process_args=true,
allow_non_const=true, context=0x7ffd36b93cc0) at clauses.c:4083
#10 0x6441fdb54624 in eval_const_expressions_mutator (node=,
context=0x7ffd36b93cc0) at clauses.c:2639
#11 0x6441fdacdf3a in expression_tree_mutator_impl (node=0x644236d87308,
mutator=mutator@entry=0x6441fdb53a10 ,
context=context@entry=0x7ffd36b93cc0) at nodeFuncs.c:3554
#12 0x6441fdb53781 in simplify_function (funcid=177, result_type=23,
result_typmod=result_typmod@entry=-
Re: Underscore in positional parameters?
On 2024-05-20 03:26 +0200, jian he wrote:
> On Sun, May 19, 2024 at 10:43 PM Erik Wienhold wrote:
> >
> > On 2024-05-19 07:00 +0200, Alexander Lakhin wrote:
> > > I encountered anomalies that you address with this patch too.
> > > And I can confirm that it fixes most cases, but there is another one:
> > > SELECT $3 \bind 'foo' \g
> > > ERROR: invalid memory alloc request size 12
> > >
> > > Maybe you would find this worth fixing as well.
> >
> > Yes, that error message is not great. In variable_paramref_hook we
> > check paramno > INT_MAX/sizeof(Oid) when in fact MaxAllocSize/sizeof(Oid)
> > is the more appropriate limit to avoid that unspecific alloc size error.
> >
> > Fixed in v4 with a separate patch because it's unrelated to the param
> > number parsing. But it fits nicely into the broader issue on the upper
> > limit for param numbers. Note that $268435455 is still the largest
> > possible param number ((2^30-1)/4) and that we just return a more
> > user-friendly error message for params beyond that limit.
> >
>
> hi, one minor issue:
>
> /* Check parameter number is in range */
> if (paramno <= 0 || paramno > MaxAllocSize / sizeof(Oid))
> ereport(ERROR,
> (errcode(ERRCODE_UNDEFINED_PARAMETER),
> errmsg("there is no parameter $%d", paramno),
> parser_errposition(pstate, pref->location)));
>
> if paramno <= 0 then "there is no parameter $%d" makes sense to me.
>
> but, if paramno > 0 why not just say, we can only allow MaxAllocSize /
> sizeof(Oid) number of parameters.
Yes, it makes sense to show the upper bound. How about a hint such as
"Valid parameters range from $%d to $%d."?
--
Erik
Re: Underscore in positional parameters?
On 2024-05-19 07:00 +0200, Alexander Lakhin wrote:
> I encountered anomalies that you address with this patch too.
> And I can confirm that it fixes most cases, but there is another one:
> SELECT $3 \bind 'foo' \g
> ERROR: invalid memory alloc request size 12
>
> Maybe you would find this worth fixing as well.
Yes, that error message is not great. In variable_paramref_hook we
check paramno > INT_MAX/sizeof(Oid) when in fact MaxAllocSize/sizeof(Oid)
is the more appropriate limit to avoid that unspecific alloc size error.
Fixed in v4 with a separate patch because it's unrelated to the param
number parsing. But it fits nicely into the broader issue on the upper
limit for param numbers. Note that $268435455 is still the largest
possible param number ((2^30-1)/4) and that we just return a more
user-friendly error message for params beyond that limit.
--
Erik
>From 5382f725ac1ff99b5830e8ca04613467660afaa2 Mon Sep 17 00:00:00 2001
From: Erik Wienhold
Date: Tue, 14 May 2024 14:12:11 +0200
Subject: [PATCH v4 1/2] Fix overflow in parsing of positional parameter
Replace atol with pg_strtoint32_safe in the backend parser and with
strtoint in ECPG to reject overflows when parsing the number of a
positional parameter. With atol from glibc, parameters $2147483648 and
$4294967297 turn into $-2147483648 and $1, respectively.
---
src/backend/parser/scan.l| 8 +++-
src/interfaces/ecpg/preproc/pgc.l| 5 -
src/test/regress/expected/numerology.out | 4
src/test/regress/sql/numerology.sql | 1 +
4 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/src/backend/parser/scan.l b/src/backend/parser/scan.l
index 9b33fb8d72..436cc64f07 100644
--- a/src/backend/parser/scan.l
+++ b/src/backend/parser/scan.l
@@ -992,8 +992,14 @@ other .
}
{param} {
+ ErrorSaveContext escontext = {T_ErrorSaveContext};
+ int32 val;
+
SET_YYLLOC();
- yylval->ival = atol(yytext + 1);
+ val = pg_strtoint32_safe(yytext + 1, (Node *) );
+ if (escontext.error_occurred)
+ yyerror("parameter number too large");
+ yylval->ival = val;
return PARAM;
}
{param_junk} {
diff --git a/src/interfaces/ecpg/preproc/pgc.l b/src/interfaces/ecpg/preproc/pgc.l
index d117cafce6..7122375d72 100644
--- a/src/interfaces/ecpg/preproc/pgc.l
+++ b/src/interfaces/ecpg/preproc/pgc.l
@@ -938,7 +938,10 @@ cppline {space}*#([^i][A-Za-z]*|{if}|{ifdef}|{ifndef}|{import})((\/\*[^*/]*\*+
}
{param} {
- base_yylval.ival = atol(yytext+1);
+ errno = 0;
+ base_yylval.ival = strtoint(yytext+1, NULL, 10);
+ if (errno == ERANGE)
+ mmfatal(PARSE_ERROR, "parameter number too large");
return PARAM;
}
{param_junk} {
diff --git a/src/test/regress/expected/numerology.out b/src/test/regress/expected/numerology.out
index c8196d2c85..5bac05c3b3 100644
--- a/src/test/regress/expected/numerology.out
+++ b/src/test/regress/expected/numerology.out
@@ -206,6 +206,10 @@ PREPARE p1 AS SELECT $1a;
ERROR: trailing junk after parameter at or near "$1a"
LINE 1: PREPARE p1 AS SELECT $1a;
^
+PREPARE p1 AS SELECT $2147483648;
+ERROR: parameter number too large at or near "$2147483648"
+LINE 1: PREPARE p1 AS SELECT $2147483648;
+ ^
SELECT 0b;
ERROR: invalid binary integer at or near "0b"
LINE 1: SELECT 0b;
diff --git a/src/test/regress/sql/numerology.sql b/src/test/regress/sql/numerology.sql
index 3f0ec34ecf..6722a09c5f 100644
--- a/src/test/regress/sql/numerology.sql
+++ b/src/test/regress/sql/numerology.sql
@@ -52,6 +52,7 @@ SELECT 0.0e1a;
SELECT 0.0e;
SELECT 0.0e+a;
PREPARE p1 AS SELECT $1a;
+PREPARE p1 AS SELECT $2147483648;
SELECT 0b;
SELECT 1b;
--
2.45.1
>From 9ae52b4a1949a0498dc75aba8fdd72927e6bd93d Mon Sep 17 00:00:00 2001
From: Erik Wienhold
Date: Sun, 19 May 2024 15:29:18 +0200
Subject: [PATCH v4 2/2] Limit max parameter number with MaxAllocSize
MaxAllocSize puts an upper bound on the largest possible parameter
number ($268435455). Use that limit instead of MAX_INT to report that
no parameters exist beyond that point instead of reporting an error
about the maximum allocation size being exceeded.
---
src/backend/parser/parse_param.c | 3 ++-
src/test/regress/expected/prepare.out | 5 +
src/test/regress/sql/prepare.sql | 3 +++
3 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/backend/parser/parse_param.c b/src/backend/parser/parse_param.c
index dbf1a7dff0..b617591ef6 100644
--- a/src/backend/parser/parse_param.c
+++ b/src/backend/parser/parse_param.c
@@ -31,6 +31,7 @@
#include "parser/parse_param.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
+#include "utils/memutils.h"
typedef struct FixedParamState
@@ -136,7 +137,7 @@ variable_paramref_hook(ParseState *pstate, ParamRef
Re: CREATE TABLE creates a composite type corresponding to the table row, which is and is not there
On 2024-05-18 03:27 +0200, David G. Johnston wrote:
> > On 2024-05-16 17:47 +0200, David G. Johnston wrote:
> > > We have a glossary.
>
> If sticking with stand-alone composite type as a formal term we should
> document it in the glossary. It's unclear whether this will survive review
> though. With the wording provided in this patch it doesn't really add
> enough to continue a strong defense of it.
Oh, I thought you meant we already have that term in the glossary (I
haven't checked until now). Let's see if we can convince Robert of the
rewording.
> > It's now a separate error message (like I already had in v1) which
> > states that the specified type must not be a row type of another table
> > (based on Peter's feedback). And the hint directs the user to CREATE
> > TYPE.
> >
> > In passing, I also quoted the type name in the existing error message
> > for consistency. I saw that table names etc. are already quoted in
> > other error messages.
> >
> >
> The hint and the quoting both violate the documented rules for these things:
>
> https://www.postgresql.org/docs/current/error-style-guide.html#ERROR-STYLE-GUIDE-QUOTES
>
> There are functions in the backend that will double-quote their own output
> as needed (for example, format_type_be()). Do not put additional quotes
> around the output of such functions.
>
> https://www.postgresql.org/docs/current/error-style-guide.html#ERROR-STYLE-GUIDE-GRAMMAR-PUNCTUATION
>
> Detail and hint messages: Use complete sentences, and end each with a
> period. Capitalize the first word of sentences.
Thanks, I didn't know that guideline. Both fixed in v6.
--
Erik
>From 39d2dc9b58dfa3b68245070648ecdf9eed892c7a Mon Sep 17 00:00:00 2001
From: Erik Wienhold
Date: Fri, 8 Mar 2024 04:21:56 +0100
Subject: [PATCH v6] Document that typed tables rely on CREATE TYPE
CREATE TABLE OF requires a stand-alone composite type that is not the
row type of another table. Clarify that with a reference to CREATE TYPE
in the docs. Also report a separate error message in this case.
Reworded docs provided by David G. Johnston.
---
doc/src/sgml/ref/create_table.sgml| 16
src/backend/commands/tablecmds.c | 9 -
src/test/regress/expected/typed_table.out | 7 ++-
src/test/regress/sql/typed_table.sql | 4
4 files changed, 26 insertions(+), 10 deletions(-)
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index f19306e776..11458be9cf 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -249,18 +249,18 @@ WITH ( MODULUS numeric_literal, REM
Creates a typed table, which takes its
- structure from the specified composite type (name optionally
- schema-qualified). A typed table is tied to its type; for
- example the table will be dropped if the type is dropped
- (with DROP TYPE ... CASCADE).
+ structure from an existing (name optionally schema-qualified) stand-alone
+ composite type (i.e., created using )
+ though it still produces a new composite type as well. The table will
+ have a dependency on the referenced type such that cascaded alter and
+ drop actions on the type will propagate to the table.
- When a typed table is created, then the data types of the
- columns are determined by the underlying composite type and are
- not specified by the CREATE TABLE command.
+ A typed table always has the same column names and data types as the
+ type it is derived from, and you cannot specify additional columns.
But the CREATE TABLE command can add defaults
- and constraints to the table and can specify storage parameters.
+ and constraints to the table, as well as specify storage parameters.
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 313c782cae..2331a9185a 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6935,8 +6935,15 @@ check_of_type(HeapTuple typetuple)
* the type before the typed table creation/conversion commits.
*/
relation_close(typeRelation, NoLock);
+
+ if (!typeOk)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("type %s must not be a row type of another table",
+ format_type_be(typ->oid)),
+ errhint("Must be a stand-alone composite type created with CREATE TYPE.")));
}
- if (!typeOk)
+ else
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("type %s is not a composite type",
diff --git a/src/test/regress/expected/typed_table.out b/src/test/regress/expected/typed_table.out
index 2e47ecbcf5..5743e74978 100644
--- a/src/test/regress/expected/typed_table.out
+++ b/src/test/regress/expected/typed
Re: Underscore in positional parameters?
On 2024-05-17 02:06 +0200, Michael Paquier wrote:
> On Thu, May 16, 2024 at 08:41:11AM +0200, Peter Eisentraut wrote:
> > On this specific patch, maybe reword "parameter too large" to "parameter
> > number too large".
>
> WFM here.
Done in v3.
I noticed this compiler warning with my previous patch:
scan.l:997:41: warning: ISO C90 forbids mixed declarations and code
[-Wdeclaration-after-statement]
997 | ErrorSaveContext escontext
= {T_ErrorSaveContext};
| ^~~~
I thought that I had to factor this out into a function similar to
process_integer_literal (which also uses ErrorSaveContext). But moving
that declaration to the start of the {param} action was enough in the
end.
While trying out the refactoring, I noticed two small things that can be
fixed as well in scan.l:
* Prototype and definition of addunicode do not match. The prototype
uses yyscan_t while the definition uses core_yyscan_t.
* Parameter base of process_integer_literal is unused.
But those should be one a separate thread, right, even for minor fixes?
--
Erik
>From d3ad2971dcb8ab15fafe1a5c69a15e5994eac76d Mon Sep 17 00:00:00 2001
From: Erik Wienhold
Date: Tue, 14 May 2024 14:12:11 +0200
Subject: [PATCH v3 1/3] Fix overflow in parsing of positional parameter
Replace atol with pg_strtoint32_safe in the backend parser and with
strtoint in ECPG to reject overflows when parsing the number of a
positional parameter. With atol from glibc, parameters $2147483648 and
$4294967297 turn into $-2147483648 and $1, respectively.
---
src/backend/parser/scan.l| 8 +++-
src/interfaces/ecpg/preproc/pgc.l| 5 -
src/test/regress/expected/numerology.out | 4
src/test/regress/sql/numerology.sql | 1 +
4 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/src/backend/parser/scan.l b/src/backend/parser/scan.l
index 9b33fb8d72..436cc64f07 100644
--- a/src/backend/parser/scan.l
+++ b/src/backend/parser/scan.l
@@ -992,8 +992,14 @@ other .
}
{param} {
+ ErrorSaveContext escontext = {T_ErrorSaveContext};
+ int32 val;
+
SET_YYLLOC();
- yylval->ival = atol(yytext + 1);
+ val = pg_strtoint32_safe(yytext + 1, (Node *) );
+ if (escontext.error_occurred)
+ yyerror("parameter number too large");
+ yylval->ival = val;
return PARAM;
}
{param_junk} {
diff --git a/src/interfaces/ecpg/preproc/pgc.l b/src/interfaces/ecpg/preproc/pgc.l
index d117cafce6..7122375d72 100644
--- a/src/interfaces/ecpg/preproc/pgc.l
+++ b/src/interfaces/ecpg/preproc/pgc.l
@@ -938,7 +938,10 @@ cppline {space}*#([^i][A-Za-z]*|{if}|{ifdef}|{ifndef}|{import})((\/\*[^*/]*\*+
}
{param} {
- base_yylval.ival = atol(yytext+1);
+ errno = 0;
+ base_yylval.ival = strtoint(yytext+1, NULL, 10);
+ if (errno == ERANGE)
+ mmfatal(PARSE_ERROR, "parameter number too large");
return PARAM;
}
{param_junk} {
diff --git a/src/test/regress/expected/numerology.out b/src/test/regress/expected/numerology.out
index c8196d2c85..5bac05c3b3 100644
--- a/src/test/regress/expected/numerology.out
+++ b/src/test/regress/expected/numerology.out
@@ -206,6 +206,10 @@ PREPARE p1 AS SELECT $1a;
ERROR: trailing junk after parameter at or near "$1a"
LINE 1: PREPARE p1 AS SELECT $1a;
^
+PREPARE p1 AS SELECT $2147483648;
+ERROR: parameter number too large at or near "$2147483648"
+LINE 1: PREPARE p1 AS SELECT $2147483648;
+ ^
SELECT 0b;
ERROR: invalid binary integer at or near "0b"
LINE 1: SELECT 0b;
diff --git a/src/test/regress/sql/numerology.sql b/src/test/regress/sql/numerology.sql
index 3f0ec34ecf..6722a09c5f 100644
--- a/src/test/regress/sql/numerology.sql
+++ b/src/test/regress/sql/numerology.sql
@@ -52,6 +52,7 @@ SELECT 0.0e1a;
SELECT 0.0e;
SELECT 0.0e+a;
PREPARE p1 AS SELECT $1a;
+PREPARE p1 AS SELECT $2147483648;
SELECT 0b;
SELECT 1b;
--
2.45.1
Re: CREATE TABLE creates a composite type corresponding to the table row, which is and is not there
On 2024-05-16 17:47 +0200, David G. Johnston wrote:
> On Wed, May 15, 2024 at 8:46 AM Robert Haas wrote:
>
> > On Thu, Apr 4, 2024 at 12:41 AM Erik Wienhold wrote:
> > > Thanks, fixed in v4. Looks like American English prefers that comma and
> > > it's also more common in our docs.
> >
> > Reviewing this patch:
> >
> > - Creates a typed table, which takes its
> > - structure from the specified composite type (name optionally
> > - schema-qualified). A typed table is tied to its type; for
> > - example the table will be dropped if the type is dropped
> > - (with DROP TYPE ... CASCADE).
> > + Creates a typed table, which takes its
> > structure
> > + from an existing (name optionally schema-qualified) stand-alone
> > composite
> > + type (i.e., created using ) though
> > it
> > + still produces a new composite type as well. The table will have
> > + a dependency on the referenced type such that cascaded alter and
> > drop
> > + actions on the type will propagate to the table.
> >
> > It would be better if this diff didn't reflow the unchanged portions
> > of the paragraph.
Right. I now reformatted it so that first line remains unchanged. But
the rest of the para is still a complete rewrite.
> > I agree that it's a good idea to mention that the table must have been
> > created using CREATE TYPE .. AS here, but I disagree with the rest of
> > the rewording in this hunk. I think we could just add "creating using
> > CREATE TYPE" to the end of the first sentence, with an xref, and leave
> > the rest as it is.
>
>
>
> > I don't see a reason to mention that the typed
> > table also spawns a rowtype; that's just standard CREATE TABLE
> > behavior and not really relevant here.
>
>
> I figured it wouldn't be immediately obvious that the system would create a
> second type with identical structure. Of course, in order for SELECT tbl
> FROM tbl; to work it must indeed do so. I'm not married to pointing out
> this dynamic explicitly though.
>
>
> > And I don't understand what the
> > rest of the rewording does for us.
> >
>
> It calls out the explicit behavior that the table's columns can change due
> to actions on the underlying type. Mentioning this unique behavior seems
> worth a sentence.
>
>
> >
> > - When a typed table is created, then the data types of the
> > - columns are determined by the underlying composite type and are
> > - not specified by the CREATE TABLE command.
> > + A typed table always has the same column names and data types as the
> > + type it is derived from, and you cannot specify additional columns.
> >But the CREATE TABLE command can add defaults
> > - and constraints to the table and can specify storage parameters.
> > + and constraints to the table, as well as specify storage parameters.
> >
> >
> > I don't see how this is better.
> >
>
> I'll agree this is more of a stylistic change, but mainly because the talk
> about data types reasonably implies the other items the patch explicitly
> mentions - names and additional columns.
I prefer David's changes to both paras because right now the details of
typed tables are spread over the respective CREATE and ALTER commands
for types and tables. Or maybe we should add those details to the
existing "Typed Tables" section at the very end of CREATE TABLE?
> > - errmsg("type %s is not a composite type",
> > + errmsg("type %s is not a stand-alone composite type",
> >
> > I agree with Peter's complaint that people aren't going to understand
> > what a stand-alone composite type means when they see the revised
> > error message; to really help people, we're going to need to do better
> > than this, I think.
> >
> >
> We have a glossary.
>
> That said, leave the wording as-is and add a conditional hint: The
> composite type must not also be a table.
It's now a separate error message (like I already had in v1) which
states that the specified type must not be a row type of another table
(based on Peter's feedback). And the hint directs the user to CREATE
TYPE.
In passing, I also quoted the type name in the existing error message
for consistency. I saw that table names etc. are already quoted in
other error messages.
--
Erik
>From 575bfec362bde5bf77ccb793bd8e2d78679c062f Mon Sep 17 00:00:00 2001
From: Erik Wienhold
Date: Fri, 8 Mar 2024 04:21:56 +0100
Subject: [PATCH v5] Document that typed tables rely on CREATE TYPE
CREATE TABLE OF requires a s
plpgsql: fix parsing of integer range with underscores
plpgsql fails to parse 1_000..1_000 as 1000..1000 in FOR loops:
DO $$
DECLARE
i int;
BEGIN
FOR i IN 1_000..1_000 LOOP
END LOOP;
END $$;
ERROR: syntax error at or near "1_000."
LINE 5: FOR i IN 1_000..1_000 LOOP
The scan.l defines rule "numericfail" to handle this ambiguity without
requiring extra whitespace or parenthesis around the integer literals.
But the rule only accepts digits 0-9. Again, an oversight in
faff8f8e47. Fixed in the attached patch.
--
Erik
>From 7e51d9ea7ef2ffc2a2b8c831533a382a5493575b Mon Sep 17 00:00:00 2001
From: Erik Wienhold
Date: Wed, 15 May 2024 02:43:12 +0200
Subject: [PATCH] plpgsql: fix parsing of integer range with underscores
Fix lexer rule "numericfail" that ensures that we parse 1_000..1_000 as
1000..1000 instead of failing with 'syntax error at or near "1_000."' or
requiring extra whitespace/parenthesis to resolve the ambiguity.
Oversight in faff8f8e47 which added underscores to numeric literals.
---
src/backend/parser/scan.l| 2 +-
src/fe_utils/psqlscan.l | 2 +-
src/interfaces/ecpg/preproc/pgc.l| 2 +-
src/test/regress/expected/numerology.out | 7 +++
src/test/regress/sql/numerology.sql | 8
5 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/src/backend/parser/scan.l b/src/backend/parser/scan.l
index 5eaadf53b2..a8e504ad09 100644
--- a/src/backend/parser/scan.l
+++ b/src/backend/parser/scan.l
@@ -407,7 +407,7 @@ octfail 0[oO]_?
binfail 0[bB]_?
numeric (({decinteger}\.{decinteger}?)|(\.{decinteger}))
-numericfail {decdigit}+\.\.
+numericfail {decinteger}\.\.
real ({decinteger}|{numeric})[Ee][-+]?{decinteger}
realfail ({decinteger}|{numeric})[Ee][-+]
diff --git a/src/fe_utils/psqlscan.l b/src/fe_utils/psqlscan.l
index c9df0594fd..7780151434 100644
--- a/src/fe_utils/psqlscan.l
+++ b/src/fe_utils/psqlscan.l
@@ -343,7 +343,7 @@ octfail 0[oO]_?
binfail 0[bB]_?
numeric (({decinteger}\.{decinteger}?)|(\.{decinteger}))
-numericfail {decdigit}+\.\.
+numericfail {decinteger}\.\.
real ({decinteger}|{numeric})[Ee][-+]?{decinteger}
realfail ({decinteger}|{numeric})[Ee][-+]
diff --git a/src/interfaces/ecpg/preproc/pgc.l b/src/interfaces/ecpg/preproc/pgc.l
index bcfbd0978b..7744d2facf 100644
--- a/src/interfaces/ecpg/preproc/pgc.l
+++ b/src/interfaces/ecpg/preproc/pgc.l
@@ -376,7 +376,7 @@ octfail 0[oO]_?
binfail 0[bB]_?
numeric (({decinteger}\.{decinteger}?)|(\.{decinteger}))
-numericfail {decdigit}+\.\.
+numericfail {decinteger}\.\.
real ({decinteger}|{numeric})[Ee][-+]?{decinteger}
realfail ({decinteger}|{numeric})[Ee][-+]
diff --git a/src/test/regress/expected/numerology.out b/src/test/regress/expected/numerology.out
index f662a5050a..86caccbbc5 100644
--- a/src/test/regress/expected/numerology.out
+++ b/src/test/regress/expected/numerology.out
@@ -297,6 +297,13 @@ SELECT 1_000.5e0_1;
10005
(1 row)
+DO $$
+DECLARE
+ i int;
+BEGIN
+ FOR i IN 1_000..1_000 LOOP
+ END LOOP;
+END $$;
-- error cases
SELECT _100;
ERROR: column "_100" does not exist
diff --git a/src/test/regress/sql/numerology.sql b/src/test/regress/sql/numerology.sql
index 1941c58e68..eb8d4e48d7 100644
--- a/src/test/regress/sql/numerology.sql
+++ b/src/test/regress/sql/numerology.sql
@@ -77,6 +77,14 @@ SELECT 1_000.;
SELECT .000_005;
SELECT 1_000.5e0_1;
+DO $$
+DECLARE
+ i int;
+BEGIN
+ FOR i IN 1_000..1_000 LOOP
+ END LOOP;
+END $$;
+
-- error cases
SELECT _100;
SELECT 100_;
--
2.45.1
Re: Underscore in positional parameters?
On 2024-05-14 16:40 +0200, Tom Lane wrote:
> Dean Rasheed writes:
> > On Tue, 14 May 2024 at 07:43, Michael Paquier wrote:
> >> On Tue, May 14, 2024 at 05:18:24AM +0200, Erik Wienhold wrote:
> >>> Parameter $1_2 is taken as $1 because in rule {param} in scan.l we get
> >>> the parameter number with atol which stops at the underscore. That's a
> >>> regression in faff8f8e47f. Before that commit, $1_2 resulted in
> >>> "ERROR: trailing junk after parameter".
>
> > I'm sure that this wasn't intentional -- I think we just failed to
> > notice that "param" also uses "decinteger" in the scanner. Taking a
> > quick look, there don't appear to be any other uses of "decinteger",
> > so at least it only affects params.
>
> > Unless the spec explicitly says otherwise, I agree that we should
> > reject this, as we used to do, and add a comment saying that it's
> > intentionally not supported. I can't believe it would ever be useful,
> > and the current behaviour is clearly broken.
>
> +1, let's put this back the way it was.
I split the change in two independent patches:
Patch 0001 changes rules param and param_junk to only accept digits 0-9.
Patch 0002 replaces atol with pg_strtoint32_safe in the backend parser
and strtoint in ECPG. This fixes overflows like:
=> PREPARE p1 AS SELECT $4294967297; -- same as $1
PREPARE
=> EXECUTE p1 (123);
?column?
--
123
(1 row)
=> PREPARE p2 AS SELECT $2147483648;
ERROR: there is no parameter $-2147483648
LINE 1: PREPARE p2 AS SELECT $2147483648;
It now returns this error:
=> PREPARE p1 AS SELECT $4294967297;
ERROR: parameter too large at or near $4294967297
=> PREPARE p2 AS SELECT $2147483648;
ERROR: parameter too large at or near $2147483648
--
Erik
>From a3a3d845bbc60cb5ff1bc510205c84ab3bdeddbf Mon Sep 17 00:00:00 2001
From: Erik Wienhold
Date: Tue, 14 May 2024 14:18:59 +0200
Subject: [PATCH v2 1/2] Forbid underscore in positional parameters
Underscores were added to numeric literals in faff8f8e47. In an
oversight, this change also affected the positional parameters rule
which use the same production for its digits. Underscores are not
necessary for positional parameters, so revert that rule to its old form
that only accepts digits 0-9.
---
src/backend/parser/scan.l| 5 +++--
src/fe_utils/psqlscan.l | 5 +++--
src/interfaces/ecpg/preproc/pgc.l| 5 +++--
src/test/regress/expected/numerology.out | 4
src/test/regress/sql/numerology.sql | 1 +
5 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/src/backend/parser/scan.l b/src/backend/parser/scan.l
index 5eaadf53b2..b499975e9c 100644
--- a/src/backend/parser/scan.l
+++ b/src/backend/parser/scan.l
@@ -419,8 +419,9 @@ bininteger_junk {bininteger}{ident_start}
numeric_junk {numeric}{ident_start}
real_junk {real}{ident_start}
-param \${decinteger}
-param_junk \${decinteger}{ident_start}
+/* Positional parameters don't accept underscores. */
+param \${decdigit}+
+param_junk \${decdigit}+{ident_start}
other .
diff --git a/src/fe_utils/psqlscan.l b/src/fe_utils/psqlscan.l
index c9df0594fd..c6d02439ab 100644
--- a/src/fe_utils/psqlscan.l
+++ b/src/fe_utils/psqlscan.l
@@ -355,8 +355,9 @@ bininteger_junk {bininteger}{ident_start}
numeric_junk {numeric}{ident_start}
real_junk {real}{ident_start}
-param \${decinteger}
-param_junk \${decinteger}{ident_start}
+/* Positional parameters don't accept underscores. */
+param \${decdigit}+
+param_junk \${decdigit}+{ident_start}
/* psql-specific: characters allowed in variable names */
variable_char [A-Za-z\200-\377_0-9]
diff --git a/src/interfaces/ecpg/preproc/pgc.l b/src/interfaces/ecpg/preproc/pgc.l
index bcfbd0978b..d117cafce6 100644
--- a/src/interfaces/ecpg/preproc/pgc.l
+++ b/src/interfaces/ecpg/preproc/pgc.l
@@ -388,8 +388,9 @@ bininteger_junk {bininteger}{ident_start}
numeric_junk {numeric}{ident_start}
real_junk {real}{ident_start}
-param \${decinteger}
-param_junk \${decinteger}{ident_start}
+/* Positional parameters don't accept underscores. */
+param \${decdigit}+
+param_junk \${decdigit}+{ident_start}
/* special characters for other dbms */
/* we have to react differently in compat mode */
diff --git a/src/test/regress/expected/numerology.out b/src/test/regress/expected/numerology.out
index f662a5050a..c8196d2c85 100644
--- a/src/test/regress/expected/numerology.out
+++ b/src/test/regress/expected/numerology.out
@@ -330,6 +330,10 @@ SELECT 1_000.5e_1;
ERROR: trailing junk after numeric literal at or near "1_000.5e"
LINE 1: SELECT 1_000.5e_1;
^
+PREPARE p1 AS SELECT $0_1;
+ERROR: trailing junk after parameter at or near "$0_"
+LINE 1: PREPARE p1 AS SELECT $0_1;
+
Underscore in positional parameters?
I noticed that we (kind of) accept underscores in positional parameters.
For example, this works:
=> PREPARE p1 AS SELECT $1_2;
PREPARE
=> EXECUTE p1 (123);
?column?
--
123
(1 row)
Parameter $1_2 is taken as $1 because in rule {param} in scan.l we get
the parameter number with atol which stops at the underscore. That's a
regression in faff8f8e47f. Before that commit, $1_2 resulted in
"ERROR: trailing junk after parameter".
I can't tell which fix is the way to go: (1) accept underscores without
using atol, or (2) just forbid underscores. Any ideas?
atol can be replaced with pg_strtoint32_safe to handle the underscores.
This also avoids atol's undefined behavior on overflows. AFAICT,
positional parameters are not part of the SQL standard, so nothing
prevents us from accepting underscores here as well. The attached patch
does that and also adds a test case.
But reverting {param} to its old form to forbid underscores also makes
sense. That is:
param \${decdigit}+
param_junk \${decdigit}+{ident_start}
It seems very unlikely that anybody uses that many parameters and still
cares about readability to use underscores. But maybe users simply
expect that underscores are valid here as well.
--
Erik
>From 0f319818360cdd26e96198ea7fcaba1b8298f264 Mon Sep 17 00:00:00 2001
From: Erik Wienhold
Date: Tue, 14 May 2024 04:14:08 +0200
Subject: [PATCH] Fix parsing of positional parameters with underscores
Underscores were added to numeric literals in faff8f8e47. That also
introduced a regression whereby positional parameters accepted
underscores as well. But such parameter numbers were not parsed
correctly by atol which stops at the first non-digit character. So
parameter $1_2 would be taken as just $1. Fix that by replacing atol
with pg_strtoint32_safe. Thereby we also avoid the undefined behavior
of atol on overflows.
---
src/backend/parser/scan.l| 5 -
src/test/regress/expected/numerology.out | 1 +
src/test/regress/sql/numerology.sql | 2 ++
3 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/parser/scan.l b/src/backend/parser/scan.l
index 5eaadf53b2..ebb7ace9ca 100644
--- a/src/backend/parser/scan.l
+++ b/src/backend/parser/scan.l
@@ -992,7 +992,10 @@ other .
{param} {
SET_YYLLOC();
- yylval->ival = atol(yytext + 1);
+ ErrorSaveContext escontext = {T_ErrorSaveContext};
+ yylval->ival = pg_strtoint32_safe(yytext + 1, (Node *) );
+ if (escontext.error_occurred)
+ yyerror("parameter too large");
return PARAM;
}
{param_junk} {
diff --git a/src/test/regress/expected/numerology.out b/src/test/regress/expected/numerology.out
index f662a5050a..3c1308e22c 100644
--- a/src/test/regress/expected/numerology.out
+++ b/src/test/regress/expected/numerology.out
@@ -297,6 +297,7 @@ SELECT 1_000.5e0_1;
10005
(1 row)
+PREPARE p2 AS SELECT $0_1;
-- error cases
SELECT _100;
ERROR: column "_100" does not exist
diff --git a/src/test/regress/sql/numerology.sql b/src/test/regress/sql/numerology.sql
index 1941c58e68..11af76828d 100644
--- a/src/test/regress/sql/numerology.sql
+++ b/src/test/regress/sql/numerology.sql
@@ -77,6 +77,8 @@ SELECT 1_000.;
SELECT .000_005;
SELECT 1_000.5e0_1;
+PREPARE p2 AS SELECT $0_1;
+
-- error cases
SELECT _100;
SELECT 100_;
--
2.45.0
Re: Q: Escapes in jsonpath Idents
On 2024-04-24 13:52 +0200, David E. Wheeler wrote:
> On Apr 24, 2024, at 05:51, Peter Eisentraut wrote:
>
> >A is classified as follows.
> >
> >Case:
> >
> >a) A that is a is a > path context variable>.
> >
> >b) A that begins with is a
> > .
> >
> >c) Otherwise, a is a .
> >
> > Does this help? I wasn't following all the discussion to see if
> > there is anything wrong with the implementation.
Thanks Peter! But what is the definition of the entire path expression?
Perhaps something like:
::= { "." }
That would imply that "$.$foo" is a valid path that accesses a variable
member (but I guess the path evaluation is also specified somewhere).
Does it say anything about double-quoted accessors? In table 8.25[1] we
allow member accessor ."$varname" and it says "If the key name matches
some named variable starting with $ or does not meet the JavaScript
rules for an identifier, it must be enclosed in double quotes to make it
a string literal."
What bugs me about this description, after reading it a couple of times,
is that it's not clear what is meant by ."$varname". It could mean two
things: (1) the double-quoting masks $varname in order to not interpret
those characters as a variable or (2) an interpolated string that
resolves $varname and yields a dynamic member accessor.
The current implementation supports (1), i.e., ."$foo" does not refer to
variable foo but the actual property "$foo":
=> select jsonb_path_query('{"$foo":123,"bar":456}', '$."$foo"',
'{"foo":"bar"}');
jsonb_path_query
--
123
(1 row)
Under case (2) I'd expect that query to return 456 (because $foo
resolves to "bar"). (Similar to how psql would resolve :'foo' to
'bar'.)
Variables already work in array accessors and table 8.25 says that "The
specified index can be an integer, as well as an expression returning a
single numeric value [...]". A variable is such an expression.
=> select jsonb_path_query('[2,3,5]', '$[$i]', '{"i":1}');
jsonb_path_query
--
3
(1 row)
So I'd expect a similar behavior for member accessors as well when
seeing ."$varname" in the same table.
> Yes, it does, as it ties the special meaning of the dollar sign to the
> *beginning* of an expression. So it makes sense that this would be an
> error:
>
> david=# select '$.$foo'::jsonpath;
> ERROR: syntax error at or near "$foo" of jsonpath input
> LINE 1: select '$.$foo'::jsonpath;
>^
> But I’m less sure when a dollar sign is used in the *middle* (or end)
> of a json path identifier:
>
> david=# select '$.xx$foo'::jsonpath;
> ERROR: syntax error at or near "$foo" of jsonpath input
> LINE 1: select '$.xx$foo'::jsonpath;
>^
> Perhaps that should be valid?
Yes, I think so. That would be case C in the spec excerpt provided by
Peter. So it's just a key name that happens to contain (but not start
with) the dollar sign.
[1]
https://www.postgresql.org/docs/current/datatype-json.html#TYPE-JSONPATH-ACCESSORS
--
Erik
Re: CASE control block broken by a single line comment
On 2024-04-13 00:20 +0200, Tom Lane wrote:
> Erik Wienhold writes:
> > I'm surprised that the lexer handles compound tokens. I'd expect to
> > find that in the parser, especially because of using the context-aware
> > plpgsql_ns_lookup to determine if we have a T_DATUM or T_{WORD,CWORD}.
>
> I'm not here to defend plpgsql's factorization ;-). However, it
> doesn't really have a parser of its own, at least not for expressions,
> so I'm not sure how your suggestion could be made to work.
Not a suggestion. Just a question about the general design, unrelated
to this fix, in case you know the answer off the cuff. I see that
863a62064c already had the lexer handle those compound tokens, but
unfortunately without an explanation on why. Never mind if that's too
much to ask about a design descision made over 25 years ago.
--
Erik
Re: CASE control block broken by a single line comment
On 2024-04-09 00:54 +0200, Tom Lane wrote:
> I poked at this and found that the failures occur when the patched
> code decides to trim an expression like "_r.v" to just "_r", naturally
> breaking the semantics completely. That happens because when
> plpgsql_yylex recognizes a compound token, it doesn't bother to
> adjust the token length to include the additional word(s).
Thanks Tom! I haven't had the time to look at your patch.
I'm surprised that the lexer handles compound tokens. I'd expect to
find that in the parser, especially because of using the context-aware
plpgsql_ns_lookup to determine if we have a T_DATUM or T_{WORD,CWORD}.
Is this done by the lexer to allow push-back of those compound tokens
and maybe even to also simplify some parser rules?
--
Erik
Re: Converting README documentation to Markdown
On 2024-04-08 21:29 +0200, Daniel Gustafsson wrote: > Over in [0] I asked whether it would be worthwhile converting all our README > files to Markdown, and since it wasn't met with pitchforks I figured it would > be an interesting excercise to see what it would take (my honest gut feeling > was that it would be way too intrusive). Markdown does brings a few key > features however so IMHO it's worth attempting to see: > > * New developers are very used to reading/writing it > * Using a defined format ensures some level of consistency > * Many users and contributors new *as well as* old like reading documentation > nicely formatted in a browser > * The documentation now prints really well > * pandoc et.al can be used to render nice looking PDF's > * All the same benefits as discussed in [0] > > The plan was to follow Grubers original motivation for Markdown closely: > > "The idea is that a Markdown-formatted document should be publishable > as-is, as plain text, without looking like it’s been marked up with > tags or formatting instructions." +1 for keeping the plaintext readable. > This translates to making the least amount of changes to achieve a) retained > plain text readability at todays level, b) proper Markdown rendering, not > looking like text files in a HTML window, and c) absolutly no reflows and > minimal impact on git blame. > > Turns out we've been writing Markdown for quite some time, so it really didn't > take much at all. I renamed all the files .md and with almost just changing > whitespace achieved what I think is pretty decent results. The rendered > versions can be seen by browsing the tree below: > > https://github.com/danielgustafsson/postgres/tree/markdown > > The whitespace changes are mostly making sure that code (anything which is to > be rendered without styling really) is indented from column 0 with tab or 4 > spaces (depending on what was already used in the file) and has a blank line > before and after. This is the bulk of the changes. I've only peeked at a couple of those READMEs, but they look alright so far (at least on GitHub). Should we settle on a specific Markdown flavor[1]? Because I'm never sure if some markups only work on specific code-hosting sites. Maybe also a guide on writing Markdown that renders properly, especially with regard to escaping that may be necessary (see below). > The non-whitespace changes introduced are: > > [...] > > * In the regex README there are two file references using * as a wildcard, but > the combination of the two makes Markdown render the text between them in > italics. Wrapping these in backticks solves it, but I'm not a fan since we > don't do that elsewhere. A solution which avoids backticks would ne nice. Escaping does the trick: regc_\*.c > [...] > > * Anything inside <> is rendered as a link if it matches, so in cases where > > is used to indicatee "replace with X" I added whitespace like "< X >" which > might be a bit ugly, but works. When referencing header files with > the <> are removed to just say the header name, which seemed like the least > bad > option there. Can be escaped as well: \ [1] https://markdownguide.offshoot.io/extended-syntax/#lightweight-markup-languages -- Erik
Re: ❓ JSON Path Dot Precedence
On 2024-04-07 18:13 +0200, David E. Wheeler wrote:
> A question about the behavior of the JSON Path parser. The docs[1]
> have this to say about numbers:
>
> > Numeric literals in SQL/JSON path expressions follow JavaScript
> > rules, which are different from both SQL and JSON in some minor
> > details. For example, SQL/JSON path allows .1 and 1., which are
> > invalid in JSON.
>
> In other words, this is valid:
>
> david=# select '2.'::jsonpath;
> jsonpath
> --
> 2
>
> But this feature creates a bit of a conflict with the use of a dot for
> path expressions. Consider `0x2.p10`. How should that be parsed? As an
> invalid decimal expression ("trailing junk after numeric literal”), or
> as a valid integer 2 followed by the path segment “p10”? Here’s the
> parser’s answer:
>
> david=# select '0x2.p10'::jsonpath;
> jsonpath
> ---
> (2)."p10"
>
> So it would seem that, other things being equal, a path key expression
> (`.foo`) is slightly higher precedence than a decimal expression. Is
> that intentional/correct?
I guess jsonpath assumes that hex, octal, and binary literals are
integers. So there's no ambiguity about any fractional part that might
follow.
> Discovered while writing my Go lexer and throwing all of Go’s floating
> point literal examples[2] at it and comparing to the Postgres path
> parser. Curiously, this is only an issue for 0x/0o/0b numeric
> expressions; a decimal expression does not behave in the same way:
>
> david=# select '2.p10'::jsonpath;
> ERROR: trailing junk after numeric literal at or near "2.p" of jsonpath input
> LINE 1: select '2.p10'::jsonpath;
It scans the decimal "2." and then finds junks "p10".
Works with a full decimal:
test=# select '3.14.p10'::jsonpath;
jsonpath
--
(3.14)."p10"
(1 row)
And with extra whitespace to resolve the ambiguity:
test=# select '2 .p10'::jsonpath;
jsonpath
---
(2)."p10"
(1 row)
> Which maybe seems a bit inconsistent.
>
> Thoughts on what the “correct” behavior should be?
I'd say a member accessor after a number doesn't really make sense
because object keys are strings. One could argue that path "$.2.p10"
should match JSON '{"2":{"p10":42}}', i.e. the numeric accessor is
converted to a string. For example, in nodejs I can do:
> var x = {2: {p10: 42}}
> x[2].p10
42
But that's JavaScript, not JSON.
Also, is there even a use case for path "0x2.p10"? The path has to
start with "$" or ("@" in case of a filter expression), doesn't it? And
it that case it doesn't parse:
test=# select '$.0x2.p10'::jsonpath;
ERROR: trailing junk after numeric literal at or near ".0x" of jsonpath
input
LINE 1: select '$.0x2.p10'::jsonpath;
Even with extra whitespace:
test=# select '$ . 0x2 . p10'::jsonpath;
ERROR: syntax error at or near "0x2" of jsonpath input
LINE 1: select '$ . 0x2 . p10'::jsonpath;
Or should it behave like an array accessor? Similar to:
test=# select jsonb_path_query('[0,1,{"p10":42},3]',
'$[0x2].p10'::jsonpath);
jsonb_path_query
--
42
(1 row)
> [1]:
> https://www.postgresql.org/docs/devel/datatype-json.html#DATATYPE-JSONPATH
> [2]: https://tip.golang.org/ref/spec#Floating-point_literals
--
Erik
Re: CASE control block broken by a single line comment
I wrote:
> Attached v2 tries to do that.
Hit send too soon. Attached now.
--
Erik
>From 23a20a410dc92946141b6c6fa5100473eac482cf Mon Sep 17 00:00:00 2001
From: Erik Wienhold
Date: Sat, 6 Apr 2024 22:36:54 +0200
Subject: [PATCH v2] plpgsql: accept trailing line comment in CASE WHEN
Expression in CASE x WHEN THEN may end with a line
comment. This results in a syntax error when the WHEN clause is
rewritten to x IN (). Fix that by tracking the end location of
the token in read_sql_construct and only copy that range instead of up
to yylloc.
TODO: This breaks other test cases, probably because the calculation of
endlocation is off. But I still don't know why.
---
src/pl/plpgsql/src/expected/plpgsql_control.out | 17 +
src/pl/plpgsql/src/pl_gram.y| 10 +-
src/pl/plpgsql/src/pl_scanner.c | 8 +++-
src/pl/plpgsql/src/plpgsql.h| 1 +
src/pl/plpgsql/src/sql/plpgsql_control.sql | 14 ++
5 files changed, 48 insertions(+), 2 deletions(-)
diff --git a/src/pl/plpgsql/src/expected/plpgsql_control.out b/src/pl/plpgsql/src/expected/plpgsql_control.out
index 328bd48586..ccd4f54704 100644
--- a/src/pl/plpgsql/src/expected/plpgsql_control.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_control.out
@@ -681,3 +681,20 @@ select case_test(13);
other
(1 row)
+-- test line comment between WHEN and THEN
+create or replace function case_comment(int) returns text as $$
+begin
+ case $1
+when 1 -- comment before THEN
+ then return 'one';
+else
+ return 'other';
+ end case;
+end;
+$$ language plpgsql immutable;
+select case_comment(1);
+ case_comment
+--
+ one
+(1 row)
+
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index bef33d58a2..b2067c1d2c 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -2693,6 +2693,7 @@ read_sql_construct(int until,
StringInfoData ds;
IdentifierLookup save_IdentifierLookup;
int startlocation = -1;
+ int endlocation = -1;
int parenlevel = 0;
PLpgSQL_expr *expr;
@@ -2743,6 +2744,9 @@ read_sql_construct(int until,
expected),
parser_errposition(yylloc)));
}
+
+ /* TODO endlocation seems to be off, thus breaking tests */
+ endlocation = yylloc + plpgsql_token_length();
}
plpgsql_IdentifierLookup = save_IdentifierLookup;
@@ -2761,7 +2765,11 @@ read_sql_construct(int until,
yyerror("missing SQL statement");
}
- plpgsql_append_source_text(, startlocation, yylloc);
+ /* TODO probably need to check that we've got proper range */
+ if (startlocation >= endlocation)
+ yyerror("startlocation >= endlocation");
+
+ plpgsql_append_source_text(, startlocation, endlocation);
/* trim any trailing whitespace, for neatness */
if (trim)
diff --git a/src/pl/plpgsql/src/pl_scanner.c b/src/pl/plpgsql/src/pl_scanner.c
index 101804d506..132c9df05b 100644
--- a/src/pl/plpgsql/src/pl_scanner.c
+++ b/src/pl/plpgsql/src/pl_scanner.c
@@ -132,7 +132,6 @@ static int internal_yylex(TokenAuxData *auxdata);
static void push_back_token(int token, TokenAuxData *auxdata);
static void location_lineno_init(void);
-
/*
* This is the yylex routine called from the PL/pgSQL grammar.
* It is a wrapper around the core lexer, with the ability to recognize
@@ -396,6 +395,13 @@ plpgsql_token_is_unreserved_keyword(int token)
return false;
}
+/* Return the length of the last scanned token. */
+int
+plpgsql_token_length(void)
+{
+ return plpgsql_yyleng;
+}
+
/*
* Append the function text starting at startlocation and extending to
* (not including) endlocation onto the existing contents of "buf".
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index 40056bb851..c48ee062a7 100644
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -1319,6 +1319,7 @@ extern int plpgsql_base_yylex(void);
extern int plpgsql_yylex(void);
extern void plpgsql_push_back_token(int token);
extern bool plpgsql_token_is_unreserved_keyword(int token);
+extern int plpgsql_token_length(void);
extern void plpgsql_append_source_text(StringInfo buf,
int startlocation, int endlocation);
extern int plpgsql_peek(void);
diff --git a/src/pl/plpgsql/src/sql/plpgsql_control.sql b/src/pl/plpgsql/src/sql/plpgsql_control.sql
index ed7231134f..8e007c51dc 100644
--- a/src/pl/plpgsql/src/sql/plpgsql_control.sql
+++ b/src/pl/plpgsql/src/sql/plpgsql_control.sql
@@ -486,3 +486,17 @@ select case_test(1);
select case_test(2);
select case_test(12);
select case_test(13);
+
+-- test line comment between WHEN and THEN
+create or replace function case_comment(int) returns text as $$
+begin
+ case $1
+when 1 -- comment before THEN
+ then return 'one';
+else
+ return 'other';
+ end case;
+end;
+$$ language plpgsql immutable;
+
+select case_comment(1);
--
2.44.0
Re: CASE control block broken by a single line comment
On 2024-04-07 06:33 +0200, Tom Lane wrote: > Erik Wienhold writes: > > I'm surprised that the comment is not skipped by the scanner at this > > point. Maybe because the parser just reads the raw expression between > > WHEN and THEN with plpgsql_append_source_text via read_sql_construct. > > > How about the attached patch? It's a workaround by simply adding a line > > feed character between the raw expression and the closing parenthesis. > > I don't have time to look into this on this deadline weekend, Sure, no rush. > but what's bothering me about this report is the worry that we've made > the same mistake elsewhere, or will do so in future. Right. At the moment only make_case is affected by this because it uses the raw expression for rewriting. I checked other uses of read_psql_construct (e.g. IF ... THEN, FOR ... LOOP) and they don't show this bug. > I suspect it'd be much more robust if we could remove the comment from > the expr->query string. No idea how hard that is. I slept on it and I think this can be fixed by tracking the end of the last token before THEN and use that instead of yylloc in the call to plpgsql_append_source_text. We already already track the token length in plpgsql_yyleng but don't make it available outside pl_scanner.c yet. Attached v2 tries to do that. But it breaks other test cases, probably because the calculation of endlocation is off. I'm missing something here. -- Erik
Re: CASE control block broken by a single line comment
On 2024-04-06 20:14 +0200, Michal Bartak wrote:
> The issue described bellow exists in postgresql ver 16.2 (found in some
> previous major versions)
Can confirm also on master.
> The documentation defines a comment as:
>
> > A comment is a sequence of characters beginning with double dashes and
> > extending to the end of the line
>
>
> When using such a comment within CASE control block, it ends up with an
> error:
>
> DO LANGUAGE plpgsql $$
> DECLARE
> t TEXT = 'a';
> BEGIN
> CASE t
> WHEN 'a' -- my comment
> THEN RAISE NOTICE 'a';
> WHEN 'b'
> THEN RAISE NOTICE 'b';
> ELSE NULL;
> END CASE;
> END;$$;
>
> ERROR: syntax error at end of input
> LINE 1: "__Case__Variable_2__" IN ('a' -- my comment)
> ^
> QUERY: "__Case__Variable_2__" IN ('a' -- my comment)
> CONTEXT: PL/pgSQL function inline_code_block line 5 at CASE
I'm surprised that the comment is not skipped by the scanner at this
point. Maybe because the parser just reads the raw expression between
WHEN and THEN with plpgsql_append_source_text via read_sql_construct.
How about the attached patch? It's a workaround by simply adding a line
feed character between the raw expression and the closing parenthesis.
--
Erik
>From 85456a22f41a8a51703650f2853fb6d8c9711fc7 Mon Sep 17 00:00:00 2001
From: Erik Wienhold
Date: Sat, 6 Apr 2024 22:36:54 +0200
Subject: [PATCH v1] plpgsql: create valid IN expression for CASE WHEN clause
The expression in CASE x WHEN THEN may end with a line comment.
This results in a syntax error when the WHEN clause is rewritten to
x IN (). Cope with that by appending \n after to terminate
the line comment.
---
src/pl/plpgsql/src/expected/plpgsql_control.out | 17 +
src/pl/plpgsql/src/pl_gram.y| 6 +-
src/pl/plpgsql/src/sql/plpgsql_control.sql | 14 ++
3 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/src/pl/plpgsql/src/expected/plpgsql_control.out
b/src/pl/plpgsql/src/expected/plpgsql_control.out
index 328bd48586..ccd4f54704 100644
--- a/src/pl/plpgsql/src/expected/plpgsql_control.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_control.out
@@ -681,3 +681,20 @@ select case_test(13);
other
(1 row)
+-- test line comment between WHEN and THEN
+create or replace function case_comment(int) returns text as $$
+begin
+ case $1
+when 1 -- comment before THEN
+ then return 'one';
+else
+ return 'other';
+ end case;
+end;
+$$ language plpgsql immutable;
+select case_comment(1);
+ case_comment
+--
+ one
+(1 row)
+
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index bef33d58a2..98339589ba 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -4161,7 +4161,11 @@ make_case(int location, PLpgSQL_expr *t_expr,
/* Do the string hacking */
initStringInfo();
- appendStringInfo(, "\"%s\" IN (%s)",
+ /*
+* The read expression may end in a line comment, so
+* append \n after it to create a valid expression.
+*/
+ appendStringInfo(, "\"%s\" IN (%s\n)",
varname, expr->query);
pfree(expr->query);
diff --git a/src/pl/plpgsql/src/sql/plpgsql_control.sql
b/src/pl/plpgsql/src/sql/plpgsql_control.sql
index ed7231134f..8e007c51dc 100644
--- a/src/pl/plpgsql/src/sql/plpgsql_control.sql
+++ b/src/pl/plpgsql/src/sql/plpgsql_control.sql
@@ -486,3 +486,17 @@ select case_test(1);
select case_test(2);
select case_test(12);
select case_test(13);
+
+-- test line comment between WHEN and THEN
+create or replace function case_comment(int) returns text as $$
+begin
+ case $1
+when 1 -- comment before THEN
+ then return 'one';
+else
+ return 'other';
+ end case;
+end;
+$$ language plpgsql immutable;
+
+select case_comment(1);
--
2.44.0
Re: IPC::Run::time[r|out] vs our TAP tests
On 2024-04-05 05:37 +0200, Tom Lane wrote:
> Erik Wienhold writes:
> > I'm trying to build Postgres with that older libedit version but can't
> > figure out what options to pass to ./configure so that it picks
> > /usr/local/lib/libedit.so instead of /usr/lib/libedit.so. This didn't
> > work:
>
> You probably want configure --with-libs=/usr/local/lib,
> and likely also --with-includes=/usr/local/include.
Thanks Tom. But I also have to run psql with:
LD_LIBRARY_PATH=/usr/local/lib:/usr/lib:/lib src/bin/psql/psql
Libedit 20191025-3.1 is the first version where ":{?VERB" works as
expected. The previous release 20190324-3.1 still produces the escaped
output that Michael found. That narrows down the changes to everything
between [1] (changed on 2019-03-24 but not included in 20190324-3.1) and
[2] (both inclusive).
[1]
https://github.com/NetBSD/src/commit/e09538bda2f805200d0f7ae09fb9b7f2f5ed75f2
[2]
https://github.com/NetBSD/src/commit/de11d876419df3570c2418468613aebcebafe6ae
--
Erik
Re: IPC::Run::time[r|out] vs our TAP tests
On 2024-04-05 05:10 +0200, Tom Lane wrote: > Erik Wienhold writes: > > It works with the latest libedit 20230828-3.1. Have to check the NetBSD > > source to find out what changed since 20181209-3.1. > > Yeah, the test is passing on mamba which is running the (just > officially released) NetBSD 10.0. I'm not sure whether 10.0 > has the "latest" libedit or something a little further back. > sidewinder, with NetBSD 9.3, is happy as well. But 20181209 > presumably belongs to NetBSD 8.x, which is theoretically still > in support, so maybe it's worth poking into. Having a look right now. Change [1] looks like a good candidate which is likely in 20221030-3.1. I'm trying to build Postgres with that older libedit version but can't figure out what options to pass to ./configure so that it picks /usr/local/lib/libedit.so instead of /usr/lib/libedit.so. This didn't work: LDFLAGS='-L/usr/local/lib' ./configure --with-libedit-preferred (My ld fu is not so great.) [1] https://github.com/NetBSD/src/commit/12863d4d7917df8a7ef5ad9dab6bb719018a22d1 -- Erik
Re: IPC::Run::time[r|out] vs our TAP tests
On 2024-04-05 04:37 +0200, Michael Paquier wrote:
> On Thu, Apr 04, 2024 at 10:31:24PM -0400, Tom Lane wrote:
> > Michael Paquier writes:
> >> This stuff is actually kind of funny on this host, "\echo :{?VERB\t"
> >> completes to something incorrect, as of:
> >> postgres=# \echo :\{\?VERBOSITY\}
> >
> > Just to be clear: you see the extra backslashes if you try this
> > tab-completion manually?
>
> Yeah, I do, after completing "\echo :{?VERB" with this version of
> libedit. I see that this completes with backslashes added before '{',
> '}' and '?'. The test is telling the same.
>
> >> Attaching the log file, for reference. Now I can see that this uses
> >> libedit at 3.1-20181209, which is far from recent. I'd be OK to just
> >> remove libedit from the build to remove this noise, still I am
> >> wondering if 927332b95e77 got what it was trying to achieve actually
> >> right. Thoughts?
> >
> > It kind of looks like a libedit bug, but maybe we should dig more
> > deeply. I felt itchy about 927332b95e77 removing '{' from the
> > WORD_BREAKS set, and wondered exactly how that would change readline's
> > behavior. But even if that somehow accounts for the extra backslash
> > before '{', it's not clear how it could lead to '?' and '}' also
> > getting backslashed.
>
> I don't have a clear idea, either. I also feel uneasy about
> 927332b95e77 and its change of WORD_BREAKS, but this has the smell
> of a bug from an outdated libedit version.
It works with the latest libedit 20230828-3.1. Have to check the NetBSD
source to find out what changed since 20181209-3.1.
https://github.com/NetBSD/src/tree/trunk/lib/libedit
--
Erik
Re: CREATE TABLE creates a composite type corresponding to the table row, which is and is not there
On 2024-04-04 03:29 +0200, David G. Johnston wrote:
> On Thu, Mar 28, 2024 at 8:02 PM Erik Wienhold wrote:
>
> > Thanks, that sounds better. I incorporated that with some minor edits
> > in the attached v3.
> >
>
> You added my missing ( but dropped the comma after "i.e."
Thanks, fixed in v4. Looks like American English prefers that comma and
it's also more common in our docs.
--
Erik
>From 8b29c5852762bacb637fab021a06b12ab5cd7f93 Mon Sep 17 00:00:00 2001
From: Erik Wienhold
Date: Fri, 8 Mar 2024 04:21:56 +0100
Subject: [PATCH v4] Document that typed tables rely on CREATE TYPE
CREATE TABLE OF requires a stand-alone composite type. Clarify that in
the error message. Also reword the docs to better explain the
connection between created table and stand-alone composite type.
Reworded docs provided by David G. Johnston.
---
doc/src/sgml/ref/create_table.sgml| 18 +-
src/backend/commands/tablecmds.c | 2 +-
src/test/regress/expected/typed_table.out | 2 +-
3 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/doc/src/sgml/ref/create_table.sgml
b/doc/src/sgml/ref/create_table.sgml
index dfb7822985..586ccb190b 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -249,19 +249,19 @@ WITH ( MODULUS numeric_literal, REM
OF type_name
- Creates a typed table, which takes its
- structure from the specified composite type (name optionally
- schema-qualified). A typed table is tied to its type; for
- example the table will be dropped if the type is dropped
- (with DROP TYPE ... CASCADE).
+ Creates a typed table, which takes its structure
+ from an existing (name optionally schema-qualified) stand-alone composite
+ type (i.e., created using ) though it
+ still produces a new composite type as well. The table will have
+ a dependency on the referenced type such that cascaded alter and drop
+ actions on the type will propagate to the table.
- When a typed table is created, then the data types of the
- columns are determined by the underlying composite type and are
- not specified by the CREATE TABLE command.
+ A typed table always has the same column names and data types as the
+ type it is derived from, and you cannot specify additional columns.
But the CREATE TABLE command can add defaults
- and constraints to the table and can specify storage parameters.
+ and constraints to the table, as well as specify storage parameters.
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 317b89f67c..d756d2b200 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6994,7 +6994,7 @@ check_of_type(HeapTuple typetuple)
if (!typeOk)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
-errmsg("type %s is not a composite type",
+errmsg("type %s is not a stand-alone composite
type",
format_type_be(typ->oid;
}
diff --git a/src/test/regress/expected/typed_table.out
b/src/test/regress/expected/typed_table.out
index 2e47ecbcf5..745fbde811 100644
--- a/src/test/regress/expected/typed_table.out
+++ b/src/test/regress/expected/typed_table.out
@@ -89,7 +89,7 @@ drop cascades to function get_all_persons()
drop cascades to table persons2
drop cascades to table persons3
CREATE TABLE persons5 OF stuff; -- only CREATE TYPE AS types may be used
-ERROR: type stuff is not a composite type
+ERROR: type stuff is not a stand-alone composite type
DROP TABLE stuff;
-- implicit casting
CREATE TYPE person_type AS (id int, name text);
--
2.44.0
Re: Add column name to error description
On 2024-03-31 15:22 +0200, Marcos Pegoraro wrote:
> This is my first patch, so sorry if I miss something.
Please make sure that tests are passing by running make check:
https://www.postgresql.org/docs/current/regress-run.html#REGRESS-RUN-TEMP-INST
The patch breaks src/test/regress/sql/plpgsql.sql at:
-- this does not work currently (no implicit casting)
create or replace function compos() returns compostype as $$
begin
return (1, 'hello');
end;
$$ language plpgsql;
select compos();
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
connection to server was lost
> If I have a function which returns lots of columns and any of these columns
> returns a wrong type it's not easy to see which one is that column because
> it points me only to its position, not its name. So, this patch only adds
> that column name, just that.
+1 for this improvement.
> create function my_f(a integer, b integer) returns table(first_col integer,
> lots_of_cols_later numeric) language plpgsql as $function$
> begin
> return query select a, b;
> end;$function$;
>
> select * from my_f(1,1);
> --ERROR: structure of query does not match function result type
> --Returned type integer does not match expected type numeric in column 2.
>
> For a function which has just 2 columns is easy but if it returns a hundred
> of columns, which one is that 66th column ?
>
> My patch just adds column name to that description message.
> --ERROR: structure of query does not match function result type
> --Returned type integer does not match expected type numeric in column 2-
> lots_of_cols_later.
> diff --git a/src/backend/access/common/attmap.c
> b/src/backend/access/common/attmap.c
> index b0fe27ef57..85f7c0cb8c 100644
> --- a/src/backend/access/common/attmap.c
> +++ b/src/backend/access/common/attmap.c
> @@ -118,12 +118,13 @@ build_attrmap_by_position(TupleDesc indesc,
> ereport(ERROR,
> (errcode(ERRCODE_DATATYPE_MISMATCH),
> errmsg_internal("%s", _(msg)),
> - errdetail("Returned type %s does not match expected
> type %s in column %d.",
> + errdetail("Returned type %s does not match expected
> type %s in column %d-%s.",
The format "%d-%s" is not ideal. I suggesst "%d (%s)".
> format_type_with_typemod(att->atttypid,
> att->atttypmod),
> format_type_with_typemod(atttypid,
> atttypmod),
> - noutcols)));
> + noutcols,
> + att->attname)));
Must be NameStr(att->attname) for use with printf's %s. GCC even prints
a warning:
attmap.c:121:60: warning: format ‘%s’ expects argument of type ‘char
*’, but argument 5 has type ‘NameData’ {aka ‘struct nameData’} [-Wformat=]
--
Erik
Re: PSQL Should \sv & \ev work with materialized views?
On 2024-03-29 04:27 +0100, Isaac Morland wrote: > On Thu, 28 Mar 2024 at 20:38, Erik Wienhold wrote: > > > > Of course the problem with using DROP and CREATE is that indexes and > > privileges (anything else?) must also be restored. I haven't bothered > > with that yet. > > > > Not just those — also anything that depends on the matview, such as views > and other matviews. Right. But you'd run into the same issue for a regular view if you use \ev and add DROP VIEW myview CASCADE which may be necessary if you want to change columns names and/or types. Likewise, you'd have to manually change DROP MATERIALIZED VIEW and add the CASCADE option to lose dependent objects. I think implementing CREATE OR REPLACE MATERIALIZED VIEW has more value. But the semantics have to be defined first. I guess it has to behave like CREATE OR REPLACE VIEW in that it only allows changing the query without altering column names and types. We could also implement \sv so that it only prints CREATE MATERIALIZED VIEW and change \ev to not work with matviews. Both commands use get_create_object_cmd to populate the query buffer, so you get \ev for free when changing \sv. -- Erik
Re: CREATE TABLE creates a composite type corresponding to the table row, which is and is not there
On 2024-03-29 02:42 +0100, David G. Johnston wrote:
> For consideration for the doc portion. The existing wording is too
> imprecise for my liking and just tacking on "expects...create type" is
> jarring.
>
> """
> Creates a typed table, which takes it structure from an existing (name
> optionally schema-qualified) stand-alone composite type i.e., one created
> using CREATE TYPE) though it still produces a new composite type as well.
> The table will have a dependency to the referenced type such cascaded alter
> and drop actions on the type will propagate to the table.
>
> A typed table always has the same column names and data types as the type
> it is derived from, and you cannot specify additional columns. But the
> CREATE TABLE command can add defaults and constraints to the table, as well
> as specify storage parameters.
> """
Thanks, that sounds better. I incorporated that with some minor edits
in the attached v3.
> We do use the term "stand-alone composite" in create type so I'm inclined
> to use it instead of "composite created with CREATE TYPE"; especially in
> the error messages; I'm a bit more willing to add the cross-reference to
> create type in the user docs.
Okay, changed in v3 as well. I used "created with CREATE TYPE" in the
error message because I thought it's clearer to the user. But I see no
reason for not using "stand-alone" here as well if it's the established
term.
--
Erik
>From e9a79e6d5e063098eed4f834b18d576089b38499 Mon Sep 17 00:00:00 2001
From: Erik Wienhold
Date: Fri, 8 Mar 2024 04:21:56 +0100
Subject: [PATCH v3] Document that typed tables rely on CREATE TYPE
CREATE TABLE OF requires a stand-alone composite type. Clarify that in
the error message. Also reword the docs to better explain the
connection between created table and stand-alone composite type.
Reworded docs provided by David G. Johnston.
---
doc/src/sgml/ref/create_table.sgml| 18 +-
src/backend/commands/tablecmds.c | 2 +-
src/test/regress/expected/typed_table.out | 2 +-
3 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/doc/src/sgml/ref/create_table.sgml
b/doc/src/sgml/ref/create_table.sgml
index dfb7822985..5c8c1edaed 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -249,19 +249,19 @@ WITH ( MODULUS numeric_literal, REM
OF type_name
- Creates a typed table, which takes its
- structure from the specified composite type (name optionally
- schema-qualified). A typed table is tied to its type; for
- example the table will be dropped if the type is dropped
- (with DROP TYPE ... CASCADE).
+ Creates a typed table, which takes its structure
+ from an existing (name optionally schema-qualified) stand-alone composite
+ type (i.e. created using ) though it
+ still produces a new composite type as well. The table will have
+ a dependency on the referenced type such that cascaded alter and drop
+ actions on the type will propagate to the table.
- When a typed table is created, then the data types of the
- columns are determined by the underlying composite type and are
- not specified by the CREATE TABLE command.
+ A typed table always has the same column names and data types as the
+ type it is derived from, and you cannot specify additional columns.
But the CREATE TABLE command can add defaults
- and constraints to the table and can specify storage parameters.
+ and constraints to the table, as well as specify storage parameters.
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 6741e721ae..8e9dbe4bee 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6990,7 +6990,7 @@ check_of_type(HeapTuple typetuple)
if (!typeOk)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
-errmsg("type %s is not a composite type",
+errmsg("type %s is not a stand-alone composite
type",
format_type_be(typ->oid;
}
diff --git a/src/test/regress/expected/typed_table.out
b/src/test/regress/expected/typed_table.out
index 2e47ecbcf5..745fbde811 100644
--- a/src/test/regress/expected/typed_table.out
+++ b/src/test/regress/expected/typed_table.out
@@ -89,7 +89,7 @@ drop cascades to function get_all_persons()
drop cascades to table persons2
drop cascades to table persons3
CREATE TABLE persons5 OF stuff; -- only CREATE TYPE AS types may be used
-ERROR: type stuff is not a composite type
+ERROR: type stuff is not a stand-alone composite type
DROP TABLE stuff;
-- implicit casting
CREATE TYPE person_type AS (id int, name text);
--
2.44.0
Re: PSQL Should \sv & \ev work with materialized views?
I wrote:
> On 2023-05-15 06:32 +0200, Kirk Wolak wrote:
> > Personally I would appreciate it if \sv actually showed you the DDL.
> > Oftentimes I will \ev something to review it, with syntax highlighting.
>
> +1. I was just reviewing some matviews and was surprised that psql
> lacks commands to show their definitions.
>
> But I think that it should be separate commands \sm and \em because we
> already have commands \dm and \dv that distinguish between matviews and
> views.
Separate commands are not necessary because \ev and \sv already have a
(disabled) provision in get_create_object_cmd for when CREATE OR REPLACE
MATERIALIZED VIEW is available. So I guess both commands should also
apply to matview. The attached patch replaces that provision with a
transaction that drops and creates the matview. This uses meta command
\; to put multiple statements into the query buffer without prematurely
sending those statements to the server.
Demo:
=> DROP MATERIALIZED VIEW IF EXISTS test;
DROP MATERIALIZED VIEW
=> CREATE MATERIALIZED VIEW test AS SELECT s FROM generate_series(1,
10) s;
SELECT 10
=> \sv test
BEGIN \;
DROP MATERIALIZED VIEW public.test \;
CREATE MATERIALIZED VIEW public.test AS
SELECT s
FROM generate_series(1, 10) s(s)
WITH DATA \;
COMMIT
=>
And \ev test works as well.
Of course the problem with using DROP and CREATE is that indexes and
privileges (anything else?) must also be restored. I haven't bothered
with that yet.
--
Erik
>From efb5e37d90b668011307b602655f28455d700635 Mon Sep 17 00:00:00 2001
From: Erik Wienhold
Date: Fri, 29 Mar 2024 01:08:35 +0100
Subject: [PATCH v1] psql: \ev and \sv for matviews
CREATE OR REPLACE is not available for materialized views so DROP and
CREATE them inside a transaction. Use meta command \; to compose the
query buffer without sending it to the server.
TODO: Re-create indexes and privileges which are currently lost by
relying on DROP and CREATE.
---
src/bin/psql/command.c | 29 -
1 file changed, 20 insertions(+), 9 deletions(-)
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 9b0fa041f7..f40c1d7f99 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -5575,19 +5575,22 @@ get_create_object_cmd(EditableObjectType obj_type, Oid
oid,
char *reloptions =
PQgetvalue(res, 0, 4);
char *checkoption =
PQgetvalue(res, 0, 5);
- /*
-* If the backend ever supports CREATE
OR REPLACE
-* MATERIALIZED VIEW, allow that here;
but as of today it
-* does not, so editing a matview
definition in this way
-* is impossible.
-*/
switch (relkind[0])
{
-#ifdef NOT_USED
case RELKIND_MATVIEW:
-
appendPQExpBufferStr(buf, "CREATE OR REPLACE MATERIALIZED VIEW ");
+ /*
+* Allow editing a
matview via separate DROP and
+* CREATE statement
inside a transaction. Use meta
+* command \; to write
more than one statement to
+* the query buffer
without sending it to the server.
+*/
+
appendPQExpBufferStr(buf, "BEGIN \\;\n");
+
appendPQExpBufferStr(buf, "DROP MATERIALIZED VIEW ");
+ appendPQExpBuffer(buf,
"%s.", fmtId(nspname));
+
appendPQExpBufferStr(buf, fmtId(relname));
+
appendPQExpBufferStr(buf, " \\;\n");
+
appendPQExpBufferStr(buf, "CREATE MATERIALIZED VIEW ");
break;
-#endif
case RELKIND_VIEW:
appendPQExpBufferStr(buf, "CREATE OR REPLACE VIEW ");
break;
@@ -5625,6 +5628,14 @@ get_create_object_cmd
Re: PSQL Should \sv & \ev work with materialized views?
On 2023-05-15 06:32 +0200, Kirk Wolak wrote: > Personally I would appreciate it if \sv actually showed you the DDL. > Oftentimes I will \ev something to review it, with syntax highlighting. +1. I was just reviewing some matviews and was surprised that psql lacks commands to show their definitions. But I think that it should be separate commands \sm and \em because we already have commands \dm and \dv that distinguish between matviews and views. > This should not be that difficult. Just looking for feedback. > Admittedly \e is questionable, because you cannot really apply the changes. > ALTHOUGH, I would consider that I could > BEGIN; > DROP MATERIALIZED VIEW ...; > CREATE MATERIALIZED VIEW ...; > > Which I had to do to change the WITH DATA so it creates with data when we > reload our object.s I think this could even be handled by optional modifiers, e.g. \em emits CREATE MATERIALIZED VIEW ... WITH NO DATA and \emD emits WITH DATA. Although I wouldn't mind manually changing WITH NO DATA to WITH DATA. -- Erik
Re: Q: Escapes in jsonpath Idents
On 2024-03-17 20:50 +0100, David E. Wheeler wrote: > On Mar 17, 2024, at 15:12, Erik Wienhold wrote: > > So I think it makes sense to reword the entire backslash part of the > > paragraph and remove references to JSON entirely. The attached patch > > does that and also formats the backslash escapes as a bulleted list for > > readability. > > Ah, it’s JavaScript format, not JSON! This does clarify things quite > nicely, thank you. Happy to add my review once it’s in a commit fest. Thanks. https://commitfest.postgresql.org/48/4899/ > > The first case ($.$foo) is in line with the restriction on member > > accessors that you quoted first. > > Huh, that’s now how I read it. Here it is again: > > >> Member accessor that returns an object member with the specified > >> key. If the key name matches some named variable starting with $ or > >> does not meet the JavaScript rules for an identifier, it must be > >> enclosed in double quotes to make it a string literal. > > > Note that in my example `$foo` does not match a variable. I mean it > looks like a variable, but none is used here. I guess it’s being > conservative because it might be used in one of the functions, like > jsonb_path_exists(), to which variables might be passed. I had the same reasoning while writing my first reply but scrapped that part because I found it obvious: That jsonpath is parsed before calling jsonb_path_exists() and therefore the parser has no context about any variables, which might not even be hardcoded but may result from a query. > > The error message 'syntax error at or near "$oo" of jsonpath input' for > > the second case ($.f$oo), however, looks as if the scanner identifies > > '$oo' as a variable instead of contiuing the scan of identifier (f$oo) > > for the member accessor. Looks like a bug to me because a variable > > doesn't even make sense in that place. > > Right. Maybe the docs should be updated to say that a literal dollar > sign isn’t supported in identifiers, unlike in JavaScript, except > through escapes like this: Unfortunately, I don't have access to that part of the SQL spec. So I don't know how the jsonpath grammar is specified. I had a look into Oracle, MySQL, and SQLite docs to see what they implement: * Oracle requires the unquoted field names to match [A-Za-z][A-Za-z0-9]* (see "object steps"). It also supports variables. https://docs.oracle.com/en/database/oracle/oracle-database/23/adjsn/json-path-expressions.html * MySQL refers to ECMAScript identifiers but does not say anything about variables: https://dev.mysql.com/doc/refman/8.3/en/json.html#json-path-syntax * SQLite skimps on details and does not document a grammar: https://sqlite.org/json1.html#path_arguments But it looks as if it strives for compatibility with MySQL and our dear Postgres: https://sqlite.org/src/doc/json-in-core/doc/json-enhancements.md Also checked git log src/backend/utils/adt/jsonpath_scan.l for some insights but haven't found any yet. -- Erik
Re: Q: Escapes in jsonpath Idents
Hi David,
On 2024-03-16 19:39 +0100, David E. Wheeler wrote:
> The jsonpath doc[1] has an excellent description of the format of
> strings, but for unquoted path keys, it simply says:
>
> > Member accessor that returns an object member with the specified
> > key. If the key name matches some named variable starting with $ or
> > does not meet the JavaScript rules for an identifier, it must be
> > enclosed in double quotes to make it a string literal.
>
> I went looking for the JavaScript rules for an identifier and found
> this in the MDN docs[2]:
>
> > In JavaScript, identifiers can contain Unicode letters, $, _, and
> > digits (0-9), but may not start with a digit. An identifier differs
> > from a string in that a string is data, while an identifier is part
> > of the code. In JavaScript, there is no way to convert identifiers
> > to strings, but sometimes it is possible to parse strings into
> > identifiers.
>
>
> However, the Postgres parsing of jsonpath keys appears to follow the
> same rules as strings, allowing backslash escapes:
>
> david=# select '$.fo\u00f8 == $x'::jsonpath;
> jsonpath ---
> ($."foø" == $"x")
>
> This would seem to contradict the documentation. Is this behavior
> required by the SQL standard? Do the docs need updating? Or should the
> code actually follow the JSON identifier behavior?
That quoted MDN page does not give the whole picture. ECMAScript and JS
do allow Unicode escape sequences in identifier names:
https://262.ecma-international.org/#sec-identifier-names
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Lexical_grammar#identifiers
> PS: Those excellent docs on strings mentions support for \v, but the
> grammar in the right nav of https://www.json.org/json-en.html does
> not. Another bonus feature?
You refer to that sentence: "Other special backslash sequences include
those recognized in JSON strings: \b, \f, \n, \r, \t, \v for various
ASCII control characters, and \u for a Unicode character identified
by its 4-hex-digit code point."
Mentioning JSON and \v in the same sentence is wrong: JavaScript allows
that escape in strings but JSON doesn't. I think the easiest is to just
replace "JSON" with "JavaScript" in that sentence to make it right. The
paragraph also already says "embedded string literals follow JavaScript/
ECMAScript conventions", so mentioning JSON seems unnecessary to me.
The last sentence also mentions backslash escapes \xNN and \u{N...} as
deviations from JSON when in fact those are valid escape sequences from
ECMA-262: https://262.ecma-international.org/#prod-HexEscapeSequence
So I think it makes sense to reword the entire backslash part of the
paragraph and remove references to JSON entirely. The attached patch
does that and also formats the backslash escapes as a bulleted list for
readability.
> [1]: https://www.postgresql.org/docs/16/datatype-json.html#DATATYPE-JSONPATH
> [2]: https://developer.mozilla.org/en-US/docs/Glossary/Identifier
On 2024-03-16 21:33 +0100, David E. Wheeler wrote:
> On Mar 16, 2024, at 14:39, David E. Wheeler
> wrote:
>
> > I went looking for the JavaScript rules for an identifier and found
> > this in the MDN docs[2]:
> >
> >> In JavaScript, identifiers can contain Unicode letters, $, _, and
> >> digits (0-9), but may not start with a digit. An identifier differs
> >> from a string in that a string is data, while an identifier is part
> >> of the code. In JavaScript, there is no way to convert identifiers
> >> to strings, but sometimes it is possible to parse strings into
> >> identifiers.
>
> Coda: Dollar signs don’t work at all outside double-quoted string
> identifiers:
>
> david=# select '$.$foo'::jsonpath;
> ERROR: syntax error at or near "$foo" of jsonpath input
> LINE 1: select '$.$foo'::jsonpath;
>^
>
> david=# select '$.f$oo'::jsonpath;
> ERROR: syntax error at or near "$oo" of jsonpath input
> LINE 1: select '$.f$oo'::jsonpath;
>^
>
> david=# select '$."$foo"'::jsonpath;
> jsonpath
> --
> $."$foo"
>
> This, too, contradicts the MDM definition an identifier (and some
> quick browser tests).
The first case ($.$foo) is in line with the restriction on member
accessors that you quoted first.
The error message 'syntax error at or near "$oo" of jsonpath input' for
the second case ($.f$oo), however, looks as if the scanner identifies
'$oo' as a variable instead of contiuing the scan of identifier (f$oo)
for the member accessor. Looks like a bug to me because a variable
doesn't even make sense in that place.
What works th
Re: Patch: Add parse_type Function
On 2024-03-09 02:42 +0100, David E. Wheeler wrote: > On Mar 7, 2024, at 23:39, Erik Wienhold wrote: > > > I think you need to swap the examples. The text mentions the error case > > first and the NULL case second. > > 臘♂️ > > Thanks, fixed in the attached patch. Thanks, LGTM. -- Erik
Re: Patch: Add parse_type Function
Hi David,
On 2024-03-08 02:37 +0100, David E. Wheeler wrote:
> I’ve rebased the patch and, in an attempt to clarify this behavior,
> added a couple of examples to the docs for to_regtype. Updated patch
> attached.
On your latest addition:
> +). Failure to extract a valid potential
> +type name results in an error. For example:
> +
> +SELECT to_regtype('party');
> + to_regtype
> +
> +
> +
> +However, if the extracted name is not known to the system, this
> function
> +will return NULL. For example:
> +
> +SELECT to_regtype('interval nonesuch');
> +ERROR: syntax error at or near "nonesuch"
> +LINE 1: select to_regtype('interval nonesuch');
> + ^
> +CONTEXT: invalid type name "interval nonesuch"
> +
I think you need to swap the examples. The text mentions the error case
first and the NULL case second.
--
Erik
Re: CREATE TABLE creates a composite type corresponding to the table row, which is and is not there
I wrote:
> The attached v2 is a simpler patch that instead modifies the existing
> error message.
Forgot to attach v2.
--
Erik
>From 9ab6b625e8f93ae2957144ec7f0ba32cf8a3bb5b Mon Sep 17 00:00:00 2001
From: Erik Wienhold
Date: Fri, 8 Mar 2024 04:21:56 +0100
Subject: [PATCH v2] Document that typed tables rely on CREATE TYPE
CREATE TABLE OF accepts only composite types that were created with
CREATE TYPE. Clarify that also in the error message.
---
doc/src/sgml/ref/create_table.sgml| 2 ++
src/backend/commands/tablecmds.c | 2 +-
src/test/regress/expected/typed_table.out | 2 +-
3 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/doc/src/sgml/ref/create_table.sgml
b/doc/src/sgml/ref/create_table.sgml
index 4cbaaccaf7..889496cf0a 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -254,6 +254,8 @@ WITH ( MODULUS numeric_literal, REM
schema-qualified). A typed table is tied to its type; for
example the table will be dropped if the type is dropped
(with DROP TYPE ... CASCADE).
+ Expects the composite type to have been created with
+ .
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7014be8039..0f43f349dc 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6979,7 +6979,7 @@ check_of_type(HeapTuple typetuple)
if (!typeOk)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
-errmsg("type %s is not a composite type",
+errmsg("type %s is not a composite type
created with CREATE TYPE",
format_type_be(typ->oid;
}
diff --git a/src/test/regress/expected/typed_table.out
b/src/test/regress/expected/typed_table.out
index 2e47ecbcf5..9edd744f6a 100644
--- a/src/test/regress/expected/typed_table.out
+++ b/src/test/regress/expected/typed_table.out
@@ -89,7 +89,7 @@ drop cascades to function get_all_persons()
drop cascades to table persons2
drop cascades to table persons3
CREATE TABLE persons5 OF stuff; -- only CREATE TYPE AS types may be used
-ERROR: type stuff is not a composite type
+ERROR: type stuff is not a composite type created with CREATE TYPE
DROP TABLE stuff;
-- implicit casting
CREATE TYPE person_type AS (id int, name text);
--
2.44.0
Re: CREATE TABLE creates a composite type corresponding to the table row, which is and is not there
I wrote: > The attached patch fixes the error message and also documents that > requirement. On second thought, adding a separate error message doesn't really make sense. The attached v2 is a simpler patch that instead modifies the existing error message. -- Erik
Re: CREATE TABLE creates a composite type corresponding to the table row, which is and is not there
| integer | | |
Inherits: parent
\d of_child
Table "public.of_child"
Column | Type | Collation | Nullable | Default
+-+---+--+-
a | integer | | |
b | integer | | |
Typed table of type: child
Whereas changing a composite type and its typed tables is possible with
ALTER TYPE ... ADD ATTRIBUTE ... CASCADE.
> # the type definitely is there as promised
>
> hannuk=# create type pair as (a int, b int);
> ERROR: type "pair" already exists
>
> # and I can create similar type wit other name and use it to create table
>
> hannuk=# create type pair2 as (a int, b int);
> CREATE TYPE
>
> hannuk=# create table anoter_pair of pair2;
> CREATE TABLE
>
> # and i can even use it in LIKE
>
> hannuk=# CREATE TABLE pair3(like pair2);
> CREATE TABLE
>
> # the type is present in pg_type with type 'c' for Composite
>
> hannuk=# select typname, typtype from pg_type where typname = 'pair';
> typname | typtype
> -+-
> pair| c
> (1 row)
>
> # and I can add comment to the type
>
> hannuk=# COMMENT ON TYPE pair is 'A Shroedingers type';
> COMMENT
>
> # but \dT does not show it (second case)
>
> hannuk=# \dT pair
> List of data types
> Schema | Name | Description
> +--+-
> (0 rows)
\dT ignores the composite types implicitly created by CREATE TABLE.
[1] https://www.postgresql.org/docs/16/catalog-pg-type.html
--
Erik
>From f279ee1eccd07da14d0ff49f267f6fceffbd0778 Mon Sep 17 00:00:00 2001
From: Erik Wienhold
Date: Fri, 8 Mar 2024 04:21:56 +0100
Subject: [PATCH v1] Document that typed tables rely on CREATE TYPE
CREATE TABLE OF accepts only composite types that were created with
CREATE TYPE. Clarify that also in the error message.
---
doc/src/sgml/ref/create_table.sgml| 2 ++
src/backend/commands/tablecmds.c | 8 +++-
src/test/regress/expected/typed_table.out | 6 +-
src/test/regress/sql/typed_table.sql | 4
4 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/doc/src/sgml/ref/create_table.sgml
b/doc/src/sgml/ref/create_table.sgml
index 4cbaaccaf7..889496cf0a 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -254,6 +254,8 @@ WITH ( MODULUS numeric_literal, REM
schema-qualified). A typed table is tied to its type; for
example the table will be dropped if the type is dropped
(with DROP TYPE ... CASCADE).
+ Expects the composite type to have been created with
+ .
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7014be8039..bef630139d 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6975,8 +6975,14 @@ check_of_type(HeapTuple typetuple)
* the type before the typed table creation/conversion commits.
*/
relation_close(typeRelation, NoLock);
+
+ if (!typeOk)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+errmsg("type %s is not a composite
type created with CREATE TYPE",
+
format_type_be(typ->oid;
}
- if (!typeOk)
+ else
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("type %s is not a composite type",
diff --git a/src/test/regress/expected/typed_table.out
b/src/test/regress/expected/typed_table.out
index 2e47ecbcf5..bb21b69a1a 100644
--- a/src/test/regress/expected/typed_table.out
+++ b/src/test/regress/expected/typed_table.out
@@ -89,8 +89,12 @@ drop cascades to function get_all_persons()
drop cascades to table persons2
drop cascades to table persons3
CREATE TABLE persons5 OF stuff; -- only CREATE TYPE AS types may be used
-ERROR: type stuff is not a composite type
+ERROR: type stuff is not a composite type created with CREATE TYPE
DROP TABLE stuff;
+CREATE TYPE simple AS ENUM ('a');
+CREATE TABLE of_simple OF simple; -- not a composite type
+ERROR: type simple is not a composite type
+DROP TYPE simple;
-- implicit casting
CREATE TYPE person_type AS (id int, name text);
CREATE TABLE persons OF person_type;
diff --git a/src/test/regress/sql/typed_table.sql
b/src/test/regress/sql/typed_table.sql
index 9ef0cdfcc7..eaa11b0f94 100644
--- a/src/test/regress/sql/typed_table.sql
+++ b/src/test/regress/sql/typed_table.sql
@@ -50,6 +50,10 @@ CREATE TABLE persons5 OF stuff; -- only CREATE TYPE AS types
may be used
DROP TABLE stuff;
+CREATE TYPE simple AS ENUM ('a');
+CREATE TABLE of_simple OF simple; -- not a composite type
+DROP TYPE simple;
+
-- implicit casting
--
2.44.0
Re: psql: fix variable existence tab completion
On 2024-03-03 03:00 +0100, Steve Chavez wrote:
> psql has the :{?name} syntax for testing a psql variable existence.
>
> But currently doing \echo :{?VERB doesn't trigger tab completion.
>
> This patch fixes it. I've also included a TAP test.
Thanks. The code looks good, all tests pass, and the tab completion
works as expected when testing manually.
--
Erik
Re: Patch: Add parse_type Function
On 2024-02-21 22:49 +0100, David E. Wheeler wrote: > On Feb 21, 2024, at 16:43, Erik Wienhold wrote: > > > The docs still state that to_regtypemod() has a named parameter, which > > is not the case per pg_proc.dat. > > Bah, I cleaned it up before but somehow put it back. Thanks for the > catch. Fixed. Thanks David! LGTM. -- Erik
Re: Patch: Add parse_type Function
On 2024-02-21 17:51 +0100, David E. Wheeler wrote: > On Feb 21, 2024, at 11:18, Erik Wienhold wrote: > > > Thanks. But it's an applefile again, not a patch :P > > Let’s try that again. Thanks. > +to_regtypemod ( type > text ) The docs still state that to_regtypemod() has a named parameter, which is not the case per pg_proc.dat. Besides that, LGTM. -- Erik
Re: Patch: Add parse_type Function
On 2024-02-21 16:14 +0100, David E. Wheeler wrote: > > V8 attached. Thanks. But it's an applefile again, not a patch :P -- Erik
Re: Patch: Add parse_type Function
On 2024-02-20 15:44 +0100, David E. Wheeler wrote:
> I’ve tweaked the comments and their order in v7, attached.
>
> This goes back to the discussion of the error raising of
> to_regtype[1], so I’ve incorporated the patch from that thread into
> this patch, and set up the docs for to_regtypemod() with similar
> information.
Makes sense.
> [1]
> https://www.postgresql.org/message-id/flat/57E1FDDC-5A38-452D-82D7-A44DA2E13862%40justatheory.com#1ae0b11634bc33c7ad3cd728e43d504e
> -
> +role="func_signature">
>
> format_type
>
> @@ -25462,7 +25462,7 @@ SELECT collation for ('foo' COLLATE "de_DE");
>
>
>
> -
> +role="func_signature">
The references are printed as "???" right now. Can be fixed with
xreflabel="format_type" and xreflabel="to_regtype" on those
elements.
> +to_regtypemod ( type
> text )
The docs show parameter name "type" but pg_proc.data does not define
proargnames. So the to_regtypemod() cannot be called using named
notation:
test=> select to_regtypemod(type => 'int'::text);
ERROR: function to_regtypemod(type => text) does not exist
> +Parses a string of text, extracts a potential type name from it, and
> +translates its typmod, the modifier for the type, if any. Failure to
s/typmod, the modifier of the type/type modifier/
Because format_type() already uses "type modifier" and I think it helps
searchability to use the same wording.
--
Erik
Re: Patch: Add parse_type Function
On 2024-02-19 23:59 +0100, David E. Wheeler wrote:
> On Feb 19, 2024, at 15:47, Tom Lane wrote:
>
> >> 1. Add a to_regtypmod() for those who just want the typemod.
> >
> > Seems like there's a good case for doing that.
>
> I’ll work on that.
See the patch I wrote for my benchmarks. But it's pretty easy anyway to
cut down parse_type() ;)
> > I'm less thrilled about that, mainly because I can't think of a good
> > name for it ("format_type_string" is certainly not that). Is the
> > use-case for this functionality really strong enough that we need to
> > provide it as a single function rather than something assembled out
> > of spare parts?
>
> Not essential for pgTAP, no, as we can just use the parts. It was the
> typmod bit that was missing.
But you don't actually need reformat_type() in pgTAP. You can just get
the type OID and modifier of the want_type and have_type and compare
those. Then use format_type() for the error message. Looks a bit
cleaner to me than doing the string comparison.
On second thought, I guess comparing the reformatted type names is
necessary in order to have a uniform API on older Postgres releases
where pgTAP has to provide its own to_regtypmod() based on typmodin
functions.
> On Feb 19, 2024, at 17:13, Tom Lane wrote:
>
> > After some time ruminating, a couple of possibilities occurred to
> > me: reformat_type(text) canonical_type_name(text)
>
> I was just thinking about hitting the thesaurus entry for “canonical”,
> but I quite like reformat_type. It’s super clear and draws the
> parallel to format_type() more clearly. Will likely steal the name for
> pgTAP if we don’t add it to core. :-)
>
> > I'm still unconvinced about that, though.
>
> I guess the questions are:
>
> * What are the other use cases for it?
Can't think of any right now other than this are-type-names-the-same
check. Perhaps also for pretty-printing user-provided type names where
we don't take the type info from e.g. pg_attribute.
> * How obvious is it how to do it?
>
> For the latter, it could easily be an example in the docs.
Can be mentioned right under format_type().
--
Erik
Re: Patch: Add parse_type Function
On 2024-02-18 20:00 +0100, Pavel Stehule wrote:
> The overhead of parse_type_and_format can be related to higher planning
> time. PL/pgSQL can assign composite without usage FROM clause.
Thanks, didn't know that this makes a difference. In that case both
variants are on par.
BEGIN;
CREATE FUNCTION format_with_parse_type(text)
RETURNS text
LANGUAGE plpgsql
STABLE STRICT
AS $$
DECLARE
p record := parse_type($1);
BEGIN
RETURN format_type(p.typid, p.typmod);
END
$$;
CREATE FUNCTION format_with_to_regtypmod(text)
RETURNS text
LANGUAGE plpgsql
STABLE STRICT
AS $$
BEGIN
RETURN format_type(to_regtype($1), to_regtypmod($1));
END
$$;
COMMIT;
Results:
SELECT format_with_parse_type('interval second(0)');
pgbench (17devel)
transaction type: format_with_parse_type.sql
scaling factor: 1
query mode: simple
number of clients: 1
number of threads: 1
maximum number of tries: 1
duration: 10 s
number of transactions actually processed: 253530
number of failed transactions: 0 (0.000%)
latency average = 0.039 ms
initial connection time = 1.846 ms
tps = 25357.551681 (without initial connection time)
SELECT format_with_to_regtypmod('interval second(0)');
pgbench (17devel)
transaction type: format_with_to_regtypmod.sql
scaling factor: 1
query mode: simple
number of clients: 1
number of threads: 1
maximum number of tries: 1
duration: 10 s
number of transactions actually processed: 257942
number of failed transactions: 0 (0.000%)
latency average = 0.039 ms
initial connection time = 1.544 ms
tps = 25798.015526 (without initial connection time)
--
Erik
Re: Patch: Add parse_type Function
On 2024-02-12 19:20 +0100, Tom Lane wrote:
> I wrote:
> > It strikes me that this is basically to_regtype() with the additional
> > option to return the typmod. That leads to some questions:
>
> BTW, another way that this problem could be approached is to use
> to_regtype() as-is, with a separate function to obtain the typmod:
>
> select format_type(to_regtype('timestamp(4)'), to_regtypmod('timestamp(4)'));
>
> This is intellectually ugly, since it implies parsing the same
> typename string twice. But on the other hand it avoids the notational
> pain and runtime overhead involved in using a record-returning
> function. So I think it might be roughly a wash for performance.
> Question to think about is which way is easier to use. I don't
> have an opinion particularly; just throwing the idea out there.
Out of curiosity, I benchmarked this with the attached to_regtypmod()
patch based on David's v5 applied to a6c21887a9. The script running
pgbench and its output are included at the end.
Just calling parse_type() vs to_regtype()/to_regtypmod() is a wash for
performance as you thought. But format_type() performs better with
to_regtypmod() than with parse_type(). Accessing the record fields
returned by parse_type() adds some overhead.
to_regtypmod() is better for our use case in pgTAP which relies on
format_type() to normalize the type name. The implementation of
to_regtypmod() is also simpler than parse_type(). Usage-wise, both are
clunky IMO.
Benchmark script:
#!/usr/bin/env bash
set -eu
cat <<'SQL' > parse_type.sql
SELECT parse_type('interval second(0)');
SQL
cat <<'SQL' > parse_type_and_format.sql
SELECT format_type(p.typid, p.typmod) FROM parse_type('interval
second(0)') p;
SQL
cat <<'SQL' > to_regtypmod.sql
SELECT to_regtype('interval second(0)'), to_regtypmod('interval
second(0)');
SQL
cat <<'SQL' > to_regtypmod_and_format.sql
SELECT format_type(to_regtype('interval second(0)'),
to_regtypmod('interval second(0)'));
SQL
for f in \
parse_type.sql \
parse_type_and_format.sql \
to_regtypmod.sql \
to_regtypmod_and_format.sql
do
pgbench -n -f "$f" -T10 postgres
echo
done
pgbench output:
pgbench (17devel)
transaction type: parse_type.sql
scaling factor: 1
query mode: simple
number of clients: 1
number of threads: 1
maximum number of tries: 1
duration: 10 s
number of transactions actually processed: 277017
number of failed transactions: 0 (0.000%)
latency average = 0.036 ms
initial connection time = 1.623 ms
tps = 27706.005513 (without initial connection time)
pgbench (17devel)
transaction type: parse_type_and_format.sql
scaling factor: 1
query mode: simple
number of clients: 1
number of threads: 1
maximum number of tries: 1
duration: 10 s
number of transactions actually processed: 222487
number of failed transactions: 0 (0.000%)
latency average = 0.045 ms
initial connection time = 1.603 ms
tps = 22252.095670 (without initial connection time)
pgbench (17devel)
transaction type: to_regtypmod.sql
scaling factor: 1
query mode: simple
number of clients: 1
number of threads: 1
maximum number of tries: 1
duration: 10 s
number of transactions actually processed: 276134
number of failed transactions: 0 (0.000%)
latency average = 0.036 ms
initial connection time = 1.570 ms
tps = 27617.628259 (without initial connection time)
pgbench (17devel)
transaction type: to_regtypmod_and_format.sql
scaling factor: 1
query mode: simple
number of clients: 1
number of threads: 1
maximum number of tries: 1
duration: 10 s
number of transactions actually processed: 270820
number of failed transactions: 0 (0.000%)
latency average = 0.037 ms
initial connection time = 1.631 ms
tps = 27086.331104 (without initial connection time)
--
Erik
>From 0b60432a84d63a7fccaae0fe123a0aa2ae67493b Mon Sep 17 00:00:00 2001
From: Erik Wienhold
Date: Sun, 18 Feb 2024 17:33:35 +0100
Subject: [PATCH] Add to_regtypmod() for benchmarking against parse_type()
---
src/backend/utils/adt/regproc.c | 18 ++
src/include/catalog/pg_proc.dat | 3 ++
src/test/regress/expected/regproc.out | 51 +++
src/test/regress/sql/regproc.sql | 26 ++
4 files changed, 98 insertions(+)
diff --git a/s
Re: Patch: Add parse_type Function
Let me comment on some issues since I wrote the very first version of
parse_type() on which David's patch is based.
> On 2024-02-01 01:00 +0100, jian he wrote:
>
> cosmetic issue. Looking around other error handling code, the format
> should be something like:
> `
> if (get_call_result_type(fcinfo, NULL, ) != TYPEFUNC_COMPOSITE)
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("function returning record called in"
> "context that cannot accept type record")));
> `
+1
> `PG_FUNCTION_INFO_V1(parse_type);`
> is unnecessary?
> based on the error information: https://cirrus-ci.com/task/5899457502380032
> not sure why it only fails on windows.
Yeah, it's not necessary for internal functions per [1]. It's a
leftover from when this function was part of the pgtap extension.
> +#define PARSE_TYPE_STRING_COLS 2 /* Returns two columns. */
> +#undef PARSE_TYPE_STRING_COLS
> Are these necessary?
> given that the comments already say return the OID and type modifier.
Not sure what the convention here is but I found this in a couple of
places, maybe even a tutorial on writing C functions. See
`git grep '_COLS\s\+[1-9]'` for those instances. It's in line with my
habit of avoiding magic constants.
> +( typid oid,
> + typmod int4 )
> here `int4` should be `integer`?
+1
> commit message:
> `Also accounts for data typs that require the SQL grammar to be parsed:`
> except the typo issue, this sentence is still hard for me to understand.
Yes, the sentence is rather handwavy. What is meant here is that this
function also resolves types whose typmod is determined by the SQL
parser or some step after that. There are types whose typmod is
whatever value is found inside the parenthesis, e.g. bit(13) has typmod
13. Our first idea before coming up with that function was to do some
string manipulation and extract the typmod from inside the parenthesis.
But we soon found out that other typmods don't translate one to one,
e.g. varchar(13) has typmod 17. The interval type is also special
because the typmod is some bitmask encoding of e.g. 'second(0)'. Hence
the mention of the SQL grammar.
> +
> +Parses a string representing an SQL type declaration as used in a
> +CREATE TABLE statement, optionally
> schema-qualified.
> +Returns a record with two fields, typid and
> +typmod, representing the OID and
> modifier for the
> +type. These correspond to the parameters to pass to the
> +format_type
> function.
> +
>
> can be simplified:
> +
> +Parses a string representing an SQL data type, optionally
> schema-qualified.
> +Returns a record with two fields, typid and
> +typmod, representing the OID and
> modifier for the
> +type. These correspond to the parameters to pass to the
> +format_type
> function.
> +
> (I don't have a strong opinion though)
Sounds better. The CREATE TABLE reference is superfluous.
[1] https://www.postgresql.org/docs/current/xfunc-c.html#XFUNC-C-V1-CALL-CONV
--
Erik
Re: Psql meta-command conninfo+
On 2024-02-08 20:37 +0100, Jim Jones wrote: > One thing I just noticed. The psql autocomplete feature does not suggest > the new + option of \conninfo. For instance, when typing "\connin[TAB]" > it automatically autocompletes to "\conninfo ". I guess it should also > be included in this patch. Modifiers such as + or S in \dS are not covered by autocompletion. src/bin/psql/tab-complete.c only specifies backslash commands in their basic form (without modifiers). (\dS actually autocompletes to \ds to my surprise) > I can do a more thorough review of the code when you add the > documentation and tests to the patch. I noticed that the pattern parameter in listConnectionInformation is unused. exec_command_conninfo scans the pattern but \conninfo should not accept any argument. So the pattern can be removed entirely. -- Erik
Re: Psql meta-command conninfo+
On 2024-02-07 05:13 +0100, Maiquel Grassi wrote:
> On Tue, Feb 06, 2024 at 09:45:54PM +, Maiquel Grassi wrote:
> > My initial idea has always been that they should continue to appear
> > because \conninfo+ should show all the things that \conninfo shows and
> > add more information. I think that's the purpose of the 'plus.' Now we're
> > on a better path than the initial one. We can still add the socket
> > directory and the host.
>
> Agreed.
>
> --//--
>
> I believe it's resolved reasonably well this way:
>
> SELECT
> pg_catalog.current_database() AS "Database",
> current_user AS "User",
> pg_catalog.current_setting('server_version') AS "Server Version",
> CASE
> WHEN pg_catalog.inet_server_addr() IS NULL
> THEN 'NULL'
> ELSE pg_catalog.inet_server_addr()::text
> END AS "Server Address",
Should be NULL instead of string 'NULL'. So the entire CASE expression
is redundant and you can just return pg_catalog.inet_server_addr().
> pg_catalog.current_setting('port') AS "Port",
> CASE
> WHEN pg_catalog.inet_client_addr() IS NULL
> THEN 'NULL'
> ELSE pg_catalog.inet_client_addr()::text
> END AS "Client Address",
> CASE
> WHEN pg_catalog.inet_client_port() IS NULL
> THEN 'NULL'
> ELSE pg_catalog.inet_client_port()::text
> END AS "Client Port",
Same here.
> pg_catalog.pg_backend_pid() AS "Session PID",
> CASE
> WHEN pg_catalog.current_setting('unix_socket_directories') = ''
> THEN 'NULL'
> ELSE pg_catalog.current_setting('unix_socket_directories')
> END AS "Socket Directory",
The CASE expression can be simplified to:
nullif(pg_catalog.current_setting('unix_socket_directories'), '')
> CASE
> WHEN
> pg_catalog.inet_server_addr() IS NULL
> AND pg_catalog.inet_client_addr() IS NULL
> THEN 'NULL'
> WHEN
> pg_catalog.inet_server_addr() = pg_catalog.inet_client_addr()
> THEN 'localhost'
Is it safe to assume localhost here? \conninfo prints localhost only
when I connect with psql -hlocalhost:
$ psql -hlocalhost postgres
psql (16.1)
postgres=# \conninfo
You are connected to database "postgres" as user "ewie" on host
"localhost" (address "::1") at port "5432".
postgres=# \q
$ psql -h127.0.0.1 postgres
psql (16.1)
postgres=# \conninfo
You are connected to database "postgres" as user "ewie" on host
"127.0.0.1" at port "5432".
> ELSE pg_catalog.inet_server_addr()::text
> END AS "Host";
--
Erik
Re: Psql meta-command conninfo+
On 2024-02-06 19:19 +0100, Nathan Bossart wrote: > On Tue, Feb 06, 2024 at 05:27:01PM +, Maiquel Grassi wrote: > > postgres=# \conninfo+ > > Current Connection Information > >Attribute| Value > > + > > Database | postgres > > User | postgres > > Server Version | 16.1 > > Server Address | 192.168.0.5/32 > > Server Port| 5433 > > Client Address | 192.168.0.5/32 > > Client Port| 52716 > > Session PID| 21624 > > (8 rows) > > My first reaction is that this should instead return a single row with the > same set of columns for any connection type (the not-applicable ones would > just be set to NULL). That would match the other meta-commands like \l and > \du, and you could still get an expanded display with \x if needed. Also, > I think it would simplify the code a bit. +1 for a single-row result and triggering expanded display with \x for consistency with other commands. -- Erik
Re: to_regtype() Raises Error
On 2024-02-04 20:20 +0100, David E. Wheeler wrote:
> On Feb 2, 2024, at 15:33, David E. Wheeler wrote:
>
> > Anyway, I’m happy to submit a documentation patch along the lines you
> > suggested.
>
> How’s this?
>
> --- a/doc/src/sgml/func.sgml
> +++ b/doc/src/sgml/func.sgml
> @@ -25460,11 +25460,12 @@ SELECT collation for ('foo' COLLATE "de_DE");
> regtype
>
>
> -Translates a textual type name to its OID. A similar result is
> +Parses a string of text, extracts a potential type name from it, and
> +translates that name into an OID. A similar result is
> obtained by casting the string to type regtype (see
> -); however, this function will return
> -NULL rather than throwing an error if the name is
> -not found.
> +). Failure to extract a valid potential
> +type name results in an error; however, if the extracted names is not
Here "extracted names" should be "extracted name" (singular).
Otherwise, the text looks good.
> +known to the system, this function will return
> NULL.
>
>
>
>
> Does similar wording need to apply to other `to_reg*` functions?
Just to_regtype() is fine IMO. The other to_reg* functions don't throw
errors on similar input, e.g.:
test=> select to_regproc('foo bar');
to_regproc
(1 row)
--
Erik
Re: Re: Patch: Improve Boolean Predicate JSON Path Docs
On 2024-01-19 22:15 +0100, Tom Lane wrote:
> "David E. Wheeler" writes:
> > [ v7-0001-Improve-boolean-predicate-JSON-Path-docs.patch ]
>
> + \set json '{
>"track": {
> "segments": [
>{
>
> I find the textual change rather unwieldy, but the bigger problem is
> that this example doesn't actually work. If you try to copy-and-paste
> this into psql, you get "unterminated quoted string", because psql
> metacommands can't span line boundaries.
Interesting... copy-pasting the entire \set command works for me with
psql 16.1 in gnome-terminal and tmux. Typing it out manually gives me
the "unterminated quoted string" error. Maybe has to do with my stty
settings.
> I experimented with
>
> SELECT '
> ... multiline json value ...
> ' AS json
> \gexec
>
> but that didn't seem to work either. Anybody have a better idea?
Fine with me (the \gset variant).
--
Erik
Re: Issue with launching PGAdmin 4 on Mac OC
On 2023-11-14 18:13 +0100, Kanmani Thamizhanban wrote: > Good day! I'm not able to launch Postgres PGAdmin 4 in my MAC OS, I have > tried with both versions 15 and 16, but nothing is working. It says that it > has quit unexpectedly (screenshot attached). I have attached the bug report > as well along with my system specifications below. Kindly help me with this > ASAP. It's very unlikely that you'll get any pgAdmin support on this list which is for Postgres development only. Unless your Postgres server also crashed but the attached bug report doesn't show that. Please create an issue on GitHub or post to the pgadmin-support list instead. https://github.com/pgadmin-org/pgadmin4/issues https://www.postgresql.org/list/pgadmin-support/ -- Erik
Re: Fix output of zero privileges in psql
On 2023-11-13 21:49 +0100, Tom Lane wrote: > Patch pushed with minor adjustments, mainly rewriting some comments. Thanks a lot! -- Erik
Re: Fix output of zero privileges in psql
On 2023-11-09 20:19 +0100, Tom Lane wrote: > Laurenz Albe writes: > > Thanks for the feedback. I'll set the patch to "ready for committer" then. > > So, just to clarify, we're settling on your v4 from [1]? > > [1] > https://www.postgresql.org/message-id/d799f996f422231a99655f1223667d6d887e4c95.ca...@cybertec.at Yes from my side. -- Erik
Re: Fix output of zero privileges in psql
On 2023-11-08 13:23 +0100, Laurenz Albe wrote: > I wonder how to proceed with this patch. The main disagreement is > whether default privileges should be displayed as NULL (less invasive, > but more confusing for beginners) or "(default)" (more invasive, > but nicer for beginners). Are there any reports from beginners being confused about default privileges being NULL or being displayed as a blank string in psql? This is usually resolved with a pointer to the docs if it comes up in discussions or the user makes the mental leap and checks the docs himself. Both patches add some details to the docs to explain psql's output. > David is for "(default)", Tom and me are for NULL, and I guess Erik > would also prefer "(default)", since that was how his original > patch did it, IIRC. I think I could live with both solutions. > > Kind of a stalemate. Who wants to tip the scales? Yes I had a slight preference for my patch but I'd go with yours (\pset null) now. I followed the discussion after my last mail but had nothing more to add that wasn't already said. Tom then wrote that NULL is the catalog's representation for the default privileges and obscuring that fact in psql is not doing any service to the users. This convinced me because users may have to deal with aclitem[] being NULL anyway at some point if they need to check privileges in more detail. So it makes absolutely sense that psql is transparent about that. -- Erik
Re: Patch: Improve Boolean Predicate JSON Path Docs
On 2023-10-24 00:58 +0200, David E. Wheeler wrote: > On Oct 22, 2023, at 20:36, Erik Wienhold wrote: > > > That's an AppleSingle file according to [1][2]. It only contains the > > resource fork and file name but no data fork. > > Ah, I had “Send large attachments with Mail Drop” enabled. To me 20K > is not big but whatever. Let’s see if turning it off fixes the issue. I suspected it had something to do with iCloud. Glad you solved it! > > Please change to "in strict mode" (without "the"). > > Hrm, I prefer it without the article, too, but it is consistently used > that way elsewhere, like here: > > > https://github.com/postgres/postgres/blob/5b36e8f/doc/src/sgml/func.sgml#L17401 > > I’d be happy to change them all, but was keeping it consistent for now. Right. I haven't really noticed that the article case is more common. I thought that you may have missed that one because I saw this change that removes the article: > -In the strict mode, the specified path must exactly match the structure > of > +In strict mode, the specified path must exactly match the structure of > Updated patch attached, thank you! LGTM. Would you create a commitfest entry? I'll set the status to RfC. -- Erik
Re: Patch: Improve Boolean Predicate JSON Path Docs
On 2023-10-20 15:49 +0200, David Wheeler wrote: > On Oct 19, 2023, at 23:49, Erik Wienhold wrote: > > > I don't even know what that represents, probably not some fancy file > > compression. That's an AppleSingle file according to [1][2]. It only contains the resource fork and file name but no data fork. > Oh, weird. Trying from a webmail client instead. Thanks. > +Does JSON path return any item for the specified JSON value? Use only > +SQL-standard JSON path expressions, not > +predicate check > +expressions. Any reason for calling it "predicate check expressions" (e.g. the link text) and sometimes "predicate path expressions" (e.g. the linked section title)? I think it should be named consistently to avoid confusion and also to simplify searching. > +Returns the result of a JSON path > +predicate > +check for the specified JSON value. If the result is not > Boolean, > +then NULL is returned. Use only with > +predicate check > +expressions. Linking the same section twice in the same paragraph seems excessive. > += select jsonb_path_query(:'json', > '$.track.segments'); > +select jsonb_path_query(:'json', '$.track.segments'); Please remove the second SELECT. > += select jsonb_path_query(:'json', 'strict > $.track.segments[0].location'); > + jsonb_path_query > +--- > + [47.763, 13.4034] Strict mode is unnecessary to get that result and I'd omit it because the different modes are not introduced yet at this point. > += select jsonb_path_query(:'json', 'strict > $.track.segments.size()'); > + jsonb_path_query > +-- > + 2 Strict mode is unnecessary here as well. > + using the lax mode. To avoid surprising results, we recommend using > + the .** accessor only in the strict mode. The Please change to "in strict mode" (without "the"). [1] https://www.rfc-editor.org/rfc/rfc1740.txt [2] https://web.archive.org/web/20180311140826/http://kaiser-edv.de/documents/AppleSingle_AppleDouble.pdf -- Erik
Re: Fix output of zero privileges in psql
On 2023-10-20 22:35 +0200, David G. Johnston wrote:
> Ok, I found my mis-understanding and better understand where you are all
> coming from now; I apparently had the usage of NULL flip-flopped.
>
> Taking pg_tablespace as an example. Its "spcacl" column produces NULL for
> default privileges and '{}'::text[] for empty privileges.
>
> Thus, today:
> empty: array_to_string('{}'::text[], '\n') produces an empty string
> default: array_to_string(null, '\n') produces null which then was printed
> as a hard-coded empty string via forcibly changing \pset null
>
> I was thinking the two cases were reversed.
>
> My proposal for changing printACLColumn is thus:
>
> case when spcacl is null then ''
> when cardinality(spcacl) = 0 then '(none)'
> else array_to_string(spcacl, E'\\n')
> end as "Access privileges"
>
> In short, I don't want default privileges to start to obey \pset null when
> it never has before and is documented as displaying the empty string. I do
> want the empty string produced by empty privileges to change to (none) so
> that it no longer is indistinguishable from our choice of presentation for
> the default privilege case.
>
> Mechanically, we remove the existing \pset null for these metacommands and
> move it into the query via the added CASE expression in the printACLColumn
> function.
>
> This gets rid of NULL as an output value for this column and makes the
> patch regarding \pset null discussion unnecessary. And it leaves the
> existing well-established empty string for default privileges alone (and
> changing this is what I believe Tom is against and I agree on that point).
I haven't thought off this yet. The attached v3 of my initial patch
does that. It also includes Laurenz' fix to no longer ignore \pset null
(minus the doc changes that suggest using \pset null to distinguish
between default and empty privileges because that's no longer needed).
--
Erik
>From 5e3f1840a5a6f49ccfa7f61c040b24638daad421 Mon Sep 17 00:00:00 2001
From: Erik Wienhold
Date: Sun, 17 Sep 2023 20:54:48 +0200
Subject: [PATCH v3] Fix output of empty privileges in psql
Print "(none)" for empty privileges instead of nothing so that the user
is able to distinguish them from default privileges. \pset null was
ignored to always print default privileges as empty strings. This
kludge is now removed by explicitly returning the empty string for
default privileges with the nice side effect that all describe commands
now honor \pset null.
Meta commands affected by empty privileges:
\db+ \dD+ \des+ \dew+ \df+ \dL+ \dl+ \dn+ \dp \dT+ \l
Default privileges start as NULL::aclitem[] in various catalog columns
but revoking the default privileges leaves an empty aclitem[]. Using
\pset null '(default)' as a workaround for spotting default privileges
did not work because the meta commands ignored this setting.
The privileges shown by \dconfig+ and \ddp as well as the column
privileges shown by \dp are not affected by this change because the
respective aclitem[] is reset to NULL or deleted from the catalog
instead of leaving empty arrays.
Commands \des+ and \dew+ are not covered in src/test/regress because no
foreign data wrapper is available at this point to create a foreign
server.
Handling of empty privileges by Erik Wienhold. Fixing \pset null by
Laurenz Albe.
---
doc/src/sgml/ddl.sgml | 4 +-
src/bin/psql/describe.c| 51 +++-
src/test/regress/expected/psql.out | 94 ++
src/test/regress/sql/psql.sql | 45 ++
4 files changed, 149 insertions(+), 45 deletions(-)
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 075ff32991..1c17c4a967 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2353,7 +2353,9 @@ GRANT SELECT (col1), UPDATE (col1) ON mytable TO
miriam_rw;
If the Access privileges column is empty for a given
object, it means the object has default privileges (that is, its
- privileges entry in the relevant system catalog is null). Default
+ privileges entry in the relevant system catalog is null). The column shows
+ (none) for empty privileges (that is, no privileges at
+ all, even for the object owner a rare occurrence). Default
privileges always include all privileges for the owner, and can include
some privileges for PUBLIC depending on the object
type, as explained above. The first GRANT
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index bac94a338c..0d7f86d80f 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -124,7 +124,6 @@ describeAggregates(const char *pattern, bool verbose, bool
showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of aggregate functions");
myopt
Re: Fix output of zero privileges in psql
On 2023-10-20 08:42 +0200, Laurenz Albe wrote:
> If you want to proceed with your patch, you could send a new version.
v2 attached.
> I think it could do with an added line of documentation to the
> "Privileges" chapter, and I'd say that the regression tests should be
> in "psql.sql" (not that it is very important).
I added some docs. There will be merge conflicts when combining with
your v5! Also moved the regression tests to psql.sql which is the right
place for testing meta commands.
--
Erik
>From 170ca82fa7b74cc9102582cdf48042131d5ae016 Mon Sep 17 00:00:00 2001
From: Erik Wienhold
Date: Sun, 17 Sep 2023 20:54:48 +0200
Subject: [PATCH v2] Fix output of empty privileges in psql
Print "(none)" for empty privileges instead of nothing so that the user
is able to distinguish empty from default privileges. This affects the
following meta commands:
\db+ \dD+ \des+ \dew+ \df+ \dL+ \dl+ \dn+ \dp \dT+ \l
Default privileges start as NULL::aclitem[] in various catalog columns
but revoking the default privileges leaves an empty aclitem array.
Using \pset null '(null)' as a workaround to spot default privileges
does not work because the meta commands ignore this setting.
The privileges shown by \dconfig+ and \ddp as well as the column
privileges shown by \dp are not affected by this change because the
respective aclitem[] is reset to NULL or deleted from the catalog
instead of leaving empty arrays.
Commands \des+ and \dew+ are not covered in src/test/regress because no
foreign data wrapper is available at this point to create a foreign
server.
---
doc/src/sgml/ddl.sgml | 4 +-
src/bin/psql/describe.c| 6 +-
src/test/regress/expected/psql.out | 94 ++
src/test/regress/sql/psql.sql | 45 ++
4 files changed, 147 insertions(+), 2 deletions(-)
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 075ff32991..1c17c4a967 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2353,7 +2353,9 @@ GRANT SELECT (col1), UPDATE (col1) ON mytable TO
miriam_rw;
If the Access privileges column is empty for a given
object, it means the object has default privileges (that is, its
- privileges entry in the relevant system catalog is null). Default
+ privileges entry in the relevant system catalog is null). The column shows
+ (none) for empty privileges (that is, no privileges at
+ all, even for the object owner a rare occurrence). Default
privileges always include all privileges for the owner, and can include
some privileges for PUBLIC depending on the object
type, as explained above. The first GRANT
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index bac94a338c..154d244d97 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -6718,7 +6718,11 @@ static void
printACLColumn(PQExpBuffer buf, const char *colname)
{
appendPQExpBuffer(buf,
- "pg_catalog.array_to_string(%s,
E'\\n') AS \"%s\"",
+ "CASE pg_catalog.cardinality(%s)\n"
+ " WHEN 0 THEN '%s'\n"
+ " ELSE
pg_catalog.array_to_string(%s, E'\\n')\n"
+ "END AS \"%s\"",
+ colname, gettext_noop("(none)"),
colname, gettext_noop("Access
privileges"));
}
diff --git a/src/test/regress/expected/psql.out
b/src/test/regress/expected/psql.out
index c70205b98a..08f854d0a8 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -6663,3 +6663,97 @@ DROP ROLE regress_du_role0;
DROP ROLE regress_du_role1;
DROP ROLE regress_du_role2;
DROP ROLE regress_du_admin;
+-- Test empty privileges.
+BEGIN;
+WARNING: there is already a transaction in progress
+-- Create an owner for tested objects because output contains owner info.
+-- Must be superuser to be owner of tablespace.
+CREATE ROLE regress_zeropriv_owner SUPERUSER;
+SET LOCAL ROLE regress_zeropriv_owner;
+ALTER TABLESPACE regress_tblspace OWNER TO CURRENT_USER;
+REVOKE ALL ON TABLESPACE regress_tblspace FROM CURRENT_USER;
+\db+ regress_tblspace
+List of tablespaces
+ Name | Owner |Location | Access
privileges | Options | Size | Description
+--++-+---+-+-+-
+ regress_tblspace | regress_zeropriv_owner | pg_tblspc/16385 | (none)
| | 0 bytes |
+(1 row)
+
+CREATE DOMAIN regress_zeropriv_domain AS int;
+REVOKE ALL ON DOMAIN regress_zeropriv_domain FROM CURRENT_USER, PUBLIC;
+\dD+ regress_zeropr
Re: Patch: Improve Boolean Predicate JSON Path Docs
On 2023-10-20 05:20 +0200, David E. Wheeler wrote: > On Oct 19, 2023, at 10:49 PM, Erik Wienhold wrote: > > > Just wanted to take a look at v5. But it's an applefile again :P > > I don’t get it. It was the other times too! Are you able to save it > with a .patch suffix? Saving it is not the problem, but the actual file contents: $ xxd v5-0001-Improve-boolean-predicate-JSON-Path-docs.patch : 0005 1600 0002 0010: 0002 0009 0020: 0032 000a 0003 003c .2...<.. 0030: 0036 7635 2d30 .6..v5-0 0040: 3030 312d 496d 7072 6f76 652d 626f 6f6c 001-Improve-bool 0050: 6561 6e2d 7072 6564 6963 6174 652d 4a53 ean-predicate-JS 0060: 4f4e 2d50 6174 682d 646f 6373 2e70 6174 ON-Path-docs.pat 0070: 6368 ch I don't even know what that represents, probably not some fancy file compression. -- Erik
Re: Patch: Improve Boolean Predicate JSON Path Docs
On 2023-10-19 15:39 +0200, David E. Wheeler wrote: > On Oct 19, 2023, at 01:22, jian he wrote: > > Updated patch attached and also on GitHub. > > > https://github.com/postgres/postgres/compare/master...theory:postgres:jsonpath-pred-docs Just wanted to take a look at v5. But it's an applefile again :P -- Erik
Re: Fix output of zero privileges in psql
On 2023-10-17 04:05 +0200, David G. Johnston wrote: > Erik seems to favors (none) Yes, with a slight favor for "(none)" because it's the least disruptive to users who change \pset null to a non-blank string. The output of \dp etc. would still look the same for default privileges. But I'm also okay with respecting \pset null because it is so simple. We will no longer hide the already documented null representation of default privileges by allowing the user to display the ACL as it is. And with Laurenz' patch we will also document the special case of zero privileges and how to distinguish it. -- Erik
Re: Patch: Improve Boolean Predicate JSON Path Docs
On 2023-10-16 21:59 +0200, David E. Wheeler write: > On Oct 15, 2023, at 23:03, Erik Wienhold wrote: > > > Your call but I'm not against including it in this patch because it > > already touches the modes section. > > Okay, added, let’s just put all our cards on the table. :-) I'll have a look but the attached v3 is not a patch but some applefile. > Thanks, got it down to one: > > postgres.sgml:112: element sect4: validity error : Element sect4 content does > not follow the DTD, expecting (sect4info? , (title , subtitle? , > titleabbrev?) , (toc | lot | index | glossary | bibliography)* , > (((calloutlist | glosslist | bibliolist | itemizedlist | orderedlist | > segmentedlist | simplelist | variablelist | caution | important | note | tip > | warning | literallayout | programlisting | programlistingco | screen | > screenco | screenshot | synopsis | cmdsynopsis | funcsynopsis | classsynopsis > | fieldsynopsis | constructorsynopsis | destructorsynopsis | methodsynopsis | > formalpara | para | simpara | address | blockquote | graphic | graphicco | > mediaobject | mediaobjectco | informalequation | informalexample | > informalfigure | informaltable | equation | example | figure | table | msgset > | procedure | sidebar | qandaset | task | anchor | bridgehead | remark | > highlights | abstract | authorblurb | epigraph | indexterm | beginpage)+ , > (refentry* | sect5* | simplesect*)) | refentry+ | sect5+ | simplesect+) , > (toc | lot | index | glossary | bibliography)*), got (para para ) > One of the added is invalid by the looks of it. Maybe is missing because it says "got (para para )" at the end. -- Erik
Re: Fix output of zero privileges in psql
On 2023-10-16 17:56 +0200, Laurenz Albe write: > David, how do you feel about this? I am wondering whether to set this patch > "ready for committer" or not. > > There is Tom wondering upthread whether changing psql's behavior like that > is too much of a compatibility break or not, but I guess it is alright to > leave that final verdict to a committer. What's the process for the CommitFest now since we settled on your patch? This is my first time being involved in this, so still learning. I'see that you've withdrawn your initial patch [1] but this thread is still attached to my patch [2]. Should I edit my CF entry (or withdraw it) and you reactivate yours? [1] https://commitfest.postgresql.org/45/4603/ [2] https://commitfest.postgresql.org/45/4593/ -- Erik
Re: Patch: Improve Boolean Predicate JSON Path Docs
On 2023-10-16 01:04 +0200, David E. Wheeler write:
> On Oct 14, 2023, at 19:51, Erik Wienhold wrote:
>
> > Thanks for putting this together. See my review at the end.
>
> Appreciate the speedy review!
You're welcome.
> >> Follow-ups I’d like to make:
> >>
> >> 1. Expand the modes section to show how the types of results can vary
> >> depending on the mode, thanks to the flattening. Examples:
> >>
> >> david=# select jsonb_path_query('{"a":[1,2,3,4,5]}', '$.a ?(@[*] > 2)');
> >> jsonb_path_query
> >> --
> >> 3
> >> 4
> >> 5
> >> (3 rows)
> >>
> >> david=# select jsonb_path_query('{"a":[1,2,3,4,5]}', 'strict $.a ?(@[*] >
> >> 2)');
> >> jsonb_path_query
> >> --
> >> [1, 2, 3, 4, 5]
> >>
> >> 2. Improve the descriptions and examples for @?/jsonb_path_exists()
> >> and @@/jsonb_path_match().
> >
> > +1
>
> I planned to submit these changes in a separate patch, based on Tom
> Lane’s suggestion[1]. Would it be preferred to add them to this patch?
Your call but I'm not against including it in this patch because it
already touches the modes section.
> I pokwds around, and it appears the computeroutput bit is used for
> function output. So I followed the precedent in queries.sgml[2] and
> omitted the computeroutput tags but added prompt, e.g.,
>
> = select jsonb_path_query(:'json', 'strict
> $.**.HR');
> jsonb_path_query
> --
> 73
> 135
>
Okay, Not sure what the preferred style is but I saw and
used together in doc/src/sgml/ref/createuser.sgml.
But it's not applied consistently in the rest of the docs.
> >> +
> >> + Predicate-only path expressions are necessary for implementation of
> >> the
> >> + @@ operator (and the
> >> + jsonb_path_match function), and should not be
> >> used
> >> + with the @? operator (or
> >> + jsonb_path_exists function).
> >> +
> >> +
> >> +
> >> + Conversely, non-predicate jsonpath expressions should
> >> not be
> >> + used with the @@ operator (or the
> >> + jsonb_path_match function).
> >> +
> >> +
> >
> > Both paras should be wrapped in a single so that they stand out
> > from the rest of the text. Maybe even , but is already
> > used on this page for things that I'd consider warnings.
>
> Agreed. Would be good if we could teach these functions and operators
> to reject path expressions they don’t support.
Right, you mentioned that idea in [1] (separate types). Not sure what
the best strategy here is but it's likely to break existing queries.
Maybe deprecating unsupported path expressions in the next major release
and changing that to an error in the major release after that.
> > This can be checked with `make -C doc/src/sgml check`.
>
> Thanks. That produces a bunch of warnings for postgres.sgml and
> legal.sgml (and a failure to load the docbook DTD), but func.sgml is
> clean now.
Hmm... I get no warnings on 1f89b73c4e. Did you install all tools as
described in [2]? The DTD needs to be installed as well.
[1]
https://www.postgresql.org/message-id/BAF11F2D-5EDD-4DBB-87FA-4F35845029AE%40justatheory.com
[2] https://www.postgresql.org/docs/current/docguide-toolsets.html
--
Erik
Re: Patch: Improve Boolean Predicate JSON Path Docs
On 2023-10-14 22:40 +0200, David E. Wheeler write:
> Following up from a suggestion from Tom Lane[1] to improve the
> documentation of boolean predicate JSON path expressions, please find
> enclosed a draft patch to do so.
Thanks for putting this together. See my review at the end.
> It does three things:
>
> 1. Converts all of the example path queries to use jsonb_path_query()
> and show the results, to make it clearer what the behaviors are.
Nice. This really does help to make some sense of it. I checked all
queries and they do work out except for two queries where the path
expression string is not properly quoted (but the intended output is
still correct).
> 2. Replaces the list of deviations from the standards with a new
> subsection, with each deviation in its own sub-subsection. The regex
> section is unchanged, but I’ve greatly expanded the boolean expression
> JSON path section with examples comparing standard filter expressions
> and nonstandard boolean predicates. I’ve also added an exhortation not
> use boolean expressions with @? or standard path expressions with @@.
LGTM.
> 3. While converting the modes section to use jsonb_path_query() and
> show the results, I also added an example of strict mode returning an
> error.
>
> Follow-ups I’d like to make:
>
> 1. Expand the modes section to show how the types of results can vary
> depending on the mode, thanks to the flattening. Examples:
>
> david=# select jsonb_path_query('{"a":[1,2,3,4,5]}', '$.a ?(@[*] > 2)');
> jsonb_path_query
> --
> 3
> 4
> 5
> (3 rows)
>
> david=# select jsonb_path_query('{"a":[1,2,3,4,5]}', 'strict $.a ?(@[*] >
> 2)');
> jsonb_path_query
> --
> [1, 2, 3, 4, 5]
>
> 2. Improve the descriptions and examples for @?/jsonb_path_exists()
> and @@/jsonb_path_match().
+1
> [1] https://www.postgresql.org/message-id/1229727.1680535592%40sss.pgh.pa.us
My review:
> diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
> index affd1254bb..295f8ca5c9 100644
> --- a/doc/src/sgml/func.sgml
> +++ b/doc/src/sgml/func.sgml
> @@ -17205,7 +17205,7 @@ array w/o UK? | t
> For example, suppose you have some JSON data from a GPS tracker that you
> would like to parse, such as:
>
> -{
> + \set json '{
Perhaps make it explicit that the reader must run this in psql in order
to use \set and :'json' in the ensuing samples? Some of the existing
examples already use psql output but they do not rely on any psql
features.
>"track": {
> "segments": [
>{
> @@ -17220,7 +17220,7 @@ array w/o UK? | t
>}
> ]
>}
> -}
> +}'
>
>
>
> @@ -17229,7 +17229,10 @@ array w/o UK? | t
> .key accessor
> operator to descend through surrounding JSON objects:
>
> -$.track.segments
> +select jsonb_path_query(:'json'::jsonb, '$.track.segments');
> +
> jsonb_path_query
> +---
> + [{"HR": 73, "location": [47.763, 13.4034], "start time": "2018-10-14
> 10:05:14"}, {"HR": 135, "location": [47.706, 13.2635], "start time":
> "2018-10-14 10:39:21"}]
>
This should use , , and if it shows
a psql session, e.g.:
select jsonb_path_query(:'json',
'$.track.segments');
jsonb_path_query
---
[{"HR": 73, "location": [47.763, 13.4034], "start time": "2018-10-14
10:05:14"}, {"HR": 135, "location": [47.706, 13.2635], "start time":
"2018-10-14 10:39:21"}]
Also the cast to jsonb is not necessary and only adds clutter IMO.
>
>
> @@ -17239,7 +17242,11 @@ $.track.segments
> the following path will return the location coordinates for all
> the available track segments:
>
> -$.track.segments[*].location
> +select jsonb_path_query(:'json'::jsonb, '$.track.segments[*].location');
> + jsonb_path_query
> +---
> + [47.763, 13.4034]
> + [47.706, 13.2635]
>
>
>
> @@ -17248,7 +17255,10 @@ $.track.segments[*].location
> specify the corresponding subscript in the []
> accessor operator. Recall that JSON array indexes are 0-relative:
>
> -$.track.segments[0].location
> +select jsonb_path_query(:'json'::jsonb, 'strict
> $.track.segments[0].location');
> + jsonb_path_query
> +---
> + [47.763, 13.4034]
>
>
>
> @@ -17259,7 +17269,10 @@ $.track.segments[0].location
> Each method name must be preceded by a dot. For example,
> you can get the size of an array:
>
> -$.track.segments.size()
> +select jsonb_path_query(:'json'::jsonb, 'strict $.track.segments.size()');
> +
Re: [Doc] Glossary Term Definitions Edits
On 2023-10-14 06:16 +0200, Andrew Atkinson write: >- When describing options for a command, changed to “option of” instead >of “option to” I think "option to" is not wrong (maybe less common). I've seen this in other texts and took it as "the X option [that applies] to Y". >- “system- or user-supplied”, removed the dash after system. Or I’d >suggest system-supplied or user-supplied, to hyphenate both. That's a suspended hyphen and is common usage. >- Changed “volume of records has been written” to “volume of records >were written” "Has been" means that something happened just now. This is perfectly fine when talking about checkpoints IMO. >- Many examples of “an SQL”. I changed those to “a SQL...”. For example >I changed “An SQL command which” to “A SQL command that”. I'm not an >English major so maybe I'm missing something here. Depends on how you pronounce SQL (ess-cue-el or sequel). "An SQL" is more common in the docs whereas "a SQL" is more common in code comments. -- Erik
Re: JSON Path and GIN Questions
On 2023-10-09 01:13 +0200, David E. Wheeler write:
> On Sep 12, 2023, at 21:00, Erik Wienhold wrote:
>
> >> I posted this question on Stack Overflow
> >> (https://stackoverflow.com/q/77046554/79202),
> >> and from the suggestion I got there, it seems that @@ expects a boolean to
> >> be
> >> returned by the path query, while @? wraps it in an implicit exists(). Is
> >> that
> >> right?
> >
> > That's also my understanding. We had a discussion about the docs on @@,
> > @?, and
> > jsonb_path_query on -general a while back [1]. Maybe it's useful also.
>
> Hi, finally getting back to this, still fiddling to figure out the
> differences. From the thread you reference [1], is the point that @@
> and jsonb_path_match() can only be properly used with a JSON Path
> expression that’s a predicate check?
I think so. That's also supported by the existing docs which only
mention "JSON path predicate" for @@ and jsonb_path_match().
> If so, as far as I can tell, only exists() around the entire path
> query, or the deviation from the SQL standard that allows an
> expression to be a predicate?
Looks like that. But note that exists() is also a filter expression.
So wrapping the entire jsonpath in exists() is also a deviation from the
SQL standard which only allows predicates in filter expressions, i.e.
' ? ()'.
> This suggest to me that the "Only the first item of the result is
> taken into account” bit from the docs may not be quite right.
Yes, this was also the issue in the referenced thread[1]. I think my
suggesstion in [2] explains it (as far as I understand it).
> Consider this example:
>
> david=# select jsonb_path_query('{"a":[false,true,false]}', '$.a ?(@[*] ==
> false)');
> jsonb_path_query
> --
> false
> false
> (2 rows)
>
> david=# select jsonb_path_match('{"a":[false,true,false]}', '$.a ?(@[*] ==
> false)');
> ERROR: single boolean result is expected
>
> jsonb_path_match(), it turns out, only wants a single result. But
> furthermore perhaps the use of a filter predicate rather than a
> predicate expression for the entire path query is an error?
Yes, I think @@ and jsonb_path_match() should not be used with filter
expressions because the jsonpath returns whatever the path expression
yields (which may be an actual boolean value in the jsonb). The filter
expression only filters (as the name suggests) what the path expression
yields.
> Curiously, @@ seems okay with it:
>
> david=# select '{"a":[false,true,false]}'@@ '$.a ?(@[*] == false)';
> ?column?
> --
> t
>
> Not a predicate query, and somehow returns true even though the first
> item of the result is false? Is that how it should be?
Your example does a text search equivalent to:
select to_tsvector('{"a":[false,true,false]}') @@ plainto_tsquery('$.a
? (@[*] == true)')
You forgot the cast to jsonb. jsonb @@ jsonpath actually returns null:
test=# select '{"a":[false,true,false]}'::jsonb @@ '$.a ? (@[*] ==
false)';
?column?
--
(1 row)
This matches the note right after the docs for @@:
"The jsonpath operators @? and @@ suppress the following errors: missing
object field or array element, unexpected JSON item type, datetime and
numeric errors. The jsonpath-related functions described below can also
be told to suppress these types of errors. This behavior might be
helpful when searching JSON document collections of varying structure."
That would be the silent argument of jsonb_path_match():
test=# select jsonb_path_match('{"a":[false,true,false]}', '$.a ? (@[*]
== false)', silent => true);
jsonb_path_match
--
(1 row)
[1]
https://www.postgresql.org/message-id/CACJufxE01sxgvtG4QEvRZPzs_roggsZeVvBSGpjM5tzE5hMCLA%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/880194083.579916.1680598906819%40office.mailbox.org
--
Erik
Re: Fix output of zero privileges in psql
On 2023-10-09 22:34 +0200, David G. Johnston write: > On Mon, Oct 9, 2023 at 12:13 PM Tom Lane wrote: > > Yeah. There is a lot of attraction in having \pset null affect these > > displays just like all other ones. The fact that they act differently > > now is a wart, not something we should replace with a different special > > case behavior. > > > > Also, I'm fairly concerned about not changing the default behavior here. > > The fact that this behavior has stood for a couple dozen years without > > many complaints indicates that there's not all that big a problem to be > > solved. I doubt that a new default behavior will be well received, > > even if it's arguably better. > > > > My position is that the default behavior should be changed such that the > distinction these reports need to make between empty privileges and default > privileges is impossible for the user to remove. I suppose the best direct > solution given that goal is for the query to simply do away with any > reliance on NULL being an indicator. Output an i18n'd "no permissions > present" line in the rare empty permissions situation and leave the empty > string for built-in default. My initial patch does exactly that. > If the consensus is to not actually fix these views to make them > environment invariant in their accuracy then so be it. Having them obey > \pset null seems like a half-measure but it at least is an improvement over > the status quo such that users are capable of altering their system to make > the reports distinguish the two cases if they realize the need. I agree. -- Erik
Re: Fix output of zero privileges in psql
On 2023-10-09 10:29 +0200, Laurenz Albe write: > On Sun, 2023-10-08 at 19:58 -0700, David G. Johnston wrote: > > We probably should add the two terms to the glossary: > > > > empty privileges: all privileges explicitly revoked from the owner and > > PUBLIC > > (where applicable), and none otherwise granted. > > > > built-in default privileges: owner having all privileges and no privileges > > granted or removed via ALTER DEFAULT PRIVILEGES > > "Empty privileges" are too unimportant to warrant an index entry. > > I can see the value of an index entry > > > privilege > default > > > Done in the attached v5 of the patch, even though this is not really > the business of this patch. +1 -- Erik
Re: Fix output of zero privileges in psql
On 2023-10-09 09:54 +0200, Laurenz Albe write: > > I tinkered a bit with your documentation. For example, the suggestion to > "\pset null" seemed to be in an inappropriate place. Tell me what you think. +1 You're right to put that sentence right after the explanation of empty privileges. -- Erik
Re: Fix output of zero privileges in psql
On 2023-10-08 06:14 +0200, Laurenz Albe write:
> On Sat, 2023-10-07 at 20:41 +0200, Erik Wienhold wrote:
> > > If you are happy enough with my patch, shall we mark it as ready for
> > > committer?
> >
> > I amended your patch to also document the effect of \pset null in this
> > case. See the attached v2.
>
> +1
>
> If you mention in ddl.sgml that you can use "\pset null" to distinguish
> default from no privileges, you should mention that this only works with
> psql. Many people out there don't use psql.
I don't think this is necessary because that section in ddl.sgml is
already about psql and \dp.
> Also, I'm not sure if "zero privileges" will be readily understood by
> everybody. Perhaps "no privileges at all, even for the object owner"
> would be a better wording.
Changed in v3 to "empty privileges" with an explanation that this means
"no privileges at all, even for the object owner".
> Perhaps it would also be good to mention this in the psql documentation.
Just once under \pset null with a reference to section 5.7? Something
like "This is also useful for distinguishing default privileges from
empty privileges as explained in Section 5.7."
Or instead under every command affected by this change? \dp and \ddp
already have such a reference ("The meaning of the privilege display is
explained in Section 5.7.")
I prefer the first one because it's less effort ;) Also done in v3.
--
Erik
>From b7ddd4daa87baab83ad7e857c996b5a4a0b3ffa7 Mon Sep 17 00:00:00 2001
From: Laurenz Albe
Date: Fri, 6 Oct 2023 22:12:33 +0200
Subject: [PATCH v3] psql: honor "\pset null" in backslash commands
In the output of backslash commands, NULL was always displayed
as empty string, rather than honoring the values set with
"\pset null".
This was surprising, and in some cases it was downright confusing:
for example, default privileges (stored as NULL) were displayed
just like an empty aclitem[], making these cases undistinguishable
in the output. Add this missing detail to the documentation.
Discussion:
https://postgr.es/m/96d6885a-5e25-9ae8-4a1a-d7e557a5fe9c%40mtneva.com
---
doc/src/sgml/ddl.sgml | 7 --
doc/src/sgml/ref/psql-ref.sgml | 3 ++-
src/bin/psql/describe.c| 43 --
3 files changed, 7 insertions(+), 46 deletions(-)
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 075ff32991..436d655d3c 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2353,8 +2353,9 @@ GRANT SELECT (col1), UPDATE (col1) ON mytable TO
miriam_rw;
If the Access privileges column is empty for a given
object, it means the object has default privileges (that is, its
- privileges entry in the relevant system catalog is null). Default
- privileges always include all privileges for the owner, and can include
+ privileges entry in the relevant system catalog is null) or empty privileges
+ (that is, no privileges at all, even for the object owner).
+ Default privileges always include all privileges for the owner, and can
include
some privileges for PUBLIC depending on the object
type, as explained above. The first GRANT
or REVOKE on an object will instantiate the default
@@ -2362,6 +2363,8 @@ GRANT SELECT (col1), UPDATE (col1) ON mytable TO
miriam_rw;
example, miriam=arwdDxt/miriam) and then modify them
per the specified request. Similarly, entries are shown in Column
privileges only for columns with nondefault privileges.
+ You can, for example, set \pset null '(default)' to
+ distinguish default privileges from empty privileges.
(Note: for this purpose, default privileges always means
the built-in default privileges for the object's type. An object whose
privileges have been affected by an ALTER DEFAULT
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index d94e3cacfc..966697eb50 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3079,7 +3079,8 @@ lo_import 152801
Sets the string to be printed in place of a null value.
The default is to print nothing, which can easily be mistaken for
an empty string. For example, one might prefer \pset null
- '(null)'.
+ '(null)'. This is also useful for distinguishing default
+ privileges from empty privileges as explained in .
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index bac94a338c..224aa22575 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -124,7 +124,6 @@ describeAggregates(const char *pattern, bool verbose, bool
showSystem)
if (!res)
return false;
- myopt.nullPrint = NULL;
myopt.title = _("List of aggregate functions&
Re: Fix output of zero privileges in psql
On 2023-10-07 14:29 +0200, Laurenz Albe write: > On Sat, 2023-10-07 at 05:07 +0200, Erik Wienhold wrote: > > On 2023-10-06 22:32 +0200, Laurenz Albe write: > > > On Sun, 2023-09-17 at 21:31 +0200, Erik Wienhold wrote: > > > > I wrote a patch to change psql's display of zero privileges after a > > > > user's > > > > reported confusion with the psql output for zero vs. default privileges > > > > [1]. > > > > Admittedly, zero privileges is a rare use case [2] but I think psql > > > > should not > > > > confuse the user in the off chance that this happens. > > > > > > > > [1] > > > > https://www.postgresql.org/message-id/efdd465d-a795-6188-7f71-7cdb4b2be031%40mtneva.com > > > > [2] > > > > https://www.postgresql.org/message-id/31246.1693337238%40sss.pgh.pa.us > > > > > > Reading that thread, I had the impression that there was more support for > > > honoring "\pset null" rather than unconditionally displaying "(none)". > > > > For example with your patch applied: > > > > create table t1 (a int); > > create table t2 (a int); > > create table t3 (a int); > > > > revoke all on t2 from :USER; > > > > \pset null > > \dp t1|t2|t3 > > Access privileges > > Schema | Name | Type | Access privileges | Column privileges | > > Policies > > > > +--+---+---+---+-- > > public | t1 | table | | | > > public | t2 | table | | | > > public | t3 | table | | | > > (3 rows) > > > > Instead of only displaying the zero privileges with my patch and default > > \pset null: > > > > \pset null '' > > \dp t1|t2|t3 > > Access privileges > > Schema | Name | Type | Access privileges | Column privileges | > > Policies > > > > +--+---+---+---+-- > > public | t1 | table | | | > > public | t2 | table | (none) | | > > public | t3 | table | | | > > (3 rows) > > > > I guess if most tables have any non-default privileges then both > > solutions are equally good. > > It is a tough call. > > For somebody who knows PostgreSQL well enough to know that default > privileges are represented by NULL values, my solution is probably > more appealing. > > It seems that we both had the goal of distinguishing the cases of > default and zero privileges, but for a beginner, both versions are > confusing. better would probably be > > Access privileges > Schema | Name | Type | Access privileges | Column privileges | Policies > +--+---+---+---+-- > public | t1 | table | default | default | > public | t2 | table | | default | > public | t3 | table | default | default | Ah yes. The problem seems to be more with default privileges producing no output right now. I was just focusing on the zero privs edge case. > The disadvantage of this (and the advantage of my proposal) is that it > might confuse experienced users (and perhaps automated tools) if the > output changes too much. I agree that your patch is less invasive under default settings. But is the output of meta commands considered part of the interface where we need to be cautious about not breaking clients? I've written quite a few scripts that parse results from psql's stdout, but always with simple queries to have control over columns and the formatting of values. I always expect meta command output to change with the next release because to me they look more like a human-readable interface, e.g. the localizable header which of course one can still hide with --tuples-only. > > > The simple attached patch does it like that. What do you think? > > > > LGTM. > > If you are happy enough with my patch, shall we mark it as ready for > committer? I amended your patch to also document the effect of \pset null in this case. See the attached v2. > Or do you want to have a stab at something like I suggested above? Not right now if the user can
Re: Fix output of zero privileges in psql
On 2023-10-06 22:32 +0200, Laurenz Albe write: > On Sun, 2023-09-17 at 21:31 +0200, Erik Wienhold wrote: > > I wrote a patch to change psql's display of zero privileges after a user's > > reported confusion with the psql output for zero vs. default privileges [1]. > > Admittedly, zero privileges is a rare use case [2] but I think psql should > > not > > confuse the user in the off chance that this happens. > > > > With this change psql now prints "(none)" for zero privileges instead of > > nothing. This affects the following meta commands: > > > > \db+ \dD+ \df+ \dL+ \dl+ \dn+ \dp \dT+ \l > > > > Default privileges start as NULL::aclitem[] in various catalog columns but > > revoking the default privileges leaves an empty aclitem array. Using > > \pset null '(null)' as a workaround to spot default privileges does not work > > because the meta commands ignore this setting. > > > > The privileges shown by \dconfig+ and \ddp as well as the column privileges > > shown by \dp are not affected by this change because those privileges are > > reset > > to NULL instead of leaving empty arrays. > > > > Commands \des+ and \dew+ are not covered in src/test/regress because no > > foreign > > data wrapper is available at this point to create a foreign server. > > > > [1] > > https://www.postgresql.org/message-id/efdd465d-a795-6188-7f71-7cdb4b2be031%40mtneva.com > > [2] https://www.postgresql.org/message-id/31246.1693337238%40sss.pgh.pa.us > > Reading that thread, I had the impression that there was more support for > honoring "\pset null" rather than unconditionally displaying "(none)". I took Tom's response in the -general thread to mean that we could fix \pset null also as a "nice to have" but not as a solution to the display of zero privileges. Only fixing \pset null has one drawback IMO because it only affects how default privileges (more common) are printed. The edge case of zero privileges (less common) gets lost in a bunch of NULL output. And I assume most users change the default \pset null to some non-empty string in their psqlrc (I do). For example with your patch applied: create table t1 (a int); create table t2 (a int); create table t3 (a int); revoke all on t2 from :USER; \pset null \dp t1|t2|t3 Access privileges Schema | Name | Type | Access privileges | Column privileges | Policies +--+---+---+---+-- public | t1 | table | | | public | t2 | table | | | public | t3 | table | | | (3 rows) Instead of only displaying the zero privileges with my patch and default \pset null: \pset null '' \dp t1|t2|t3 Access privileges Schema | Name | Type | Access privileges | Column privileges | Policies +--+---+---+---+-- public | t1 | table | | | public | t2 | table | (none)| | public | t3 | table | | | (3 rows) I guess if most tables have any non-default privileges then both solutions are equally good. > The simple attached patch does it like that. What do you think? LGTM. -- Erik
Re: How to Know the number of attrs?
On 2023-09-24 20:56 +0800, jacktby jacktby wrote:
> typedef struct TupleDescData
> {
> int natts; /* number of attributes
> in the tuple */
> Oid tdtypeid; /* composite type ID
> for tuple type */
> int32 tdtypmod; /* typmod for tuple type */
> int tdrefcount; /* reference count, or
> -1 if not counting */
> TupleConstr *constr;/* constraints, or NULL if none */
> /* attrs[N] is the description of Attribute Number N+1 */
> FormData_pg_attribute attrs[FLEXIBLE_ARRAY_MEMBER];
> } TupleDescData;
>
> Hi, the attrs use FLEXIBLE_ARRAY_MEMBER, so which API should I use to
> get the real length of this array?
Use the natts field of TupleDescData.
--
Erik
Re: to_regtype() Raises Error
On 18/09/2023 00:57 CEST Vik Fearing wrote:
> On 9/18/23 00:41, Erik Wienhold wrote:
> > On 18/09/2023 00:13 CEST David E. Wheeler wrote:
> >
> >> david=# select to_regtype('inteval second');
> >> ERROR: syntax error at or near "second"
> >> LINE 1: select to_regtype('inteval second');
> >> ^
> >> CONTEXT: invalid type name "inteval second”
> >
> > Probably a typo and you meant 'interval second' which works.
>
> No, that is precisely the point. The result should be null instead of
> an error.
Well, the docs say "return NULL rather than throwing an error if the name is
not found". To me "name is not found" implies that it has to be valid syntax
first to even have a name that can be looked up.
String 'inteval second' is a syntax error when interpreted as a type name.
The same when I want to create a table with that typo:
test=# create table t (a inteval second);
ERROR: syntax error at or near "second"
LINE 1: create table t (a inteval second);
And a custom function is always an option:
create function to_regtype_lax(name text)
returns regtype
language plpgsql
as $$
begin
return to_regtype(name);
exception
when others then
return null;
end
$$;
test=# select to_regtype_lax('inteval second');
to_regtype_lax
(1 row)
--
Erik
Re: to_regtype() Raises Error
On 18/09/2023 00:13 CEST David E. Wheeler wrote:
> The docs for `to_regtype()` say, “this function will return NULL rather than
> throwing an error if the name is not found.” And it’s true most of the time:
>
> david=# select to_regtype('foo'), to_regtype('clam');
> to_regtype | to_regtype
> +
> [null] | [null]
>
> But not others:
>
> david=# select to_regtype('inteval second');
> ERROR: syntax error at or near "second"
> LINE 1: select to_regtype('inteval second');
> ^
> CONTEXT: invalid type name "inteval second”
Probably a typo and you meant 'interval second' which works.
> I presume this has something to do with not catching errors from the parser?
>
> david=# select to_regtype('clam bake');
> ERROR: syntax error at or near "bake"
> LINE 1: select to_regtype('clam bake');
> ^
> CONTEXT: invalid type name "clam bake"
Double-quoting the type name to treat it as an identifier works:
test=# select to_regtype('"clam bake"');
to_regtype
(1 row)
So it's basically a matter of keywords vs. identifiers.
--
Erik
Re: Fix output of zero privileges in psql
On 17/09/2023 21:31 CEST Erik Wienhold wrote: > This affects the following meta commands: > > \db+ \dD+ \df+ \dL+ \dl+ \dn+ \dp \dT+ \l also \des+ and \dew+ -- Erik
Fix output of zero privileges in psql
I wrote a patch to change psql's display of zero privileges after a user's
reported confusion with the psql output for zero vs. default privileges [1].
Admittedly, zero privileges is a rare use case [2] but I think psql should not
confuse the user in the off chance that this happens.
With this change psql now prints "(none)" for zero privileges instead of
nothing. This affects the following meta commands:
\db+ \dD+ \df+ \dL+ \dl+ \dn+ \dp \dT+ \l
Default privileges start as NULL::aclitem[] in various catalog columns but
revoking the default privileges leaves an empty aclitem array. Using
\pset null '(null)' as a workaround to spot default privileges does not work
because the meta commands ignore this setting.
The privileges shown by \dconfig+ and \ddp as well as the column privileges
shown by \dp are not affected by this change because those privileges are reset
to NULL instead of leaving empty arrays.
Commands \des+ and \dew+ are not covered in src/test/regress because no foreign
data wrapper is available at this point to create a foreign server.
[1]
https://www.postgresql.org/message-id/efdd465d-a795-6188-7f71-7cdb4b2be031%40mtneva.com
[2] https://www.postgresql.org/message-id/31246.1693337238%40sss.pgh.pa.us
--
ErikFrom 16af995acb4c0e5fb5981308610a76b7e87c3358 Mon Sep 17 00:00:00 2001
From: Erik Wienhold
Date: Sun, 17 Sep 2023 20:54:48 +0200
Subject: [PATCH v1] Fix output of zero privileges in psql
Print "(none)" for zero privileges instead of nothing so that the user
is able to distinguish zero from default privileges. This affects the
following meta commands:
\db+ \dD+ \des+ \dew+ \df+ \dL+ \dl+ \dn+ \dp \dT+ \l
Default privileges start as NULL::aclitem[] in various catalog columns
but revoking the default privileges leaves an empty aclitem array.
Using \pset null '(null)' as a workaround to spot default privileges
does not work because the meta commands ignore this setting.
The privileges shown by \dconfig+ and \ddp as well as the column
privileges shown by \dp are not affected by this change because those
privileges are reset to NULL instead of leaving empty arrays.
Commands \des+ and \dew+ are not covered in src/test/regress because no
foreign data wrapper is available at this point to create a foreign
server.
---
src/bin/psql/describe.c | 6 +-
src/test/regress/expected/privileges.out | 93
src/test/regress/sql/privileges.sql | 46
3 files changed, 144 insertions(+), 1 deletion(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index bac94a338c..154d244d97 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -6718,7 +6718,11 @@ static void
printACLColumn(PQExpBuffer buf, const char *colname)
{
appendPQExpBuffer(buf,
- "pg_catalog.array_to_string(%s, E'\\n') AS \"%s\"",
+ "CASE pg_catalog.cardinality(%s)\n"
+ " WHEN 0 THEN '%s'\n"
+ " ELSE pg_catalog.array_to_string(%s, E'\\n')\n"
+ "END AS \"%s\"",
+ colname, gettext_noop("(none)"),
colname, gettext_noop("Access privileges"));
}
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index c1e610e62f..b9eb52f7b1 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -2895,3 +2895,96 @@ DROP SCHEMA regress_roleoption;
DROP ROLE regress_roleoption_protagonist;
DROP ROLE regress_roleoption_donor;
DROP ROLE regress_roleoption_recipient;
+-- Test zero privileges.
+BEGIN;
+-- Create an owner for tested objects because output contains owner info.
+-- Must be superuser to be owner of tablespace.
+CREATE ROLE regress_zeropriv_owner SUPERUSER;
+SET LOCAL ROLE regress_zeropriv_owner;
+ALTER TABLESPACE regress_tblspace OWNER TO CURRENT_USER;
+REVOKE ALL ON TABLESPACE regress_tblspace FROM CURRENT_USER;
+\db+ regress_tblspace
+List of tablespaces
+ Name | Owner |Location | Access privileges | Options | Size | Description
+--++-+---+-+-+-
+ regress_tblspace | regress_zeropriv_owner | pg_tblspc/16385 | (none)| | 0 bytes |
+(1 row)
+
+CREATE DOMAIN regress_zeropriv_domain AS int;
+REVOKE ALL ON DOMAIN regress_zeropriv_domain FROM CURRENT_USER, PUBLIC;
+\dD+ regress_zeropriv_domain
+List of domains
+ Schema | Name | Type | Collation | Nullable | Default | Check | Access privileges | Description
++-+-+---+--+-+---+---+-
+ public | regress_zeropriv_domain | integer | | | | | (none)
Re: JSON Path and GIN Questions
On 16/09/2023 22:26 CEST David E. Wheeler wrote: > I’ve started work on this; there’s so much to learn! Here’s a new example > that surprised me a bit. Using the GPS tracker example from the docs [1] > loaded into a `:json` psql variable, this output of this query makes perfect > sense to me: > > david=# select jsonb_path_query(:'json', '$.track.segments.location[*] ? (@ < > 14)'); > jsonb_path_query > -- > 13.4034 > 13.2635 > > Because `[*]` selects all the values. This, however, I did not expect: > > david=# select jsonb_path_query(:'json', '$.track.segments.location ? (@[*] < > 14)'); > jsonb_path_query > -- > 13.4034 > 13.2635 > (2 rows) > > I had expected it to return two single-value arrays, instead: > > [13.4034] > [13.2635] > > It appears that the filter expression is doing some sub-selection, too. > Is that expected? Looks like the effect of lax mode which may unwrap arrays when necessary [1]. The array unwrapping looks like the result of jsonb_array_elements(). It kinda works in strict mode: SELECT jsonb_path_query(:'json', 'strict $.track.segments[*].location ? (@[*] < 14)'); jsonb_path_query --- [47.763, 13.4034] [47.706, 13.2635] (2 rows) But it does not remove elements from the matching arrays. Which I don't even expect here because the path specifies the location array as the object to be returned. The filter expression then only decides whether to return the location array or not. Nowhere in the docs does it say that the filter expression itself removes any elements from a matched array. Here's a query that filter's out individual array elements. It's quite a mouthful (especially to preserve the order of array elements): WITH location AS ( SELECT loc, row_number() OVER () AS array_num FROM jsonb_path_query(:'json', 'strict $.track.segments[*].location') loc ), element AS ( SELECT array_num, e.num AS elem_num, e.elem FROM location CROSS JOIN jsonb_array_elements(loc) WITH ORDINALITY AS e (elem, num) ) SELECT jsonb_agg(elem ORDER BY elem_num) FROM element WHERE jsonb_path_exists(elem, '$ ? (@ < 14)') GROUP BY array_num; jsonb_agg --- [13.2635] [13.4034] (2 rows) [1] https://www.postgresql.org/docs/current/functions-json.html#STRICT-AND-LAX-MODES -- Erik
Re: JSON Path and GIN Questions
On 16/09/2023 22:19 CEST David E. Wheeler wrote: > On Sep 15, 2023, at 23:59, Erik Rijkers wrote: > > > movie @? '$ ?($.year >= 2023)' > > > > I believe it is indeed not possible to have such a unequality-search use > > the GIN index. It is another weakness of JSON that can be unexpected to > > those not in the fullness of Knowledge of the manual. Yes, this too would > > be good to explain in the doc where JSON indexes are explained. > > Is that a limitation of GIN indexes in general? Or could there be opclass > improvements in the future that would enable such comparisons? This detail is mentioned in docs [1]: "For these operators, a GIN index extracts clauses of the form **accessors_chain = constant** out of the jsonpath pattern, and does the index search based on the keys and values mentioned in these clauses." I don't know if this is a general limitation of GIN indexes or just how these operators are implemented right now. [1] https://www.postgresql.org/docs/current/datatype-json.html#JSON-INDEXING -- Erik
Re: JSON Path and GIN Questions
Hi David, On 13/09/2023 02:16 CEST David E. Wheeler wrote: > CREATE TABLE MOVIES (id SERIAL PRIMARY KEY, movie JSONB NOT NULL); > \copy movies(movie) from PROGRAM 'curl -s > https://raw.githubusercontent.com/prust/wikipedia-movie-data/master/movies.json > | jq -c ".[]" | sed "s|||g"'; > create index on movies using gin (movie); > analyze movies; > > I have been confused as to the difference between @@ vs @?: Why do these > return different results? > > david=# select id from movies where movie @@ '$ ?(@.title == "New Life > Rescue")'; > id > > (0 rows) > > david=# select id from movies where movie @? '$ ?(@.title == "New Life > Rescue")'; > id > > 10 > (1 row) > > I posted this question on Stack Overflow > (https://stackoverflow.com/q/77046554/79202), > and from the suggestion I got there, it seems that @@ expects a boolean to be > returned by the path query, while @? wraps it in an implicit exists(). Is that > right? That's also my understanding. We had a discussion about the docs on @@, @?, and jsonb_path_query on -general a while back [1]. Maybe it's useful also. > If so, I’d like to submit a patch to the docs talking about this, and > suggesting the use of jsonb_path_query() to test paths to see if they return > a boolean or not. +1 [1] https://www.postgresql.org/message-id/CACJufxE01sxgvtG4QEvRZPzs_roggsZeVvBSGpjM5tzE5hMCLA%40mail.gmail.com -- Erik
Re: Autogenerate some wait events code and documentation
On 05/09/2023 13:50 CEST Michael Paquier wrote: > On Tue, Sep 05, 2023 at 11:06:36AM +0200, Drouvot, Bertrand wrote: > > Oh ok, out of curiosity, why are 2 whitespaces intentional? > > That depends on the individual who write the code, but I recall that > this is some old-school style from the 70's and/or the 80's when > typing machines were still something. I'm just used to this style > after the end of a sentence in a comment. FYI: https://en.wikipedia.org/wiki/Sentence_spacing -- Erik
