Обсуждение: [HACKERS] 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

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

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

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

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

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.

"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

"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

"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 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:
> ​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
eachoutput column as a scalar in one-by-one fashion.  If "c" is not the only target column (#3) assign a single output
columnto 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-variabletarget (# 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
reasonablealternative 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

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