issue: record or row variable cannot be part of multiple-item INTO list

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).

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 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).

here is patch

all regress tests passed

Regards

Pavel

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 tested

I'm afraid this patch conflicts with current master branch.

The new status of this patch is: Waiting on Author

rebased

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 tested

I'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

Hi

I am sending rebased patch

Regards

Pavel

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, 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?

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

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, 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

Thank you very much

Pavel

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/ 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.)

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

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/ 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 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