issue: record or row variable cannot be part of multiple-item INTO list
Hi
I am working on migration large Oracle application to Postgres. When I
started migration procedures with OUT parameters I found following limit
"record or row variable cannot be part of multiple-item INTO list"
I checked code and it looks so this limit is not necessary for ROW types
(what is enough for migration from Oracle, where REC is not available).
Do you think so this limit is necessary for ROW types?
@@ -3368,19 +3368,7 @@ read_into_target(PLpgSQL_rec **rec, PLpgSQL_row
**row, bool *strict)
switch (tok)
{
case T_DATUM:
- if (yylval.wdatum.datum->dtype == PLPGSQL_DTYPE_ROW)
- {
- check_assignable(yylval.wdatum.datum, yylloc);
- *row = (PLpgSQL_row *) yylval.wdatum.datum;
-
- if ((tok = yylex()) == ',')
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("record or row variable cannot be part
of multiple-item INTO list"),
- parser_errposition(yylloc)));
- plpgsql_push_back_token(tok);
- }
- else if (yylval.wdatum.datum->dtype == PLPGSQL_DTYPE_REC)Regards
Pavel
Pavel Stehule <pavel.stehule@gmail.com> writes:
I am working on migration large Oracle application to Postgres. When I
started migration procedures with OUT parameters I found following limit
"record or row variable cannot be part of multiple-item INTO list"
IIRC, the reason for disallowing that is that it's totally unclear what
the semantics ought to be. Is that variable a single target (demanding
a compatible composite-valued column from the source query), or does it
eat one source column per field within the record/row? The former is 100%
inconsistent with what happens if the record/row is the only INTO target;
while the latter would be very bug-prone, and it's especially unclear what
ought to happen if it's an as-yet-undefined record variable.
Yeah, we could invent some semantics or other, but I think it would
mostly be a foot-gun for unwary programmers.
We do allow you to write out the columns individually for such cases:
SELECT ... INTO v1, rowvar.c1, rowvar.c2, rowvar.c3, v2 ...
and I think it's better to encourage people to stick to that.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2017-05-13 22:20 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:
Pavel Stehule <pavel.stehule@gmail.com> writes:
I am working on migration large Oracle application to Postgres. When I
started migration procedures with OUT parameters I found following limit"record or row variable cannot be part of multiple-item INTO list"
IIRC, the reason for disallowing that is that it's totally unclear what
the semantics ought to be. Is that variable a single target (demanding
a compatible composite-valued column from the source query), or does it
eat one source column per field within the record/row? The former is 100%
inconsistent with what happens if the record/row is the only INTO target;
while the latter would be very bug-prone, and it's especially unclear what
ought to happen if it's an as-yet-undefined record variable.
I don't think so. The semantics should be same like now.
now, the output (s1,s2,s3) can be assigned to
1. scalar variables - implemented with aux row variable (s1,s2,s3) ->
r(ts1,ts2,ts3)
2. record - (s1, s2, s3) -> rec(s1,s2,s3)
3. row - (s1,s2,s3) -> r(s1,s2,s3)
If we allow composite values there, then situation is same
1. (s1, c2, s3, c4) -> r(ts1, tc2, ts3, tc4)
2. (s1, c2, s3, c4) -> rec(s1, c2, s3, c4)
3. (s1, c2, s3, c4) -> row(s1, c2, s3, c4)
So there are not any inconsistency if we use rule
1. if there is one target, use it
2. if there are more target, create aux row variable
Same technique is used for function output - build_row_from_vars - and
there are not any problem.
If you try assign composite to scalar or scalar to composite, then the
assignment should to fail. But when statement is correct, then this invalid
assignments should not be there.
Yeah, we could invent some semantics or other, but I think it would
mostly be a foot-gun for unwary programmers.We do allow you to write out the columns individually for such cases:
SELECT ... INTO v1, rowvar.c1, rowvar.c2, rowvar.c3, v2 ...
It doesn't help to performance and readability (and maintainability) for
following cases
There are often pattern
PROCEDURE p(..., OUT r widetab%ROWTYPE, OUT errordesc COMPOSITE)
Now there is a workaround
SELECT * FROM p() INTO auxrec;
r := auxrec.widetab;
errordesc := auxrec.errordesc;
But it creates N (number of OUT variables) of assignments commands over
records.
If this workaround is safe, then implementation based on aux row variable
should be safe too, because it is manual implementation.
and I think it's better to encourage people to stick to that.
I don't think so using tens OUT variables is some nice, but current behave
is too restrictive. More, I didn't find a case, where current
implementation should not work (allow records needs some work).
Show quoted text
regards, tom lane
2017-05-14 5:04 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:
2017-05-13 22:20 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:
Pavel Stehule <pavel.stehule@gmail.com> writes:
I am working on migration large Oracle application to Postgres. When I
started migration procedures with OUT parameters I found following limit"record or row variable cannot be part of multiple-item INTO list"
IIRC, the reason for disallowing that is that it's totally unclear what
the semantics ought to be. Is that variable a single target (demanding
a compatible composite-valued column from the source query), or does it
eat one source column per field within the record/row? The former is 100%
inconsistent with what happens if the record/row is the only INTO target;
while the latter would be very bug-prone, and it's especially unclear what
ought to happen if it's an as-yet-undefined record variable.I don't think so. The semantics should be same like now.
now, the output (s1,s2,s3) can be assigned to
1. scalar variables - implemented with aux row variable (s1,s2,s3) ->
r(ts1,ts2,ts3)
2. record - (s1, s2, s3) -> rec(s1,s2,s3)
3. row - (s1,s2,s3) -> r(s1,s2,s3)If we allow composite values there, then situation is same
1. (s1, c2, s3, c4) -> r(ts1, tc2, ts3, tc4)
2. (s1, c2, s3, c4) -> rec(s1, c2, s3, c4)
3. (s1, c2, s3, c4) -> row(s1, c2, s3, c4)So there are not any inconsistency if we use rule
1. if there is one target, use it
2. if there are more target, create aux row variableSame technique is used for function output - build_row_from_vars - and
there are not any problem.If you try assign composite to scalar or scalar to composite, then the
assignment should to fail. But when statement is correct, then this invalid
assignments should not be there.Yeah, we could invent some semantics or other, but I think it would
mostly be a foot-gun for unwary programmers.We do allow you to write out the columns individually for such cases:
SELECT ... INTO v1, rowvar.c1, rowvar.c2, rowvar.c3, v2 ...
It doesn't help to performance and readability (and maintainability) for
following casesThere are often pattern
PROCEDURE p(..., OUT r widetab%ROWTYPE, OUT errordesc COMPOSITE)
Now there is a workaround
SELECT * FROM p() INTO auxrec;
r := auxrec.widetab;
errordesc := auxrec.errordesc;But it creates N (number of OUT variables) of assignments commands over
records.If this workaround is safe, then implementation based on aux row variable
should be safe too, because it is manual implementation.and I think it's better to encourage people to stick to that.
I don't think so using tens OUT variables is some nice, but current behave
is too restrictive. More, I didn't find a case, where current
implementation should not work (allow records needs some work).
here is patch
all regress tests passed
Regards
Pavel
Show quoted text
regards, tom lane
Attachments:
plpgsql-into-multitarget.sqlapplication/sql; name=plpgsql-into-multitarget.sqlDownload
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index 94f1f58..6610332 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -92,9 +92,10 @@ static char *NameOfDatum(PLwdatum *wdatum);
static void check_assignable(PLpgSQL_datum *datum, int location);
static void read_into_target(PLpgSQL_rec **rec, PLpgSQL_row **row,
bool *strict);
-static PLpgSQL_row *read_into_scalar_list(char *initial_name,
- PLpgSQL_datum *initial_datum,
- int initial_location);
+static void read_into_list(char *initial_name,
+ PLpgSQL_datum *initial_datum, int initial_location,
+ PLpgSQL_datum **scalar,
+ PLpgSQL_rec **rec, PLpgSQL_row **row);
static PLpgSQL_row *make_scalar_list1(char *initial_name,
PLpgSQL_datum *initial_datum,
int lineno, int location);
@@ -1558,33 +1559,9 @@ for_variable : T_DATUM
{
$$.name = NameOfDatum(&($1));
$$.lineno = plpgsql_location_to_lineno(@1);
- if ($1.datum->dtype == PLPGSQL_DTYPE_ROW)
- {
- $$.scalar = NULL;
- $$.rec = NULL;
- $$.row = (PLpgSQL_row *) $1.datum;
- }
- else if ($1.datum->dtype == PLPGSQL_DTYPE_REC)
- {
- $$.scalar = NULL;
- $$.rec = (PLpgSQL_rec *) $1.datum;
- $$.row = NULL;
- }
- else
- {
- int tok;
- $$.scalar = $1.datum;
- $$.rec = NULL;
- $$.row = NULL;
- /* check for comma-separated list */
- tok = yylex();
- plpgsql_push_back_token(tok);
- if (tok == ',')
- $$.row = read_into_scalar_list($$.name,
- $$.scalar,
- @1);
- }
+ read_into_list($$.name, $1.datum, @1,
+ &$$.scalar, &$$.rec, &$$.row);
}
| T_WORD
{
@@ -3337,89 +3314,21 @@ check_assignable(PLpgSQL_datum *datum, int location)
}
/*
- * Read the argument of an INTO clause. On entry, we have just read the
- * INTO keyword.
- */
-static void
-read_into_target(PLpgSQL_rec **rec, PLpgSQL_row **row, bool *strict)
-{
- int tok;
-
- /* Set default results */
- *rec = NULL;
- *row = NULL;
- if (strict)
- *strict = false;
-
- tok = yylex();
- if (strict && tok == K_STRICT)
- {
- *strict = true;
- tok = yylex();
- }
-
- /*
- * Currently, a row or record variable can be the single INTO target,
- * but not a member of a multi-target list. So we throw error if there
- * is a comma after it, because that probably means the user tried to
- * write a multi-target list. If this ever gets generalized, we should
- * probably refactor read_into_scalar_list so it handles all cases.
- */
- switch (tok)
- {
- case T_DATUM:
- if (yylval.wdatum.datum->dtype == PLPGSQL_DTYPE_ROW)
- {
- check_assignable(yylval.wdatum.datum, yylloc);
- *row = (PLpgSQL_row *) yylval.wdatum.datum;
-
- if ((tok = yylex()) == ',')
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("record or row variable cannot be part of multiple-item INTO list"),
- parser_errposition(yylloc)));
- plpgsql_push_back_token(tok);
- }
- else if (yylval.wdatum.datum->dtype == PLPGSQL_DTYPE_REC)
- {
- check_assignable(yylval.wdatum.datum, yylloc);
- *rec = (PLpgSQL_rec *) yylval.wdatum.datum;
-
- if ((tok = yylex()) == ',')
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("record or row variable cannot be part of multiple-item INTO list"),
- parser_errposition(yylloc)));
- plpgsql_push_back_token(tok);
- }
- else
- {
- *row = read_into_scalar_list(NameOfDatum(&(yylval.wdatum)),
- yylval.wdatum.datum, yylloc);
- }
- break;
-
- default:
- /* just to give a better message than "syntax error" */
- current_token_is_not_variable(tok);
- }
-}
-
-/*
* Given the first datum and name in the INTO list, continue to read
- * comma-separated scalar variables until we run out. Then construct
+ * comma-separated variables until we run out. Then construct
* and return a fake "row" variable that represents the list of
- * scalars.
+ * fields. When there is only one rec or row field, then return
+ * this variable without nesting.
*/
-static PLpgSQL_row *
-read_into_scalar_list(char *initial_name,
- PLpgSQL_datum *initial_datum,
- int initial_location)
+static void
+read_into_list(char *initial_name,
+ PLpgSQL_datum *initial_datum, int initial_location,
+ PLpgSQL_datum **scalar, PLpgSQL_rec **rec, PLpgSQL_row **row)
{
int nfields;
char *fieldnames[1024];
int varnos[1024];
- PLpgSQL_row *row;
+ PLpgSQL_row *auxrow;
int tok;
check_assignable(initial_datum, initial_location);
@@ -3427,6 +3336,21 @@ read_into_scalar_list(char *initial_name,
varnos[0] = initial_datum->dno;
nfields = 1;
+ *rec = NULL;
+ *row = NULL;
+ if (scalar)
+ *scalar = NULL;
+
+ /*
+ * save row or rec if list has only one field.
+ */
+ if (initial_datum->dtype == PLPGSQL_DTYPE_ROW)
+ *row = (PLpgSQL_row *) initial_datum;
+ else if (initial_datum->dtype == PLPGSQL_DTYPE_REC)
+ *rec = (PLpgSQL_rec *) initial_datum;
+ else if (scalar != NULL)
+ *scalar = initial_datum;
+
while ((tok = yylex()) == ',')
{
/* Check for array overflow */
@@ -3441,13 +3365,6 @@ read_into_scalar_list(char *initial_name,
{
case T_DATUM:
check_assignable(yylval.wdatum.datum, yylloc);
- if (yylval.wdatum.datum->dtype == PLPGSQL_DTYPE_ROW ||
- yylval.wdatum.datum->dtype == PLPGSQL_DTYPE_REC)
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("\"%s\" is not a scalar variable",
- NameOfDatum(&(yylval.wdatum))),
- parser_errposition(yylloc)));
fieldnames[nfields] = NameOfDatum(&(yylval.wdatum));
varnos[nfields++] = yylval.wdatum.datum->dno;
break;
@@ -3464,23 +3381,65 @@ read_into_scalar_list(char *initial_name,
*/
plpgsql_push_back_token(tok);
- row = palloc(sizeof(PLpgSQL_row));
- row->dtype = PLPGSQL_DTYPE_ROW;
- row->refname = pstrdup("*internal*");
- row->lineno = plpgsql_location_to_lineno(initial_location);
- row->rowtupdesc = NULL;
- row->nfields = nfields;
- row->fieldnames = palloc(sizeof(char *) * nfields);
- row->varnos = palloc(sizeof(int) * nfields);
+ /* leave when new row var is not necessary */
+ if (nfields == 1 &&
+ (*row != NULL || *rec != NULL || scalar != NULL))
+ return;
+
+ auxrow = palloc(sizeof(PLpgSQL_row));
+ auxrow->dtype = PLPGSQL_DTYPE_ROW;
+ auxrow->refname = pstrdup("*internal*");
+ auxrow->lineno = plpgsql_location_to_lineno(initial_location);
+ auxrow->rowtupdesc = NULL;
+ auxrow->nfields = nfields;
+ auxrow->fieldnames = palloc(sizeof(char *) * nfields);
+ auxrow->varnos = palloc(sizeof(int) * nfields);
while (--nfields >= 0)
{
- row->fieldnames[nfields] = fieldnames[nfields];
- row->varnos[nfields] = varnos[nfields];
+ auxrow->fieldnames[nfields] = fieldnames[nfields];
+ auxrow->varnos[nfields] = varnos[nfields];
}
- plpgsql_adddatum((PLpgSQL_datum *)row);
+ plpgsql_adddatum((PLpgSQL_datum *)auxrow);
- return row;
+ /* result should not be rec */
+ *rec = NULL;
+ *row = auxrow;
+}
+
+
+/*
+ * Read the argument of an INTO clause. On entry, we have just read the
+ * INTO keyword.
+ */
+static void
+read_into_target(PLpgSQL_rec **rec, PLpgSQL_row **row, bool *strict)
+{
+ int tok;
+
+ /* Set default results */
+ if (strict)
+ *strict = false;
+
+ tok = yylex();
+ if (strict && tok == K_STRICT)
+ {
+ *strict = true;
+ tok = yylex();
+ }
+
+ switch (tok)
+ {
+ case T_DATUM:
+ read_into_list(NameOfDatum(&(yylval.wdatum)),
+ yylval.wdatum.datum, yylloc,
+ NULL, rec, row);
+ break;
+
+ default:
+ /* just to give a better message than "syntax error" */
+ current_token_is_not_variable(tok);
+ }
}
/*
diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index 7ebbde6..49829ec 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -5979,3 +5979,41 @@ LINE 1: SELECT (SELECT string_agg(id || '=' || name, ',') FROM d)
^
QUERY: SELECT (SELECT string_agg(id || '=' || name, ',') FROM d)
CONTEXT: PL/pgSQL function alter_table_under_transition_tables_upd_func() line 3 at RAISE
+CREATE TYPE ct1 AS (a int, b numeric, c varchar);
+CREATE TYPE ct2 AS (a varchar, b int, c date);
+CREATE OR REPLACE FUNCTION multiout(IN id int, OUT v1 ct1, OUT v2 ct2, OUT v3 text)
+AS $$
+BEGIN
+ v1.a := 10;
+ v1.b := 3.14;
+ v1.c := 'ok';
+ v2.a := 'without any error';
+ v2.b := 45442;
+ v2.c := '20170514';
+ v3 := 'no error';
+END;
+$$ LANGUAGE plpgsql;
+DO $$
+DECLARE v1 ct1; v2 ct2; v3 text;
+BEGIN
+ SELECT * FROM multiout(10) INTO v1, v2, v3;
+ RAISE NOTICE 'v1 := %', v1;
+ RAISE NOTICE 'v2 := %', v2;
+ RAISE NOTICE 'v3 := %', v3;
+END;
+$$;
+NOTICE: v1 := (10,3.14,ok)
+NOTICE: v2 := ("without any error",45442,05-14-2017)
+NOTICE: v3 := no error
+-- should fail
+DO $$
+DECLARE v1 ct1; v2 ct2; v3 text;
+BEGIN
+ SELECT * FROM multiout(10) INTO v2, v1, v3;
+ RAISE NOTICE 'v1 := %', v1;
+ RAISE NOTICE 'v2 := %', v2;
+ RAISE NOTICE 'v3 := %', v3;
+END;
+$$;
+ERROR: invalid input syntax for type date: "ok"
+CONTEXT: PL/pgSQL function inline_code_block line 4 at SQL statement
diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index 60d1d38..0c67d76 100644
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -4766,3 +4766,41 @@ ALTER TABLE alter_table_under_transition_tables
DROP column name;
UPDATE alter_table_under_transition_tables
SET id = id;
+
+CREATE TYPE ct1 AS (a int, b numeric, c varchar);
+CREATE TYPE ct2 AS (a varchar, b int, c date);
+
+CREATE OR REPLACE FUNCTION multiout(IN id int, OUT v1 ct1, OUT v2 ct2, OUT v3 text)
+AS $$
+BEGIN
+ v1.a := 10;
+ v1.b := 3.14;
+ v1.c := 'ok';
+ v2.a := 'without any error';
+ v2.b := 45442;
+ v2.c := '20170514';
+ v3 := 'no error';
+END;
+$$ LANGUAGE plpgsql;
+
+DO $$
+DECLARE v1 ct1; v2 ct2; v3 text;
+BEGIN
+ SELECT * FROM multiout(10) INTO v1, v2, v3;
+ RAISE NOTICE 'v1 := %', v1;
+ RAISE NOTICE 'v2 := %', v2;
+ RAISE NOTICE 'v3 := %', v3;
+END;
+$$;
+
+-- should fail
+DO $$
+DECLARE v1 ct1; v2 ct2; v3 text;
+BEGIN
+ SELECT * FROM multiout(10) INTO v2, v1, v3;
+ RAISE NOTICE 'v1 := %', v1;
+ RAISE NOTICE 'v2 := %', v2;
+ RAISE NOTICE 'v3 := %', v3;
+END;
+$$;
+
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested
I'm afraid this patch conflicts with current master branch.
The new status of this patch is: Waiting on Author
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2017-09-07 8:08 GMT+02:00 Anthony Bykov <a.bykov@postgrespro.ru>:
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not testedI'm afraid this patch conflicts with current master branch.
The new status of this patch is: Waiting on Author
rebased
Show quoted text
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachments:
psql-named-arguments-02.patchtext/x-patch; charset=US-ASCII; name=psql-named-arguments-02.patchDownload
diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
index a637551..49327b2 100644
--- a/src/pl/plpgsql/src/pl_comp.c
+++ b/src/pl/plpgsql/src/pl_comp.c
@@ -409,6 +409,7 @@ do_compile(FunctionCallInfo fcinfo,
for (i = 0; i < numargs; i++)
{
char buf[32];
+ char *argname = NULL;
Oid argtypeid = argtypes[i];
char argmode = argmodes ? argmodes[i] : PROARGMODE_IN;
PLpgSQL_type *argdtype;
@@ -433,9 +434,12 @@ do_compile(FunctionCallInfo fcinfo,
errmsg("PL/pgSQL functions cannot accept type %s",
format_type_be(argtypeid))));
+ argname = (argnames && argnames[i][0] != 0) ? argnames[i] : NULL;
+
/* Build variable and add to datum list */
- argvariable = plpgsql_build_variable(buf, 0,
- argdtype, false);
+ argvariable = plpgsql_build_variable(argname != NULL ?
+ argname : buf,
+ 0, argdtype, false);
if (argvariable->dtype == PLPGSQL_DTYPE_VAR)
{
@@ -461,9 +465,9 @@ do_compile(FunctionCallInfo fcinfo,
add_parameter_name(argitemtype, argvariable->dno, buf);
/* If there's a name for the argument, make an alias */
- if (argnames && argnames[i][0] != '\0')
+ if (argname != '\0')
add_parameter_name(argitemtype, argvariable->dno,
- argnames[i]);
+ argname);
}
/*
diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index 7ebbde6..a9fe736 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -5979,3 +5979,15 @@ LINE 1: SELECT (SELECT string_agg(id || '=' || name, ',') FROM d)
^
QUERY: SELECT (SELECT string_agg(id || '=' || name, ',') FROM d)
CONTEXT: PL/pgSQL function alter_table_under_transition_tables_upd_func() line 3 at RAISE
+-- should fail -- message should to contain argument name
+CREATE TYPE ct AS (a int, b int);
+CREATE OR REPLACE FUNCTION fx(x ct)
+RETURNS void AS $$
+BEGIN
+ GET DIAGNOSTICS x = ROW_COUNT;
+end;
+$$ language plpgsql;
+ERROR: "x" is not a scalar variable
+LINE 4: GET DIAGNOSTICS x = ROW_COUNT;
+ ^
+DROP TYPE ct;
diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index 60d1d38..4716ac0 100644
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -4766,3 +4766,15 @@ ALTER TABLE alter_table_under_transition_tables
DROP column name;
UPDATE alter_table_under_transition_tables
SET id = id;
+
+-- should fail -- message should to contain argument name
+CREATE TYPE ct AS (a int, b int);
+
+CREATE OR REPLACE FUNCTION fx(x ct)
+RETURNS void AS $$
+BEGIN
+ GET DIAGNOSTICS x = ROW_COUNT;
+end;
+$$ language plpgsql;
+
+DROP TYPE ct;
2017-09-07 19:48 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:
2017-09-07 8:08 GMT+02:00 Anthony Bykov <a.bykov@postgrespro.ru>:
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not testedI'm afraid this patch conflicts with current master branch.
The new status of this patch is: Waiting on Author
rebased
I am sorry - wrong patch
I am sending correct patch
Regards
Pavel
Show quoted text
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachments:
plpgsql-into-multitarget-2.sqlapplication/sql; name=plpgsql-into-multitarget-2.sqlDownload
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index 94f1f58593..4b6bf0b5bc 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -92,9 +92,10 @@ static char *NameOfDatum(PLwdatum *wdatum);
static void check_assignable(PLpgSQL_datum *datum, int location);
static void read_into_target(PLpgSQL_rec **rec, PLpgSQL_row **row,
bool *strict);
-static PLpgSQL_row *read_into_scalar_list(char *initial_name,
- PLpgSQL_datum *initial_datum,
- int initial_location);
+static void read_into_list(char *initial_name,
+ PLpgSQL_datum *initial_datum, int initial_location,
+ PLpgSQL_datum **scalar,
+ PLpgSQL_rec **rec, PLpgSQL_row **row);
static PLpgSQL_row *make_scalar_list1(char *initial_name,
PLpgSQL_datum *initial_datum,
int lineno, int location);
@@ -1558,33 +1559,9 @@ for_variable : T_DATUM
{
$$.name = NameOfDatum(&($1));
$$.lineno = plpgsql_location_to_lineno(@1);
- if ($1.datum->dtype == PLPGSQL_DTYPE_ROW)
- {
- $$.scalar = NULL;
- $$.rec = NULL;
- $$.row = (PLpgSQL_row *) $1.datum;
- }
- else if ($1.datum->dtype == PLPGSQL_DTYPE_REC)
- {
- $$.scalar = NULL;
- $$.rec = (PLpgSQL_rec *) $1.datum;
- $$.row = NULL;
- }
- else
- {
- int tok;
- $$.scalar = $1.datum;
- $$.rec = NULL;
- $$.row = NULL;
- /* check for comma-separated list */
- tok = yylex();
- plpgsql_push_back_token(tok);
- if (tok == ',')
- $$.row = read_into_scalar_list($$.name,
- $$.scalar,
- @1);
- }
+ read_into_list($$.name, $1.datum, @1,
+ &$$.scalar, &$$.rec, &$$.row);
}
| T_WORD
{
@@ -3337,89 +3314,21 @@ check_assignable(PLpgSQL_datum *datum, int location)
}
/*
- * Read the argument of an INTO clause. On entry, we have just read the
- * INTO keyword.
- */
-static void
-read_into_target(PLpgSQL_rec **rec, PLpgSQL_row **row, bool *strict)
-{
- int tok;
-
- /* Set default results */
- *rec = NULL;
- *row = NULL;
- if (strict)
- *strict = false;
-
- tok = yylex();
- if (strict && tok == K_STRICT)
- {
- *strict = true;
- tok = yylex();
- }
-
- /*
- * Currently, a row or record variable can be the single INTO target,
- * but not a member of a multi-target list. So we throw error if there
- * is a comma after it, because that probably means the user tried to
- * write a multi-target list. If this ever gets generalized, we should
- * probably refactor read_into_scalar_list so it handles all cases.
- */
- switch (tok)
- {
- case T_DATUM:
- if (yylval.wdatum.datum->dtype == PLPGSQL_DTYPE_ROW)
- {
- check_assignable(yylval.wdatum.datum, yylloc);
- *row = (PLpgSQL_row *) yylval.wdatum.datum;
-
- if ((tok = yylex()) == ',')
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("record or row variable cannot be part of multiple-item INTO list"),
- parser_errposition(yylloc)));
- plpgsql_push_back_token(tok);
- }
- else if (yylval.wdatum.datum->dtype == PLPGSQL_DTYPE_REC)
- {
- check_assignable(yylval.wdatum.datum, yylloc);
- *rec = (PLpgSQL_rec *) yylval.wdatum.datum;
-
- if ((tok = yylex()) == ',')
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("record or row variable cannot be part of multiple-item INTO list"),
- parser_errposition(yylloc)));
- plpgsql_push_back_token(tok);
- }
- else
- {
- *row = read_into_scalar_list(NameOfDatum(&(yylval.wdatum)),
- yylval.wdatum.datum, yylloc);
- }
- break;
-
- default:
- /* just to give a better message than "syntax error" */
- current_token_is_not_variable(tok);
- }
-}
-
-/*
* Given the first datum and name in the INTO list, continue to read
- * comma-separated scalar variables until we run out. Then construct
+ * comma-separated variables until we run out. Then construct
* and return a fake "row" variable that represents the list of
- * scalars.
+ * fields. When there is only one rec or row field, then return
+ * this variable without nesting.
*/
-static PLpgSQL_row *
-read_into_scalar_list(char *initial_name,
- PLpgSQL_datum *initial_datum,
- int initial_location)
+static void
+read_into_list(char *initial_name,
+ PLpgSQL_datum *initial_datum, int initial_location,
+ PLpgSQL_datum **scalar, PLpgSQL_rec **rec, PLpgSQL_row **row)
{
int nfields;
char *fieldnames[1024];
int varnos[1024];
- PLpgSQL_row *row;
+ PLpgSQL_row *auxrow;
int tok;
check_assignable(initial_datum, initial_location);
@@ -3427,6 +3336,21 @@ read_into_scalar_list(char *initial_name,
varnos[0] = initial_datum->dno;
nfields = 1;
+ *rec = NULL;
+ *row = NULL;
+ if (scalar)
+ *scalar = NULL;
+
+ /*
+ * save row or rec if list has only one field.
+ */
+ if (initial_datum->dtype == PLPGSQL_DTYPE_ROW)
+ *row = (PLpgSQL_row *) initial_datum;
+ else if (initial_datum->dtype == PLPGSQL_DTYPE_REC)
+ *rec = (PLpgSQL_rec *) initial_datum;
+ else if (scalar != NULL)
+ *scalar = initial_datum;
+
while ((tok = yylex()) == ',')
{
/* Check for array overflow */
@@ -3441,13 +3365,6 @@ read_into_scalar_list(char *initial_name,
{
case T_DATUM:
check_assignable(yylval.wdatum.datum, yylloc);
- if (yylval.wdatum.datum->dtype == PLPGSQL_DTYPE_ROW ||
- yylval.wdatum.datum->dtype == PLPGSQL_DTYPE_REC)
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("\"%s\" is not a scalar variable",
- NameOfDatum(&(yylval.wdatum))),
- parser_errposition(yylloc)));
fieldnames[nfields] = NameOfDatum(&(yylval.wdatum));
varnos[nfields++] = yylval.wdatum.datum->dno;
break;
@@ -3464,23 +3381,64 @@ read_into_scalar_list(char *initial_name,
*/
plpgsql_push_back_token(tok);
- row = palloc(sizeof(PLpgSQL_row));
- row->dtype = PLPGSQL_DTYPE_ROW;
- row->refname = pstrdup("*internal*");
- row->lineno = plpgsql_location_to_lineno(initial_location);
- row->rowtupdesc = NULL;
- row->nfields = nfields;
- row->fieldnames = palloc(sizeof(char *) * nfields);
- row->varnos = palloc(sizeof(int) * nfields);
+ /* leave when new row var is not necessary */
+ if (nfields == 1 && (*row != NULL || *rec != NULL || scalar != NULL))
+ return;
+
+ auxrow = palloc(sizeof(PLpgSQL_row));
+ auxrow->dtype = PLPGSQL_DTYPE_ROW;
+ auxrow->refname = pstrdup("*internal*");
+ auxrow->lineno = plpgsql_location_to_lineno(initial_location);
+ auxrow->rowtupdesc = NULL;
+ auxrow->nfields = nfields;
+ auxrow->fieldnames = palloc(sizeof(char *) * nfields);
+ auxrow->varnos = palloc(sizeof(int) * nfields);
while (--nfields >= 0)
{
- row->fieldnames[nfields] = fieldnames[nfields];
- row->varnos[nfields] = varnos[nfields];
+ auxrow->fieldnames[nfields] = fieldnames[nfields];
+ auxrow->varnos[nfields] = varnos[nfields];
}
- plpgsql_adddatum((PLpgSQL_datum *)row);
+ plpgsql_adddatum((PLpgSQL_datum *)auxrow);
- return row;
+ /* result should not be rec */
+ *rec = NULL;
+ *row = auxrow;
+}
+
+
+/*
+ * Read the argument of an INTO clause. On entry, we have just read the
+ * INTO keyword.
+ */
+static void
+read_into_target(PLpgSQL_rec **rec, PLpgSQL_row **row, bool *strict)
+{
+ int tok;
+
+ /* Set default results */
+ if (strict)
+ *strict = false;
+
+ tok = yylex();
+ if (strict && tok == K_STRICT)
+ {
+ *strict = true;
+ tok = yylex();
+ }
+
+ switch (tok)
+ {
+ case T_DATUM:
+ read_into_list(NameOfDatum(&(yylval.wdatum)),
+ yylval.wdatum.datum, yylloc,
+ NULL, rec, row);
+ break;
+
+ default:
+ /* just to give a better message than "syntax error" */
+ current_token_is_not_variable(tok);
+ }
}
/*
diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index 71099969a4..e2f7bbbae6 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -6029,3 +6029,41 @@ SELECT * FROM list_partitioned_table() AS t;
2
(2 rows)
+CREATE TYPE ct1 AS (a int, b numeric, c varchar);
+CREATE TYPE ct2 AS (a varchar, b int, c date);
+CREATE OR REPLACE FUNCTION multiout(IN id int, OUT v1 ct1, OUT v2 ct2, OUT v3 text)
+AS $$
+BEGIN
+ v1.a := 10;
+ v1.b := 3.14;
+ v1.c := 'ok';
+ v2.a := 'without any error';
+ v2.b := 45442;
+ v2.c := '20170514';
+ v3 := 'no error';
+END;
+$$ LANGUAGE plpgsql;
+DO $$
+DECLARE v1 ct1; v2 ct2; v3 text;
+BEGIN
+ SELECT * FROM multiout(10) INTO v1, v2, v3;
+ RAISE NOTICE 'v1 := %', v1;
+ RAISE NOTICE 'v2 := %', v2;
+ RAISE NOTICE 'v3 := %', v3;
+END;
+$$;
+NOTICE: v1 := (10,3.14,ok)
+NOTICE: v2 := ("without any error",45442,05-14-2017)
+NOTICE: v3 := no error
+-- should fail
+DO $$
+DECLARE v1 ct1; v2 ct2; v3 text;
+BEGIN
+ SELECT * FROM multiout(10) INTO v2, v1, v3;
+ RAISE NOTICE 'v1 := %', v1;
+ RAISE NOTICE 'v2 := %', v2;
+ RAISE NOTICE 'v3 := %', v3;
+END;
+$$;
+ERROR: invalid input syntax for type date: "ok"
+CONTEXT: PL/pgSQL function inline_code_block line 4 at SQL statement
diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index 771d68282e..d471f2e99f 100644
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -4811,3 +4811,40 @@ BEGIN
END; $$ LANGUAGE plpgsql;
SELECT * FROM list_partitioned_table() AS t;
+
+CREATE TYPE ct1 AS (a int, b numeric, c varchar);
+CREATE TYPE ct2 AS (a varchar, b int, c date);
+
+CREATE OR REPLACE FUNCTION multiout(IN id int, OUT v1 ct1, OUT v2 ct2, OUT v3 text)
+AS $$
+BEGIN
+ v1.a := 10;
+ v1.b := 3.14;
+ v1.c := 'ok';
+ v2.a := 'without any error';
+ v2.b := 45442;
+ v2.c := '20170514';
+ v3 := 'no error';
+END;
+$$ LANGUAGE plpgsql;
+
+DO $$
+DECLARE v1 ct1; v2 ct2; v3 text;
+BEGIN
+ SELECT * FROM multiout(10) INTO v1, v2, v3;
+ RAISE NOTICE 'v1 := %', v1;
+ RAISE NOTICE 'v2 := %', v2;
+ RAISE NOTICE 'v3 := %', v3;
+END;
+$$;
+
+-- should fail
+DO $$
+DECLARE v1 ct1; v2 ct2; v3 text;
+BEGIN
+ SELECT * FROM multiout(10) INTO v2, v1, v3;
+ RAISE NOTICE 'v1 := %', v1;
+ RAISE NOTICE 'v2 := %', v2;
+ RAISE NOTICE 'v3 := %', v3;
+END;
+$$;
Hi
I am sending rebased patch
Regards
Pavel
Attachments:
plpgsql-into-multitarget3.patchtext/x-patch; charset=US-ASCII; name=plpgsql-into-multitarget3.patchDownload
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index 94f1f58593..4b6bf0b5bc 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -92,9 +92,10 @@ static char *NameOfDatum(PLwdatum *wdatum);
static void check_assignable(PLpgSQL_datum *datum, int location);
static void read_into_target(PLpgSQL_rec **rec, PLpgSQL_row **row,
bool *strict);
-static PLpgSQL_row *read_into_scalar_list(char *initial_name,
- PLpgSQL_datum *initial_datum,
- int initial_location);
+static void read_into_list(char *initial_name,
+ PLpgSQL_datum *initial_datum, int initial_location,
+ PLpgSQL_datum **scalar,
+ PLpgSQL_rec **rec, PLpgSQL_row **row);
static PLpgSQL_row *make_scalar_list1(char *initial_name,
PLpgSQL_datum *initial_datum,
int lineno, int location);
@@ -1558,33 +1559,9 @@ for_variable : T_DATUM
{
$$.name = NameOfDatum(&($1));
$$.lineno = plpgsql_location_to_lineno(@1);
- if ($1.datum->dtype == PLPGSQL_DTYPE_ROW)
- {
- $$.scalar = NULL;
- $$.rec = NULL;
- $$.row = (PLpgSQL_row *) $1.datum;
- }
- else if ($1.datum->dtype == PLPGSQL_DTYPE_REC)
- {
- $$.scalar = NULL;
- $$.rec = (PLpgSQL_rec *) $1.datum;
- $$.row = NULL;
- }
- else
- {
- int tok;
- $$.scalar = $1.datum;
- $$.rec = NULL;
- $$.row = NULL;
- /* check for comma-separated list */
- tok = yylex();
- plpgsql_push_back_token(tok);
- if (tok == ',')
- $$.row = read_into_scalar_list($$.name,
- $$.scalar,
- @1);
- }
+ read_into_list($$.name, $1.datum, @1,
+ &$$.scalar, &$$.rec, &$$.row);
}
| T_WORD
{
@@ -3337,89 +3314,21 @@ check_assignable(PLpgSQL_datum *datum, int location)
}
/*
- * Read the argument of an INTO clause. On entry, we have just read the
- * INTO keyword.
- */
-static void
-read_into_target(PLpgSQL_rec **rec, PLpgSQL_row **row, bool *strict)
-{
- int tok;
-
- /* Set default results */
- *rec = NULL;
- *row = NULL;
- if (strict)
- *strict = false;
-
- tok = yylex();
- if (strict && tok == K_STRICT)
- {
- *strict = true;
- tok = yylex();
- }
-
- /*
- * Currently, a row or record variable can be the single INTO target,
- * but not a member of a multi-target list. So we throw error if there
- * is a comma after it, because that probably means the user tried to
- * write a multi-target list. If this ever gets generalized, we should
- * probably refactor read_into_scalar_list so it handles all cases.
- */
- switch (tok)
- {
- case T_DATUM:
- if (yylval.wdatum.datum->dtype == PLPGSQL_DTYPE_ROW)
- {
- check_assignable(yylval.wdatum.datum, yylloc);
- *row = (PLpgSQL_row *) yylval.wdatum.datum;
-
- if ((tok = yylex()) == ',')
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("record or row variable cannot be part of multiple-item INTO list"),
- parser_errposition(yylloc)));
- plpgsql_push_back_token(tok);
- }
- else if (yylval.wdatum.datum->dtype == PLPGSQL_DTYPE_REC)
- {
- check_assignable(yylval.wdatum.datum, yylloc);
- *rec = (PLpgSQL_rec *) yylval.wdatum.datum;
-
- if ((tok = yylex()) == ',')
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("record or row variable cannot be part of multiple-item INTO list"),
- parser_errposition(yylloc)));
- plpgsql_push_back_token(tok);
- }
- else
- {
- *row = read_into_scalar_list(NameOfDatum(&(yylval.wdatum)),
- yylval.wdatum.datum, yylloc);
- }
- break;
-
- default:
- /* just to give a better message than "syntax error" */
- current_token_is_not_variable(tok);
- }
-}
-
-/*
* Given the first datum and name in the INTO list, continue to read
- * comma-separated scalar variables until we run out. Then construct
+ * comma-separated variables until we run out. Then construct
* and return a fake "row" variable that represents the list of
- * scalars.
+ * fields. When there is only one rec or row field, then return
+ * this variable without nesting.
*/
-static PLpgSQL_row *
-read_into_scalar_list(char *initial_name,
- PLpgSQL_datum *initial_datum,
- int initial_location)
+static void
+read_into_list(char *initial_name,
+ PLpgSQL_datum *initial_datum, int initial_location,
+ PLpgSQL_datum **scalar, PLpgSQL_rec **rec, PLpgSQL_row **row)
{
int nfields;
char *fieldnames[1024];
int varnos[1024];
- PLpgSQL_row *row;
+ PLpgSQL_row *auxrow;
int tok;
check_assignable(initial_datum, initial_location);
@@ -3427,6 +3336,21 @@ read_into_scalar_list(char *initial_name,
varnos[0] = initial_datum->dno;
nfields = 1;
+ *rec = NULL;
+ *row = NULL;
+ if (scalar)
+ *scalar = NULL;
+
+ /*
+ * save row or rec if list has only one field.
+ */
+ if (initial_datum->dtype == PLPGSQL_DTYPE_ROW)
+ *row = (PLpgSQL_row *) initial_datum;
+ else if (initial_datum->dtype == PLPGSQL_DTYPE_REC)
+ *rec = (PLpgSQL_rec *) initial_datum;
+ else if (scalar != NULL)
+ *scalar = initial_datum;
+
while ((tok = yylex()) == ',')
{
/* Check for array overflow */
@@ -3441,13 +3365,6 @@ read_into_scalar_list(char *initial_name,
{
case T_DATUM:
check_assignable(yylval.wdatum.datum, yylloc);
- if (yylval.wdatum.datum->dtype == PLPGSQL_DTYPE_ROW ||
- yylval.wdatum.datum->dtype == PLPGSQL_DTYPE_REC)
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("\"%s\" is not a scalar variable",
- NameOfDatum(&(yylval.wdatum))),
- parser_errposition(yylloc)));
fieldnames[nfields] = NameOfDatum(&(yylval.wdatum));
varnos[nfields++] = yylval.wdatum.datum->dno;
break;
@@ -3464,23 +3381,64 @@ read_into_scalar_list(char *initial_name,
*/
plpgsql_push_back_token(tok);
- row = palloc(sizeof(PLpgSQL_row));
- row->dtype = PLPGSQL_DTYPE_ROW;
- row->refname = pstrdup("*internal*");
- row->lineno = plpgsql_location_to_lineno(initial_location);
- row->rowtupdesc = NULL;
- row->nfields = nfields;
- row->fieldnames = palloc(sizeof(char *) * nfields);
- row->varnos = palloc(sizeof(int) * nfields);
+ /* leave when new row var is not necessary */
+ if (nfields == 1 && (*row != NULL || *rec != NULL || scalar != NULL))
+ return;
+
+ auxrow = palloc(sizeof(PLpgSQL_row));
+ auxrow->dtype = PLPGSQL_DTYPE_ROW;
+ auxrow->refname = pstrdup("*internal*");
+ auxrow->lineno = plpgsql_location_to_lineno(initial_location);
+ auxrow->rowtupdesc = NULL;
+ auxrow->nfields = nfields;
+ auxrow->fieldnames = palloc(sizeof(char *) * nfields);
+ auxrow->varnos = palloc(sizeof(int) * nfields);
while (--nfields >= 0)
{
- row->fieldnames[nfields] = fieldnames[nfields];
- row->varnos[nfields] = varnos[nfields];
+ auxrow->fieldnames[nfields] = fieldnames[nfields];
+ auxrow->varnos[nfields] = varnos[nfields];
}
- plpgsql_adddatum((PLpgSQL_datum *)row);
+ plpgsql_adddatum((PLpgSQL_datum *)auxrow);
- return row;
+ /* result should not be rec */
+ *rec = NULL;
+ *row = auxrow;
+}
+
+
+/*
+ * Read the argument of an INTO clause. On entry, we have just read the
+ * INTO keyword.
+ */
+static void
+read_into_target(PLpgSQL_rec **rec, PLpgSQL_row **row, bool *strict)
+{
+ int tok;
+
+ /* Set default results */
+ if (strict)
+ *strict = false;
+
+ tok = yylex();
+ if (strict && tok == K_STRICT)
+ {
+ *strict = true;
+ tok = yylex();
+ }
+
+ switch (tok)
+ {
+ case T_DATUM:
+ read_into_list(NameOfDatum(&(yylval.wdatum)),
+ yylval.wdatum.datum, yylloc,
+ NULL, rec, row);
+ break;
+
+ default:
+ /* just to give a better message than "syntax error" */
+ current_token_is_not_variable(tok);
+ }
}
/*
diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index 7d3e9225bb..320fa68d99 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -6040,3 +6040,41 @@ END; $$ LANGUAGE plpgsql;
ERROR: "x" is not a scalar variable
LINE 3: GET DIAGNOSTICS x = ROW_COUNT;
^
+CREATE TYPE ct1 AS (a int, b numeric, c varchar);
+CREATE TYPE ct2 AS (a varchar, b int, c date);
+CREATE OR REPLACE FUNCTION multiout(IN id int, OUT v1 ct1, OUT v2 ct2, OUT v3 text)
+AS $$
+BEGIN
+ v1.a := 10;
+ v1.b := 3.14;
+ v1.c := 'ok';
+ v2.a := 'without any error';
+ v2.b := 45442;
+ v2.c := '20170514';
+ v3 := 'no error';
+END;
+$$ LANGUAGE plpgsql;
+DO $$
+DECLARE v1 ct1; v2 ct2; v3 text;
+BEGIN
+ SELECT * FROM multiout(10) INTO v1, v2, v3;
+ RAISE NOTICE 'v1 := %', v1;
+ RAISE NOTICE 'v2 := %', v2;
+ RAISE NOTICE 'v3 := %', v3;
+END;
+$$;
+NOTICE: v1 := (10,3.14,ok)
+NOTICE: v2 := ("without any error",45442,05-14-2017)
+NOTICE: v3 := no error
+-- should fail
+DO $$
+DECLARE v1 ct1; v2 ct2; v3 text;
+BEGIN
+ SELECT * FROM multiout(10) INTO v2, v1, v3;
+ RAISE NOTICE 'v1 := %', v1;
+ RAISE NOTICE 'v2 := %', v2;
+ RAISE NOTICE 'v3 := %', v3;
+END;
+$$;
+ERROR: invalid input syntax for type date: "ok"
+CONTEXT: PL/pgSQL function inline_code_block line 4 at SQL statement
diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index 6c9399696b..15da3d709f 100644
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -4820,3 +4820,40 @@ BEGIN
GET DIAGNOSTICS x = ROW_COUNT;
RETURN;
END; $$ LANGUAGE plpgsql;
+
+CREATE TYPE ct1 AS (a int, b numeric, c varchar);
+CREATE TYPE ct2 AS (a varchar, b int, c date);
+
+CREATE OR REPLACE FUNCTION multiout(IN id int, OUT v1 ct1, OUT v2 ct2, OUT v3 text)
+AS $$
+BEGIN
+ v1.a := 10;
+ v1.b := 3.14;
+ v1.c := 'ok';
+ v2.a := 'without any error';
+ v2.b := 45442;
+ v2.c := '20170514';
+ v3 := 'no error';
+END;
+$$ LANGUAGE plpgsql;
+
+DO $$
+DECLARE v1 ct1; v2 ct2; v3 text;
+BEGIN
+ SELECT * FROM multiout(10) INTO v1, v2, v3;
+ RAISE NOTICE 'v1 := %', v1;
+ RAISE NOTICE 'v2 := %', v2;
+ RAISE NOTICE 'v3 := %', v3;
+END;
+$$;
+
+-- should fail
+DO $$
+DECLARE v1 ct1; v2 ct2; v3 text;
+BEGIN
+ SELECT * FROM multiout(10) INTO v2, v1, v3;
+ RAISE NOTICE 'v1 := %', v1;
+ RAISE NOTICE 'v2 := %', v2;
+ RAISE NOTICE 'v3 := %', v3;
+END;
+$$;
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: tested, failed
Hello,
As far as I understand, this patch adds functionality (correct me if I'm wrong) for users. Shouldn't there be any changes in doc/src/sgml/ with the description of new functionality?
Regards
Anthony
The new status of this patch is: Waiting on Author
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi
2017-09-14 12:33 GMT+02:00 Anthony Bykov <a.bykov@postgrespro.ru>:
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: tested, failedHello,
As far as I understand, this patch adds functionality (correct me if I'm
wrong) for users. Shouldn't there be any changes in doc/src/sgml/ with the
description of new functionality?
It removes undocumented limit. I recheck plpgsql documentation and there
are not any rows about prohibited combinations of data types.
There is line:
where command-string is an expression yielding a string (of type text)
containing the command to be executed. The optional target is a record
variable, a row variable, or a comma-separated list of simple variables and
record/row fields, into which the results of the command will be stored.
The optional USING expressions supply values to be inserted into the
command.
what is correct if I understand well with this patch.
Regards
Pavel
Show quoted text
Regards
AnthonyThe new status of this patch is: Waiting on Author
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed
Hello,
I've tested it (make check-world) and as far as I understand, it works fine.
The new status of this patch is: Ready for Committer
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2017-09-19 11:43 GMT+02:00 Anthony Bykov <a.bykov@postgrespro.ru>:
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passedHello,
I've tested it (make check-world) and as far as I understand, it works
fine.The new status of this patch is: Ready for Committer
Thank you very much
Pavel
Show quoted text
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Pavel Stehule <pavel.stehule@gmail.com> writes:
2017-09-14 12:33 GMT+02:00 Anthony Bykov <a.bykov@postgrespro.ru>:
As far as I understand, this patch adds functionality (correct me if I'm
wrong) for users. Shouldn't there be any changes in doc/src/sgml/ with the
description of new functionality?
It removes undocumented limit. I recheck plpgsql documentation and there
are not any rows about prohibited combinations of data types.
I remain of the opinion that this patch is a fundamentally bad idea.
It creates an inconsistency between what happens if the INTO target list
is a single record/row variable (it absorbs all the columns of the query
output) and what happens if a record/row variable is part of a
multi-variable target list (now it just absorbs one column, which had
better be composite). That's going to confuse people, especially since
you haven't documented it. But even with documentation, it doesn't seem
like good design. Aside from being inconsistent, it doesn't cover all
the cases --- what if you have just one query output column, that is
composite, and you'd like it to go into a composite variable? That
doesn't work today, and this patch doesn't fix it, but it does create
enough confusion that we never would be able to fix it.
I'd be much happier if there were some notational difference
between I-want-the-composite-variable-to-absorb-a-composite-column
and I-want-the-composite-variable-to-absorb-N-scalar-columns.
For backwards compatibility with what happens now, the latter would
have to be the default. I'm wondering about "var.*" or "(var)" as
the notation signaling that you want the former, though neither of
those seem like they're very intuitive.
If we had a notation like that, it'd be possible to ask for either
behavior within a larger target list, except that we'd still need
to say that I-want-a-RECORD-variable-to-absorb-N-scalar-columns
has to be the only thing in its target list. Otherwise it's not
very clear what N ought to be. (In some cases maybe you could
reverse-engineer N from context, but I think that'd be overly
complicated and bug prone.)
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, May 13, 2017 at 4:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
IIRC, the reason for disallowing that is that it's totally unclear what
the semantics ought to be. Is that variable a single target (demanding
a compatible composite-valued column from the source query), or does it
eat one source column per field within the record/row? The former is 100%
inconsistent with what happens if the record/row is the only INTO target;
while the latter would be very bug-prone, and it's especially unclear what
ought to happen if it's an as-yet-undefined record variable.
Maybe I'm just confused here, but do you think that anyone at all
wants the second behavior?
I think the fact that single-target INTO lists and multiple-target
INTO lists are handled completely differently is extremely poor
language design. It would have been far better, as you suggested
downthread, to have added some syntax up front to let people select
the behavior that they want, but I think there's little hope of
changing this now without creating even more pain.
I have a really hard time, however, imagining that anyone writes
SELECT a, b, c, d, e, f, g, h, i, j, k INTO x, y, z and wants some of
a-k to go into x, some more to go into y, and some more to go into z
(and heaven help you if you drop a column from x or y -- now the whole
semantics of the query change, yikes). What's reasonable is to write
SELECT a, b, c INTO x, y, z and have those correspond 1:1.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2017-09-19 20:29 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:
Pavel Stehule <pavel.stehule@gmail.com> writes:
2017-09-14 12:33 GMT+02:00 Anthony Bykov <a.bykov@postgrespro.ru>:
As far as I understand, this patch adds functionality (correct me if I'm
wrong) for users. Shouldn't there be any changes in doc/src/sgml/ withthe
description of new functionality?
It removes undocumented limit. I recheck plpgsql documentation and there
are not any rows about prohibited combinations of data types.I remain of the opinion that this patch is a fundamentally bad idea.
It creates an inconsistency between what happens if the INTO target list
is a single record/row variable (it absorbs all the columns of the query
output) and what happens if a record/row variable is part of a
multi-variable target list (now it just absorbs one column, which had
better be composite). That's going to confuse people, especially since
you haven't documented it. But even with documentation, it doesn't seem
like good design. Aside from being inconsistent, it doesn't cover all
the cases --- what if you have just one query output column, that is
composite, and you'd like it to go into a composite variable? That
doesn't work today, and this patch doesn't fix it, but it does create
enough confusion that we never would be able to fix it.I'd be much happier if there were some notational difference
between I-want-the-composite-variable-to-absorb-a-composite-column
and I-want-the-composite-variable-to-absorb-N-scalar-columns.
For backwards compatibility with what happens now, the latter would
have to be the default. I'm wondering about "var.*" or "(var)" as
the notation signaling that you want the former, though neither of
those seem like they're very intuitive.If we had a notation like that, it'd be possible to ask for either
behavior within a larger target list, except that we'd still need
to say that I-want-a-RECORD-variable-to-absorb-N-scalar-columns
has to be the only thing in its target list. Otherwise it's not
very clear what N ought to be. (In some cases maybe you could
reverse-engineer N from context, but I think that'd be overly
complicated and bug prone.)
I am not sure if I understand to your objection. This patch do nothing with
RECORD variables - where is is impossible or pretty hard to implement any
clean solution.
If we do some sophisticated game with multiple RECORD type variables, then
probably some positional notations has sense, and in this case we cannot to
allow mix scalar and composite values.
so SELECT s,s, C,s,C TO sv, sv, CV, s, RV should be allowed
but
so SELECT s,s, C,s,C TO R, CV, s, RV should be disallowed
Regards
Pavel
Show quoted text
regards, tom lane
On Tuesday, September 19, 2017, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Pavel Stehule <pavel.stehule@gmail.com <javascript:;>> writes:
2017-09-14 12:33 GMT+02:00 Anthony Bykov <a.bykov@postgrespro.ru
<javascript:;>>:
As far as I understand, this patch adds functionality (correct me if I'm
wrong) for users. Shouldn't there be any changes in doc/src/sgml/ withthe
description of new functionality?
It removes undocumented limit. I recheck plpgsql documentation and there
are not any rows about prohibited combinations of data types.I remain of the opinion that this patch is a fundamentally bad idea.
It creates an inconsistency between what happens if the INTO target list
is a single record/row variable (it absorbs all the columns of the query
output) and what happens if a record/row variable is part of a
multi-variable target list (now it just absorbs one column, which had
better be composite). That's going to confuse people, especially since
you haven't documented it. But even with documentation, it doesn't seem
like good design. Aside from being inconsistent, it doesn't cover all
the cases --- what if you have just one query output column, that is
composite, and you'd like it to go into a composite variable? That
doesn't work today, and this patch doesn't fix it, but it does create
enough confusion that we never would be able to fix it.
I think it's worth definitively addressing the limitations
noted, documenting how they are resolved/handled, and then give the
programmer more flexibility while, IMO, marginally increasing complexity.
For me we've programmed the "convenience case" and punted on the
technically correct solution. i.e., we could have chosen to force the user
to write select (c1, c2)::ct into vct.
I'd be much happier if there were some notational difference
between I-want-the-composite-variable-to-absorb-a-composite-column
and I-want-the-composite-variable-to-absorb-N-scalar-columns.
If we change to considering exactly one output column for each target var
this seems unnecessary. Then the one special case is today's single
composite column target and multiple output columns. If there is only one
select column it has to be composite.
David J.
"David G. Johnston" <david.g.johnston@gmail.com> writes:
On Tuesday, September 19, 2017, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I'd be much happier if there were some notational difference
between I-want-the-composite-variable-to-absorb-a-composite-column
and I-want-the-composite-variable-to-absorb-N-scalar-columns.
If we change to considering exactly one output column for each target var
this seems unnecessary.
Breaking backwards compatibility to that extent seems like a nonstarter.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
I think the fact that single-target INTO lists and multiple-target
INTO lists are handled completely differently is extremely poor
language design. It would have been far better, as you suggested
downthread, to have added some syntax up front to let people select
the behavior that they want, but I think there's little hope of
changing this now without creating even more pain.
How so? The proposal I gave is fully backwards-compatible. It's
likely not the way we'd do it in a green field, but we don't have
a green field.
I have a really hard time, however, imagining that anyone writes
SELECT a, b, c, d, e, f, g, h, i, j, k INTO x, y, z and wants some of
a-k to go into x, some more to go into y, and some more to go into z
(and heaven help you if you drop a column from x or y -- now the whole
semantics of the query change, yikes). What's reasonable is to write
SELECT a, b, c INTO x, y, z and have those correspond 1:1.
That's certainly a case that we ought to support somehow. The problem is
staying reasonably consistent with the two-decades-old precedent of the
existing behavior for one target variable.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Sep 19, 2017 at 11:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Aside from being inconsistent, it doesn't cover all
the cases --- what if you have just one query output column, that is
composite, and you'd like it to go into a composite variable? That
doesn't work today, and this patch doesn't fix it, but it does create
enough confusion that we never would be able to fix it.
Actually, this does work, just not the way one would immediately expect.
ct1: (text, text)
DO $$
SELECT ('1', '2')::ct1 INTO c1;
RAISE NOTICE '%', c1;
END;
$$;
Notice: ("(1,2)",)
And so, yes, my thinking has a backward compatibility problem. But one
that isn't fixable when constrained by backward compatibility - whether
this patch goes in or not.
David J.
"David G. Johnston" <david.g.johnston@gmail.com> writes:
Aside from being inconsistent, it doesn't cover all
the cases --- what if you have just one query output column, that is
composite, and you'd like it to go into a composite variable? That
doesn't work today, and this patch doesn't fix it, but it does create
enough confusion that we never would be able to fix it.
Actually, this does work, just not the way one would immediately expect.
Uh ... how did you declare ct1, exactly? I tried this before claiming
it doesn't work, and it doesn't, for me:
create type complex as (r float8, i float8);
create or replace function mkc(a float8, b float8) returns complex
language sql as 'select a,b';
select mkc(1,2);
create or replace function test() returns void language plpgsql as $$
declare c complex;
begin
select mkc(1,2) into c;
raise notice 'c = %', c;
end$$;
select test();
I get
ERROR: invalid input syntax for type double precision: "(1,2)"
CONTEXT: PL/pgSQL function test() line 4 at SQL statement
That's because it's trying to assign the result of mkc() into c.r,
not into the whole composite variable.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
"David G. Johnston" <david.g.johnston@gmail.com> writes:
Actually, this does work, just not the way one would immediately expect.
On closer inspection, what's actually happening in your example is that
you're getting the SELECT's ct1 result crammed into the first column of
c1, and then a default NULL is stuck into its second column:
ct1: (text, text)
DO $$
SELECT ('1', '2')::ct1 INTO c1;
RAISE NOTICE '%', c1;
END;
$$;
Notice: ("(1,2)",)
Notice the parens/comma positioning. It's only because text is such
a lax datatype that you didn't notice the problem.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Sep 19, 2017 at 2:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
"David G. Johnston" <david.g.johnston@gmail.com> writes:
Actually, this does work, just not the way one would immediately expect.
On closer inspection, what's actually happening in your example is that
you're getting the SELECT's ct1 result crammed into the first column of
c1, and then a default NULL is stuck into its second column:ct1: (text, text)
DO $$
SELECT ('1', '2')::ct1 INTO c1;
RAISE NOTICE '%', c1;
END;
$$;Notice: ("(1,2)",)
Notice the parens/comma positioning. It's only because text is such
a lax datatype that you didn't notice the problem.
I saw exactly what you described - that it didn't error out and that the
text representation of the single output composite was being stored in the
first field of the target composite. i.e., that it "worked". Does that
fact that it only works if the first field of the composite type is text
change your opinion that the behavior is OK to break?
David J.
"David G. Johnston" <david.g.johnston@gmail.com> writes:
On Tue, Sep 19, 2017 at 2:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Notice the parens/comma positioning. It's only because text is such
a lax datatype that you didn't notice the problem.
I saw exactly what you described - that it didn't error out and that the
text representation of the single output composite was being stored in the
first field of the target composite. i.e., that it "worked". Does that
fact that it only works if the first field of the composite type is text
change your opinion that the behavior is OK to break?
No. That's not "working" for any useful value of "working".
I would indeed be happy if we could just change this behavior, but I do
not care to answer to the crowd of villagers that will appear if we do
that. It's just way too easy to do, eg,
declare r record;
...
for r in select lots-o-columns from ... loop ...
and then expect r to contain all the columns of the SELECT, separately.
We can't change that behavior now. (And making FOR behave differently
for this purpose than INTO wouldn't be very nice, either.)
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Sep 19, 2017 at 11:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
T
hat
doesn't work today, and this patch doesn't fix it, but it does create
enough confusion that we never would be able to fix it.I'd be much happier if there were some notational difference
between I-want-the-composite-variable-to-absorb-a-composite-column
and I-want-the-composite-variable-to-absorb-N-scalar-columns.
For backwards compatibility with what happens now, the latter would
have to be the default.
So, using "()" syntax
s,t: scalar text
c,d: (text, text)
treat all numbers below as text; and the ((1,2),) as ("(1,2)",)
A. SELECT 1 INTO s; -- today 1, this patch is the same
B. SELECT 1, 2 INTO s; -- today 1, this patch is the same
C. SELECT 1, 2 INTO (s); -- ERROR syntax - scalars cannot be tagged with
(), this patch N/A
D. SELECT 1, 2 INTO s, t; -- today 1, 2, this patch is the same
E. SELECT 1, 2 INTO c; -- today (1,2), this patch is the same
F. SELECT 1, 2 INTO (c); --ERROR "1" cannot be converted to (text, text),
this patch N/A
1. SELECT (1,2) INTO c; -- today ((1,2),); this patch is the same
2. SELECT (1,2) INTO (c); -- (1,2) -- this patch N/A
3. SELECT (1,2),(3,4) INTO c,d; -- ERROR syntax -- this patch gives [I
think...it can be made to give] (1,2),(3,4)
4. SELECT (1,2),(3,4) INTO c,(d); -- ERROR syntax -- this patch N/A
5. SELECT (1,2),(3,4) INTO (c),d; -- ERROR syntax -- this patch N/A
6. SELECT (1,2),(3,4) INTO (c),(d); -- (1,2),(3,4) -- this patch N/A
!. SELECT 1, (2,3), 4 INTO s, (c), t -- 1, (2,3), 4 -- this patch N/A
@. SELECT 1, 2, 3, 4 INTO s, (c), t -- ERROR "2" cannot be made into (text,
text) -- this patch N/A
IOW, this patch, if "c" is the only target (#1) and is composite pretend
the user wrote "INTO c.1, c.2" and assign each output column as a scalar in
one-by-one fashion. If "c" is not the only target column (#3) assign a
single output column to it. This maintains compatibility and clean syntax
at the cost of inconsistency.
The alternate, backward compatible, option introduces mandatory () in the
syntax for all composite columns in a multi-variable target (# 3-5 errors,
#6 valid) while it changes the behavior if present on a single variable
target (#1 vs #2).
So, allowing #3 to work makes implementing #2 even more unappealing.
Making only #2 and #6 work seems like a reasonable alternative position.
The last option is to fix #1 to return (1,2) - cleanly reporting an error
if possible, must like we just did with SRFs, and apply the patch thus
gaining both consistency and a clean syntax at the expense of limited
backward incompatibility.
Arrays not considered; single-column composites might end up looking like
scalars when presented to a (c) target.
Hope this helps someone besides me understand the problem space.
David J.
On 20 Sep 2017, at 01:05, David G. Johnston <david.g.johnston@gmail.com> wrote:
On Tue, Sep 19, 2017 at 11:29 AM, Tom Lane <tgl@sss.pgh.pa.us <mailto:tgl@sss.pgh.pa.us>> wrote:
That doesn't work today, and this patch doesn't fix it, but it does create
enough confusion that we never would be able to fix it.I'd be much happier if there were some notational difference
between I-want-the-composite-variable-to-absorb-a-composite-column
and I-want-the-composite-variable-to-absorb-N-scalar-columns.
For backwards compatibility with what happens now, the latter would
have to be the default.So, using "()" syntax
s,t: scalar text
c,d: (text, text)treat all numbers below as text; and the ((1,2),) as ("(1,2)",)
A. SELECT 1 INTO s; -- today 1, this patch is the same
B. SELECT 1, 2 INTO s; -- today 1, this patch is the same
C. SELECT 1, 2 INTO (s); -- ERROR syntax - scalars cannot be tagged with (), this patch N/A
D. SELECT 1, 2 INTO s, t; -- today 1, 2, this patch is the same
E. SELECT 1, 2 INTO c; -- today (1,2), this patch is the same
F. SELECT 1, 2 INTO (c); --ERROR "1" cannot be converted to (text, text), this patch N/A
1. SELECT (1,2) INTO c; -- today ((1,2),); this patch is the same
2. SELECT (1,2) INTO (c); -- (1,2) -- this patch N/A
3. SELECT (1,2),(3,4) INTO c,d; -- ERROR syntax -- this patch gives [I think...it can be made to give] (1,2),(3,4)
4. SELECT (1,2),(3,4) INTO c,(d); -- ERROR syntax -- this patch N/A
5. SELECT (1,2),(3,4) INTO (c),d; -- ERROR syntax -- this patch N/A
6. SELECT (1,2),(3,4) INTO (c),(d); -- (1,2),(3,4) -- this patch N/A
!. SELECT 1, (2,3), 4 INTO s, (c), t -- 1, (2,3), 4 -- this patch N/A
@. SELECT 1, 2, 3, 4 INTO s, (c), t -- ERROR "2" cannot be made into (text, text) -- this patch N/AIOW, this patch, if "c" is the only target (#1) and is composite pretend the user wrote "INTO c.1, c.2" and assign each output column as a scalar in one-by-one fashion. If "c" is not the only target column (#3) assign a single output column to it. This maintains compatibility and clean syntax at the cost of inconsistency.
The alternate, backward compatible, option introduces mandatory () in the syntax for all composite columns in a multi-variable target (# 3-5 errors, #6 valid) while it changes the behavior if present on a single variable target (#1 vs #2).
So, allowing #3 to work makes implementing #2 even more unappealing. Making only #2 and #6 work seems like a reasonable alternative position.
The last option is to fix #1 to return (1,2) - cleanly reporting an error if possible, must like we just did with SRFs, and apply the patch thus gaining both consistency and a clean syntax at the expense of limited backward incompatibility.
Arrays not considered; single-column composites might end up looking like scalars when presented to a (c) target.
Hope this helps someone besides me understand the problem space.
Based on the discussion in this thread I’ve moved this patch to the next CF
with the new status Waiting for author, as it seems to need a revised version.
cheers ./daniel
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Sep 19, 2017 at 3:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
I think the fact that single-target INTO lists and multiple-target
INTO lists are handled completely differently is extremely poor
language design. It would have been far better, as you suggested
downthread, to have added some syntax up front to let people select
the behavior that they want, but I think there's little hope of
changing this now without creating even more pain.How so? The proposal I gave is fully backwards-compatible. It's
likely not the way we'd do it in a green field, but we don't have
a green field.I have a really hard time, however, imagining that anyone writes
SELECT a, b, c, d, e, f, g, h, i, j, k INTO x, y, z and wants some of
a-k to go into x, some more to go into y, and some more to go into z
(and heaven help you if you drop a column from x or y -- now the whole
semantics of the query change, yikes). What's reasonable is to write
SELECT a, b, c INTO x, y, z and have those correspond 1:1.That's certainly a case that we ought to support somehow. The problem is
staying reasonably consistent with the two-decades-old precedent of the
existing behavior for one target variable.
My point is that you objected to Pavel's proposal saying "it's not
clear whether users want A or B". But I think they always want A.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
On Tue, Sep 19, 2017 at 3:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
That's certainly a case that we ought to support somehow. The problem is
staying reasonably consistent with the two-decades-old precedent of the
existing behavior for one target variable.
My point is that you objected to Pavel's proposal saying "it's not
clear whether users want A or B". But I think they always want A.
I'm not sure if that's true or not. I am sure, though, that since
we've done B for twenty years we can't just summarily change to A.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Oct 2, 2017 at 12:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Tue, Sep 19, 2017 at 3:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
That's certainly a case that we ought to support somehow. The problem is
staying reasonably consistent with the two-decades-old precedent of the
existing behavior for one target variable.My point is that you objected to Pavel's proposal saying "it's not
clear whether users want A or B". But I think they always want A.I'm not sure if that's true or not. I am sure, though, that since
we've done B for twenty years we can't just summarily change to A.
I agree, but so what? You said that we couldn't adopt Pavel's
proposal for this reason:
===
IIRC, the reason for disallowing that is that it's totally unclear what
the semantics ought to be. Is that variable a single target (demanding
a compatible composite-valued column from the source query), or does it
eat one source column per field within the record/row? The former is 100%
inconsistent with what happens if the record/row is the only INTO target;
while the latter would be very bug-prone, and it's especially unclear what
ought to happen if it's an as-yet-undefined record variable.
===
And I'm saying - that argument is bogus. Regardless of what people
want or what we have historically done in the case where the
record/row is the only INTO target, when there are multiple targets it
seems clear that they want to match up the query's output columns with
the INTO targets 1:1. So let's just do that.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
On Mon, Oct 2, 2017 at 12:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I'm not sure if that's true or not. I am sure, though, that since
we've done B for twenty years we can't just summarily change to A.
I agree, but so what? You said that we couldn't adopt Pavel's
proposal for this reason:
===
IIRC, the reason for disallowing that is that it's totally unclear what
the semantics ought to be. Is that variable a single target (demanding
a compatible composite-valued column from the source query), or does it
eat one source column per field within the record/row? The former is 100%
inconsistent with what happens if the record/row is the only INTO target;
while the latter would be very bug-prone, and it's especially unclear what
ought to happen if it's an as-yet-undefined record variable.
===
And I'm saying - that argument is bogus. Regardless of what people
want or what we have historically done in the case where the
record/row is the only INTO target, when there are multiple targets it
seems clear that they want to match up the query's output columns with
the INTO targets 1:1. So let's just do that.
Arguing that that's what people want (even if I granted your argument,
which I do not) does not make the inconsistency magically disappear,
nor will it stop people from being confused by that inconsistency.
Furthermore, if we do it like this, we will be completely backed into
a backwards-compatibility corner if someone does come along and say
"hey, I wish I could do the other thing".
I'm fine with doing something where we add new notation to dispel
the ambiguity. I don't want to put in another layer of inconsistency
and then have even more backwards-compatibility problems constraining
our response to the inevitable complaints.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2017-10-02 18:44 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:
Robert Haas <robertmhaas@gmail.com> writes:
On Mon, Oct 2, 2017 at 12:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I'm not sure if that's true or not. I am sure, though, that since
we've done B for twenty years we can't just summarily change to A.I agree, but so what? You said that we couldn't adopt Pavel's
proposal for this reason:===
IIRC, the reason for disallowing that is that it's totally unclear what
the semantics ought to be. Is that variable a single target (demanding
a compatible composite-valued column from the source query), or does it
eat one source column per field within the record/row? The former is100%
inconsistent with what happens if the record/row is the only INTO target;
while the latter would be very bug-prone, and it's especially unclearwhat
ought to happen if it's an as-yet-undefined record variable.
===And I'm saying - that argument is bogus. Regardless of what people
want or what we have historically done in the case where the
record/row is the only INTO target, when there are multiple targets it
seems clear that they want to match up the query's output columns with
the INTO targets 1:1. So let's just do that.Arguing that that's what people want (even if I granted your argument,
which I do not) does not make the inconsistency magically disappear,
nor will it stop people from being confused by that inconsistency.
Furthermore, if we do it like this, we will be completely backed into
a backwards-compatibility corner if someone does come along and say
"hey, I wish I could do the other thing".I'm fine with doing something where we add new notation to dispel
the ambiguity. I don't want to put in another layer of inconsistency
and then have even more backwards-compatibility problems constraining
our response to the inevitable complaints.
I didn't talk about record type. I talked just only about composite
variables (ROW in our terminology).
I don't think so for this case the special syntax is necessary, although we
can use a parallel assignment with different semantics for this case.
What is a motivation for this thread?
I had to migrate lot of Oracle procedures where was usually two OUT
variables - first - known composite type (some state variable), and second
- result (text or int variable). Now, the CALL of this function in Postgres
is:
SELECT fx() INTO rec;
var_state := rec.state;
var_result := rec.result;
It works, Ora2pg supports it, plpgsql_check is able to check it, but it is
not elegant and less readable.
So, when target is not clean REC or ROW, I am think so we can allow
assignment with few limits
1. The REC type should not be used
2. The target and source fields should be same - this assignment should not
be tolerant like now. Because, this situation is not supported now, there
is not a compatibility risk
Some modern and now well known languages like GO supports parallel
assignment. Can be it the special syntax requested by Tom?
So there are two proposals:
1. Implement safe restrictive SELECT INTO where target can be combination
of REC or scalars
2. Parallel assignment with new behave, that allows any list of REC, ROW or
scalar as target - but composite should be attached to composite var, and
scalar to scalar. List of scalars should be disallowed as target for
composite value should be a) disallowed every time, b) disallowed when some
target var is a composite.
The differences between assign command and INTO command can be messy too.
So the best solution should be one rules for := and INTO - but I am not
sure if it is possible
Comments?
Regards
Pavel
Show quoted text
regards, tom lane
On Mon, Oct 2, 2017 at 12:44 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
And I'm saying - that argument is bogus. Regardless of what people
want or what we have historically done in the case where the
record/row is the only INTO target, when there are multiple targets it
seems clear that they want to match up the query's output columns with
the INTO targets 1:1. So let's just do that.Arguing that that's what people want (even if I granted your argument,
which I do not) does not make the inconsistency magically disappear,
nor will it stop people from being confused by that inconsistency.
Furthermore, if we do it like this, we will be completely backed into
a backwards-compatibility corner if someone does come along and say
"hey, I wish I could do the other thing".
That is not really true. Even if we define INTO a, b, c as I am
proposing (and Pavel, too, I think), I think we can later define INTO
*a, INTO (a), INTO a ..., INTO a MULTIPLE, INTO a STRANGELY, and INTO
%@!a??ppp#zorp to mean anything we like (unless one or more of those
already have some semantics already, in which case pick something that
doesn't).
If we're really on the fence about which behavior people will want, we
could implement both with a syntax marker for each, say SELECT ...
INTO a #rhaas for the behavior I like and SELECT ... INTO a #tgl_ftw
for the other behavior, and require specifying one of those
decorations. Maybe that's more or less what you were proposing. If
we're going to have a default, though, I think it should be the one
you described as "inconsistent with the single row case" rather than
the one you described as "very bug-prone", because I agree with those
characterizations but feel that the latter is a much bigger problem
than the former and, again, not what anybody actually wants.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers