Re: [HACKERS] Support UPDATE table SET(*)=...

2015-04-08 Thread Andrew Gierth
 Tom == Tom Lane t...@sss.pgh.pa.us writes:

 Tom I spent a fair amount of time cleaning this patch up to get it
 Tom into committable shape, but as I was working on the documentation
 Tom I started to lose enthusiasm for it, because I was having a hard
 Tom time coming up with compelling examples.  The originally proposed
 Tom motivation was

  It solves the problem of doing UPDATE from a record variable of the same
  type as the table e.g. update foo set (*) = (select foorec.*) where ...;

There are a number of motivating examples for this (which have nothing
to do with rules; I doubt anyone cares much about that).

The fundamental point is that currently, given a table foo and some
column or variable of foo's rowtype, you can do this:

insert into foo select foorec.* [from ...]

but there is no comparable way to do an update without naming every
column explicitly, the closest being

update foo set (a,b,...) = (foorec.a, foorec.b, ...) where ...

One example that comes up occasionally (and that I've had to do myself
more than once) is this: given a table foo and another with identical
schema reference_foo, apply appropriate inserts, updates and deletes
to table foo to make the content of the two tables identical. This can
be done these days with wCTEs:

with
  t_diff as (select o.id as o_id, n.id as n_id, o, n
   from foo o full outer join reference_foo n on (o.id=n.id)
  where (o.*) is distinct from (n.*)),
  ins as (insert into foo select (n).* from t_diff where o_id is null),
  del as (delete from foo
   where id in (select o_id from t_diff where n_id is null)),
  upd as (update foo
 set (col1,col2,...) = ((n).col1,(n).col2,...)  -- XXX
from t_diff
   where foo.id = n_id and o_id = n_id)
select count(*) filter (where o_id is null) as num_ins,
   count(*) filter (where o_id = n_id) as num_upd,
   count(*) filter (where n_id is null) as num_del
  from t_diff;

(This would be preferred over simply replacing the table content if the
table is large and the changes few, you want to audit the changes, you
need to avoid interfering with concurrent selects, etc.)

The update part of that would be much improved by simply being able to
say update all columns of foo with values from (n). The exact syntax
isn't a big deal - though since SET (cols...) = ...  is in the spec, it
seems reasonable to at least keep some kind of consistency with it.

Other examples arise from things one might want to do in plpgsql; for
example to update a record from an hstore or json value, one can use
[json_]populate_record to construct a record variable, but then it's
back to naming all the columns in order to actually perform the update
statement.

[My connection with this patch is only that I suggested it to Atri as a
possible project for him to do, because I wanted the feature and knew
others did also, and helped explain how the existing MultiAssign worked
and some of the criticism. I did not contribute any code.]

-- 
Andrew (irc:RhodiumToad)


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Support UPDATE table SET(*)=...

2015-04-08 Thread Andrew Gierth
 Tom == Tom Lane t...@sss.pgh.pa.us writes:

  Now this I think is wrong; I think it's just as robust against
  schema changes as using the composite value directly would
  be. Consider the case where foo and reference_foo match with the
  exception of dropped columns; the code as it is should just work,
  while a variant that used the composite values would have to
  explicitly deal with that.

 Tom AFAICS you've got that backwards.

 Tom As I illustrated upthread, after parse-time expansion we would
 Tom have a command that explicitly tells the system to insert or
 Tom update only the enumerated columns.  That will *not* work as
 Tom desired if columns are added later,

Where later is between parse analysis and execution - and if this
query is not in a rule, then any such schema change will force a
re-analysis if it's a prepared statement, no? and otherwise, we have the
tables locked against schema changes anyway? Is there a failure case
here that doesn't involve rules?

 Tom and (if it's in a rule)

well, the example I gave is not something that anyone in their right
minds would try and put in a rule.

  ... The alternative of
  set * = populate_record(foo, new_values)
  is less satisfactory since it introduces inconsistencies with the case
  where you _do_ want to specify explicit columns, unless you also allow
  set (a,b) = row_value
  which is required by the spec for T641 but which we don't currently
  have.

 Tom Right, but we should be trying to move in that direction.  I see
 Tom your point though that (*) is more notationally consistent with
 Tom that case.  Maybe we should be looking at trying to implement T641
 Tom in full and then including (*) as a special case of that.

I would have liked to have done that, but that would have raised the
complexity of the project from Atri can take a stab at this one with
negligible supervision to Andrew will have to spend more time than he
has conveniently available staring at the raw parser to try and make it
work.

As I said, the perfect is the enemy of the good.

 Tom Anyway, my core point here is that we should avoid parse-time
 Tom expansion of the column set.

Parse-time expansion of * is pretty widespread elsewhere. Changing that
for this one specific case seems a bit marginal to me - and if the main
motivation to do so is to support cases in DML rules, which are already
a foot-bazooka, I think it's honestly not worth it.

-- 
Andrew (irc:RhodiumToad)


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Support UPDATE table SET(*)=...

2015-04-08 Thread Alvaro Herrera
Andrew Gierth wrote:
  Tom == Tom Lane t...@sss.pgh.pa.us writes:

  Tom Right, but we should be trying to move in that direction.  I see
  Tom your point though that (*) is more notationally consistent with
  Tom that case.  Maybe we should be looking at trying to implement T641
  Tom in full and then including (*) as a special case of that.
 
 I would have liked to have done that, but that would have raised the
 complexity of the project from Atri can take a stab at this one with
 negligible supervision to Andrew will have to spend more time than he
 has conveniently available staring at the raw parser to try and make it
 work.

Not to mention that, at this stage, we should be looking at reducing the
scope of patches in commitfest rather than enlarge it.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Support UPDATE table SET(*)=...

2015-04-08 Thread Tom Lane
Alvaro Herrera alvhe...@2ndquadrant.com writes:
 Andrew Gierth wrote:
 Tom == Tom Lane t...@sss.pgh.pa.us writes:
 Tom Right, but we should be trying to move in that direction.  I see
 Tom your point though that (*) is more notationally consistent with
 Tom that case.  Maybe we should be looking at trying to implement T641
 Tom in full and then including (*) as a special case of that.

 I would have liked to have done that, but that would have raised the
 complexity of the project from Atri can take a stab at this one with
 negligible supervision to Andrew will have to spend more time than he
 has conveniently available staring at the raw parser to try and make it
 work.

Well, we've never considered implementation convenience to be more
important than getting it right, and this doesn't seem like a place
to start.

(FWIW, the raw-parser changes would be a negligible fraction of the work
involved to do it like that.)

 Not to mention that, at this stage, we should be looking at reducing the
 scope of patches in commitfest rather than enlarge it.

I already took it out of the current commitfest ;-).

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


Re: [HACKERS] Support UPDATE table SET(*)=...

2015-04-08 Thread Andrew Gierth
 Tom == Tom Lane t...@sss.pgh.pa.us writes:

  One example that comes up occasionally (and that I've had to do
  myself more than once) is this: given a table foo and another with
  identical schema reference_foo, apply appropriate inserts, updates
  and deletes to table foo to make the content of the two tables
  identical. This can be done these days with wCTEs:

  with
t_diff as (select o.id as o_id, n.id as n_id, o, n
 from foo o full outer join reference_foo n on (o.id=n.id)
where (o.*) is distinct from (n.*)),
ins as (insert into foo select (n).* from t_diff where o_id is null),
del as (delete from foo
 where id in (select o_id from t_diff where n_id is null)),
upd as (update foo
   set (col1,col2,...) = ((n).col1,(n).col2,...)  -- XXX
  from t_diff
 where foo.id = n_id and o_id = n_id)
  select count(*) filter (where o_id is null) as num_ins,
 count(*) filter (where o_id = n_id) as num_upd,
 count(*) filter (where n_id is null) as num_del
 from t_diff;

 Tom While I agree that the UPDATE part of that desperately needs
 Tom improvement, I don't agree that the INSERT part is entirely fine.
 Tom You're still relying on a parse-time expansion of the (n).*
 Tom notation, which is inefficient

Not in my experience a huge deal given what else is going on...

 Tom and not at all robust against schema changes (the same problem as
 Tom with the patch's approach to UPDATE).

Now this I think is wrong; I think it's just as robust against schema
changes as using the composite value directly would be. Consider the
case where foo and reference_foo match with the exception of dropped
columns; the code as it is should just work, while a variant that used
the composite values would have to explicitly deal with that.

(When I've used this kind of operation in practice, reference_foo has
just been created using CREATE TEMP TABLE reference_foo (LIKE foo); and
then populated via COPY from an external data source. Even if
reference_foo were a non-temp table, the logic of the situation requires
it to have the same schema as foo as far as SQL statements are
concerned.)

 Tom So if we're taking this as a motivating example, I'd want to see a
 Tom fix that allows both INSERT and UPDATE directly from a composite
 Tom value of proper rowtype, without any expansion to individual
 Tom columns at all.

I would argue that this is a case of the perfect being the enemy of the
good.

 Tom Perhaps we could adopt some syntax like
 Tom   INSERT INTO table (*) values-or-select
 Tom to represent the case that the values-or-select delivers a single
 Tom composite column of the appropriate type.

We could, but I think in all practical cases it'll be nothing more than
a small performance optimization rather than something that really
benefits people in terms of enhanced functionality.

  Other examples arise from things one might want to do in plpgsql; for
  example to update a record from an hstore or json value, one can use
  [json_]populate_record to construct a record variable, but then it's
  back to naming all the columns in order to actually perform the update
  statement.

 Tom Sure, but the patch as given didn't work very well for that
 Tom either,

Partly that's a limitation resulting from how much can be done with the
existing SET (...) = syntax and implementation without ripping it all
out and starting over. An incremental improvement seemed to be a more
readily achievable goal.

In practice it would indeed probably look like:

  declare
new_id integer;
new_values hstore;
  begin
/* do stuff */
update foo
   set (*) = (select * from populate_record(foo, new_values))
 where foo.id = new_id;

A agree that it would be nicer to do

update foo
   set (*) = populate_record(foo, new_values)
 where foo.id = new_id;

but that would be a substantially larger project. The alternative of

   set * = populate_record(foo, new_values)

is less satisfactory since it introduces inconsistencies with the case
where you _do_ want to specify explicit columns, unless you also allow

   set (a,b) = row_value

which is required by the spec for T641 but which we don't currently
have.

-- 
Andrew (irc:RhodiumToad)


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Support UPDATE table SET(*)=...

2015-04-08 Thread Tom Lane
Andrew Gierth and...@tao11.riddles.org.uk writes:
 Tom == Tom Lane t...@sss.pgh.pa.us writes:
  Tom I spent a fair amount of time cleaning this patch up to get it
  Tom into committable shape, but as I was working on the documentation
  Tom I started to lose enthusiasm for it, because I was having a hard
  Tom time coming up with compelling examples.

 One example that comes up occasionally (and that I've had to do myself
 more than once) is this: given a table foo and another with identical
 schema reference_foo, apply appropriate inserts, updates and deletes
 to table foo to make the content of the two tables identical. This can
 be done these days with wCTEs:

 with
   t_diff as (select o.id as o_id, n.id as n_id, o, n
from foo o full outer join reference_foo n on (o.id=n.id)
   where (o.*) is distinct from (n.*)),
   ins as (insert into foo select (n).* from t_diff where o_id is null),
   del as (delete from foo
where id in (select o_id from t_diff where n_id is null)),
   upd as (update foo
  set (col1,col2,...) = ((n).col1,(n).col2,...)  -- XXX
 from t_diff
where foo.id = n_id and o_id = n_id)
 select count(*) filter (where o_id is null) as num_ins,
count(*) filter (where o_id = n_id) as num_upd,
count(*) filter (where n_id is null) as num_del
   from t_diff;

While I agree that the UPDATE part of that desperately needs improvement,
I don't agree that the INSERT part is entirely fine.  You're still relying
on a parse-time expansion of the (n).* notation, which is inefficient and
not at all robust against schema changes (the same problem as with the
patch's approach to UPDATE).  So if we're taking this as a motivating
example, I'd want to see a fix that allows both INSERT and UPDATE directly
from a composite value of proper rowtype, without any expansion to
individual columns at all.

Perhaps we could adopt some syntax like
INSERT INTO table (*) values-or-select
to represent the case that the values-or-select delivers a single
composite column of the appropriate type.

 Other examples arise from things one might want to do in plpgsql; for
 example to update a record from an hstore or json value, one can use
 [json_]populate_record to construct a record variable, but then it's
 back to naming all the columns in order to actually perform the update
 statement.

Sure, but the patch as given didn't work very well for that either,
at least not if you wanted to avoid multiple evaluation of the
composite-returning function.  You'd have to adopt some obscure syntax
like UPDATE target SET (*) = (SELECT * FROM composite_function(...)).
With what I'm thinking about now you could do
UPDATE target SET * = composite_function(...)
which is a good deal less notation, and with a bit of luck it would not
require disassembling and reassembling the function's output tuple.

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


Re: [HACKERS] Support UPDATE table SET(*)=...

2015-04-08 Thread Tom Lane
Andrew Gierth and...@tao11.riddles.org.uk writes:
 Tom == Tom Lane t...@sss.pgh.pa.us writes:
  Tom and not at all robust against schema changes (the same problem as
  Tom with the patch's approach to UPDATE).

 Now this I think is wrong; I think it's just as robust against schema
 changes as using the composite value directly would be. Consider the
 case where foo and reference_foo match with the exception of dropped
 columns; the code as it is should just work, while a variant that used
 the composite values would have to explicitly deal with that.

AFAICS you've got that backwards.

As I illustrated upthread, after parse-time expansion we would have a
command that explicitly tells the system to insert or update only the
enumerated columns.  That will *not* work as desired if columns are added
later, and (if it's in a rule) I would expect the system to have to fail
a DROP command trying to drop one of those named columns, the same way
you can't drop a column referenced in a view/rule today.

On the other hand, if it's a composite-type assignment, then dealing
with things like dropped columns becomes the system's responsibility,
and we already have code for that sort of thing.

 ... The alternative of
set * = populate_record(foo, new_values)
 is less satisfactory since it introduces inconsistencies with the case
 where you _do_ want to specify explicit columns, unless you also allow
set (a,b) = row_value
 which is required by the spec for T641 but which we don't currently
 have.

Right, but we should be trying to move in that direction.  I see your
point though that (*) is more notationally consistent with that case.
Maybe we should be looking at trying to implement T641 in full and then
including (*) as a special case of that.

Anyway, my core point here is that we should avoid parse-time expansion
of the column set.  The *only* reason for doing that is that it makes the
patch simpler, not that there is some functional advantage; in fact there
are considerable functional disadvantages as we've just been through.
So I do not want to commit a patch that institutionalizes those
disadvantages just for short-term implementation simplicity.

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


Re: [HACKERS] Support UPDATE table SET(*)=...

2015-04-07 Thread Tom Lane
Atri Sharma atri.j...@gmail.com writes:
 Please find attached latest version of UPDATE (*) patch.The patch
 implements review comments and Tom's gripe about touching
 transformTargetList is addressed now. I have added regression tests and
 simplified parser representation a bit.

I spent a fair amount of time cleaning this patch up to get it into
committable shape, but as I was working on the documentation I started
to lose enthusiasm for it, because I was having a hard time coming up
with compelling examples.  The originally proposed motivation was

 It solves the problem of doing UPDATE from a record variable of the same
 type as the table e.g. update foo set (*) = (select foorec.*) where ...;

but it feels to me that this is not actually a very good solution to that
problem --- even granting that the problem needs to be solved, a claim
that still requires some justification IMO.  Here is a possible use-case:

regression=# create table src (f1 int, f2 text, f3 real);
CREATE TABLE
regression=# create table dst (f1 int, f2 text, f3 real);
CREATE TABLE
regression=# create rule r1 as on update to src do instead
regression-# update dst set (*) = (new.*) where dst.f1 = old.f1;
ERROR:  number of columns does not match number of values
LINE 2: update dst set (*) = (new.*) where dst.f1 = old.f1;
  ^

So that's annoying.  You can work around it with this very unobvious
(and undocumented) syntax hack:

regression=# create rule r1 as on update to src do instead
regression-# update dst set (*) = (select new.*) where dst.f1 = old.f1;
CREATE RULE

But what happens if dst has no matching row?  Your data goes into the
bit bucket, that's what.  What you really wanted here was some kind of
UPSERT.  There's certainly a possible use-case for a notation like this
in UPSERT, when we get around to implementing that; but I'm less than
convinced that we need it in plain UPDATE.

What's much worse though is that the rule that actually gets stored is:

regression=# \d+ src
  Table public.src
 Column |  Type   | Modifiers | Storage  | Stats target | Description 
+-+---+--+--+-
 f1 | integer |   | plain|  | 
 f2 | text|   | extended |  | 
 f3 | real|   | plain|  | 
Rules:
r1 AS
ON UPDATE TO src DO INSTEAD  UPDATE dst SET (f1, f2, f3) = ( SELECT new.f1,
new.f2,
new.f3)
  WHERE dst.f1 = old.f1

That is, you don't actually have any protection at all against future
additions of columns, which seems like it would be most of the point
of using a notation like this.

We could possibly address that by postponing expansion of the *-notation
to rule rewrite time, but that seems like it'd be a complete disaster in
terms of modularity, or even usability (since column-mismatch errors
wouldn't get raised till then either).  And it'd certainly be a far more
invasive and complex patch than this.

So I'm feeling that this may not be a good idea, or at least not a good
implementation of the idea.  I'm inclined to reject the patch rather than
lock us into something that is not standard and doesn't really do what
people would be likely to want.

Attached is the updated patch that I had before arriving at this
discouraging conclusion.

regards, tom lane

diff --git a/doc/src/sgml/ref/update.sgml b/doc/src/sgml/ref/update.sgml
index 35b0699..adb4473 100644
*** a/doc/src/sgml/ref/update.sgml
--- b/doc/src/sgml/ref/update.sgml
*** PostgreSQL documentation
*** 25,31 
  UPDATE [ ONLY ] replaceable class=PARAMETERtable_name/replaceable [ * ] [ [ AS ] replaceable class=parameteralias/replaceable ]
  SET { replaceable class=PARAMETERcolumn_name/replaceable = { replaceable class=PARAMETERexpression/replaceable | DEFAULT } |
( replaceable class=PARAMETERcolumn_name/replaceable [, ...] ) = ( { replaceable class=PARAMETERexpression/replaceable | DEFAULT } [, ...] ) |
!   ( replaceable class=PARAMETERcolumn_name/replaceable [, ...] ) = ( replaceable class=PARAMETERsub-SELECT/replaceable )
  } [, ...]
  [ FROM replaceable class=PARAMETERfrom_list/replaceable ]
  [ WHERE replaceable class=PARAMETERcondition/replaceable | WHERE CURRENT OF replaceable class=PARAMETERcursor_name/replaceable ]
--- 25,33 
  UPDATE [ ONLY ] replaceable class=PARAMETERtable_name/replaceable [ * ] [ [ AS ] replaceable class=parameteralias/replaceable ]
  SET { replaceable class=PARAMETERcolumn_name/replaceable = { replaceable class=PARAMETERexpression/replaceable | DEFAULT } |
( replaceable class=PARAMETERcolumn_name/replaceable [, ...] ) = ( { replaceable class=PARAMETERexpression/replaceable | DEFAULT } [, ...] ) |
!   ( replaceable class=PARAMETERcolumn_name/replaceable [, ...] ) = ( replaceable class=PARAMETERsub-SELECT/replaceable ) |
!   (*) = ( { 

Re: [HACKERS] Support UPDATE table SET(*)=...

2015-04-07 Thread Tom Lane
I wrote:
 So I'm feeling that this may not be a good idea, or at least not a good
 implementation of the idea.  I'm inclined to reject the patch rather than
 lock us into something that is not standard and doesn't really do what
 people would be likely to want.

BTW, a potentially workable fix to the problem of not wanting to lock
down column lists in stored rules is to create a syntax that represents
whole-row, record-oriented assignment directly.  Then we need not be
concerned with individual columns at parse time at all.  So imagine
something like this:

UPDATE dst SET * = new WHERE ...;

UPDATE dst SET * = (SELECT src FROM src WHERE ...);

or if you needed to construct a row value at runtime you could write

UPDATE dst SET * = ROW(x,y,z) WHERE ...;

UPDATE dst SET * = (SELECT ROW(x,y,z) FROM src WHERE ...);

The main bit of functionality that would be lost compared to the current
patch is the ability to use DEFAULT for some of the row members.  But I am
not sure there is a compelling use-case for that: seems like if you have
some DEFAULTs in there then it's unlikely that you don't know the column
list accurately, so the existing (col1,col2,...) syntax will serve fine.

This seems like it might not be unduly complex to implement, although
it would have roughly nothing in common with the current patch.

If we were to go in this direction, it would be nice to at the same time
add a similar whole-record syntax for INSERT.  I'm not sure exactly what
that should look like though.  Also, again, we ought to be paying
attention to how this would match up with UPSERT syntax.

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


Re: [HACKERS] Support UPDATE table SET(*)=...

2015-04-07 Thread Peter Geoghegan
On Tue, Apr 7, 2015 at 12:01 PM, Peter Geoghegan p...@heroku.com wrote:
 I still don't like the idea of
 supporting this, though. I'm not aware of any other system allowing
 something like this for either MERGE or a non-standard UPSERT.

That includes MySQL, BTW. Only their REPLACE statement (which is a
disaster for various reasons) can do something like this. Their INSERT
... ON DUPLICATE KEY UPDATE statement (which is roughly comparable to
the proposed UPSERT patch) cannot update the entire row using a terse
expression that references a row excluded from insertion (following
the implementation taking the UPDATE path).

-- 
Peter Geoghegan


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Support UPDATE table SET(*)=...

2015-04-07 Thread Peter Geoghegan
On Tue, Apr 7, 2015 at 11:19 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 If we were to go in this direction, it would be nice to at the same time
 add a similar whole-record syntax for INSERT.  I'm not sure exactly what
 that should look like though.  Also, again, we ought to be paying
 attention to how this would match up with UPSERT syntax.

I expressed concern about allowing this for UPSERT [1].

To be fair, VoltDB's new UPSERT statement allows something similar (or
rather mandates it, since you cannot just update some columns), and
that doesn't look wholly unreasonable. I still don't like the idea of
supporting this, though. I'm not aware of any other system allowing
something like this for either MERGE or a non-standard UPSERT.

[1] 
http://www.postgresql.org/message-id/CAM3SWZT=vxbj7qkaidamybu40ap10udsqooqhvix3ykj7wb...@mail.gmail.com
-- 
Peter Geoghegan


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Support UPDATE table SET(*)=...

2015-04-07 Thread Alvaro Herrera
Tom Lane wrote:

 I spent a fair amount of time cleaning this patch up to get it into
 committable shape, but as I was working on the documentation I started
 to lose enthusiasm for it, because I was having a hard time coming up
 with compelling examples.  The originally proposed motivation was
 
  It solves the problem of doing UPDATE from a record variable of the same
  type as the table e.g. update foo set (*) = (select foorec.*) where ...;
 
 but it feels to me that this is not actually a very good solution to that
 problem --- even granting that the problem needs to be solved, a claim
 that still requires some justification IMO.

How about an UPDATE ran inside a plpgsql function, which is using a row
variable of the table type?  You could assign values to individual
columns of q row variable, and run the multicolumn UPDATE last.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Support UPDATE table SET(*)=...

2015-04-07 Thread Tom Lane
Peter Geoghegan p...@heroku.com writes:
 On Tue, Apr 7, 2015 at 11:19 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 If we were to go in this direction, it would be nice to at the same time
 add a similar whole-record syntax for INSERT.  I'm not sure exactly what
 that should look like though.  Also, again, we ought to be paying
 attention to how this would match up with UPSERT syntax.

 I expressed concern about allowing this for UPSERT [1].

Yeah, your analogy to SELECT * being considered dangerous in production
is not without merit.  However, to the extent that the syntax is used to
assign from a composite variable of the same (or compatible) data type,
it seems like it would be safe enough.  IOW, I think that analogy holds
for the syntax implemented by the current patch, but not what I suggested
in my followup.

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


Re: [HACKERS] Support UPDATE table SET(*)=...

2015-04-07 Thread Jim Nasby

On 4/7/15 2:00 PM, Alvaro Herrera wrote:

Tom Lane wrote:


I spent a fair amount of time cleaning this patch up to get it into
committable shape, but as I was working on the documentation I started
to lose enthusiasm for it, because I was having a hard time coming up
with compelling examples.  The originally proposed motivation was


It solves the problem of doing UPDATE from a record variable of the same
type as the table e.g. update foo set (*) = (select foorec.*) where ...;


but it feels to me that this is not actually a very good solution to that
problem --- even granting that the problem needs to be solved, a claim
that still requires some justification IMO.


How about an UPDATE ran inside a plpgsql function, which is using a row
variable of the table type?  You could assign values to individual
columns of q row variable, and run the multicolumn UPDATE last.


Along similar lines, I've often wanted something akin to *, but allowing 
finer control over what you got. Generally when I want this it's because 
I really do want everything (as in, don't want to re-code a bunch of 
stuff if a column is added), but perhaps not the surrogate key field. Or 
I want everything, but rename some field to something else.


Basically, another way to do what Alvaro is suggesting (though, the 
ability to rename something is new...)


If we had that ability I think UPDATE * would become a lot more useful.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Support UPDATE table SET(*)=...

2015-02-13 Thread Atri Sharma
Hi all,

Sorry for the delay.

Please find attached latest version of UPDATE (*) patch.The patch
implements review comments and Tom's gripe about touching
transformTargetList is addressed now. I have added regression tests and
simplified parser representation a bit.

Regards,

Atri
diff --git a/doc/src/sgml/ref/update.sgml b/doc/src/sgml/ref/update.sgml
index 35b0699..1f68bdf 100644
--- a/doc/src/sgml/ref/update.sgml
+++ b/doc/src/sgml/ref/update.sgml
@@ -25,7 +25,9 @@ PostgreSQL documentation
 UPDATE [ ONLY ] replaceable class=PARAMETERtable_name/replaceable [ * ] [ [ AS ] replaceable class=parameteralias/replaceable ]
 SET { replaceable class=PARAMETERcolumn_name/replaceable = { replaceable class=PARAMETERexpression/replaceable | DEFAULT } |
   ( replaceable class=PARAMETERcolumn_name/replaceable [, ...] ) = ( { replaceable class=PARAMETERexpression/replaceable | DEFAULT } [, ...] ) |
-  ( replaceable class=PARAMETERcolumn_name/replaceable [, ...] ) = ( replaceable class=PARAMETERsub-SELECT/replaceable )
+  ( replaceable class=PARAMETERcolumn_name/replaceable [, ...] ) = ( replaceable class=PARAMETERsub-SELECT/replaceable ) |
+  ( replaceable class=PARAMETER*/replaceable [, ...] ) = ( { replaceable class=PARAMETERexpression/replaceable | DEFAULT } [, ...] ) |
+  ( replaceable class=PARAMETER*/replaceable [, ...] ) = ( replaceable class=PARAMETERsub-SELECT/replaceable )
 } [, ...]
 [ FROM replaceable class=PARAMETERfrom_list/replaceable ]
 [ WHERE replaceable class=PARAMETERcondition/replaceable | WHERE CURRENT OF replaceable class=PARAMETERcursor_name/replaceable ]
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index a68f2e8..6d08dbd 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -1937,6 +1937,101 @@ transformUpdateStmt(ParseState *pstate, UpdateStmt *stmt)
 	nsitem-p_lateral_only = false;
 	nsitem-p_lateral_ok = true;
 
+	/*
+	 * Check if (SET(*) = SELECT ...)  is present. If it is present we
+	 * need to resolve and populate the remaining needed MultiAssignRefs in the
+	 * target list. We modify target list in place and add needed MultiAssignRefs.
+	 */
+	if (list_length(stmt-targetList) == 1)
+	{
+		ResTarget *current_val = linitial(stmt-targetList);
+
+		/* Currently there is no way that ResTarget node's name value in UPDATE command
+		 * is set to NULL except for UPDATE SET (*) case.
+		 * Hence we can safely depend on name value being NULL as a check for
+		 * presence of UPDATE SET (*) case.
+		 */
+		if (current_val-name == NULL)
+		{
+			List *rel_cols_list;
+			List *expanded_tlist = NULL;
+			List *sub_list = NULL;
+			ListCell *lc_val;
+			ListCell *lc_relcol;
+			int rteindex = 0;
+			int sublevels_up = 0;
+			int i = 0;
+
+			rteindex = RTERangeTablePosn(pstate, pstate-p_target_rangetblentry,
+		 sublevels_up);
+
+			expandRTE(pstate-p_target_rangetblentry, rteindex, sublevels_up,
+	  current_val-location, false,
+	  (rel_cols_list), NULL);
+
+
+			/* SET(*) = (SELECT ...) case */
+			if (IsA(current_val-val, MultiAssignRef))
+			{
+MultiAssignRef *orig_val = (MultiAssignRef *) (current_val-val);
+
+orig_val-ncolumns = list_length(rel_cols_list);
+
+Assert(sub_list == NULL);
+
+sub_list = list_make1(orig_val);
+
+/* Change targetlist to have corresponding ResTarget nodes
+ * as corresponding to the columns in target relation */
+for (i = 1;i  list_length(rel_cols_list);i++)
+{
+	MultiAssignRef *r = makeNode(MultiAssignRef);
+
+	r-source = orig_val-source;
+	r-colno = i + 1;
+	r-ncolumns = orig_val-ncolumns;
+
+	lappend(sub_list, r);
+}
+			}
+			else if (IsA(current_val-val, List))  /* SET(*) = (val, val,...) case */
+			{
+
+Assert(sub_list == NULL);
+sub_list = (List *) (current_val-val);
+			}
+			else
+			{
+elog(ERROR, Unknown type in UPDATE command);
+			}
+
+			if (list_length(rel_cols_list) != list_length(sub_list))
+elog(ERROR, number of columns does not match number of values);
+
+			/* Change targetlist to have corresponding ResTarget nodes
+			 * as corresponding to the columns in target relation */
+			forboth(lc_val, sub_list, lc_relcol, rel_cols_list)
+			{
+ResTarget *current_res = makeNode(ResTarget);
+
+current_res-name = strVal(lfirst(lc_relcol));
+current_res-val = (Node *) (lfirst(lc_val));
+
+if (expanded_tlist == NULL)
+{
+	expanded_tlist = list_make1(current_res);
+}
+else
+{
+	lappend(expanded_tlist, current_res);
+}
+			}
+
+			stmt-targetList = expanded_tlist;
+		}
+	}
+
+
 	qry-targetList = transformTargetList(pstate, stmt-targetList,
 		  EXPR_KIND_UPDATE_SOURCE);
 
@@ -1982,18 +2077,20 @@ transformUpdateStmt(ParseState *pstate, UpdateStmt *stmt)
 			continue;
 		}
 		if (origTargetList == NULL)
-			elog(ERROR, UPDATE target count mismatch --- internal error);
+elog(ERROR, UPDATE target count mismatch 

Re: [HACKERS] Support UPDATE table SET(*)=...

2015-01-14 Thread Michael Paquier
On Mon, Dec 15, 2014 at 7:50 PM, Atri Sharma atri.j...@gmail.com wrote:
 I have moved patch to current CF and marked it as Waiting on Author since I
 plan to resubmit after addressing the concerns.
Nothing happened in the last month, so marking as returned with feedback.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Support UPDATE table SET(*)=...

2014-12-15 Thread Atri Sharma
On Mon, Dec 8, 2014 at 6:06 AM, Michael Paquier michael.paqu...@gmail.com
wrote:

 On Wed, Nov 26, 2014 at 4:26 AM, Tom Lane t...@sss.pgh.pa.us wrote:
  I think what's likely missing here is a clear design for the raw parse
  tree representation (what's returned by the bison grammar).  The patch
  seems to be trying to skate by without creating any new parse node types
  or fields, but that may well be a bad idea.  At the very least there
  needs to be some commentary added to parsenodes.h explaining what the
  representation actually is (cf commentary there for MultiAssignRef).
 
  Also, I think it's a mistake not to be following the MultiAssignRef
  model for the case of (*) = ctext_row.  We optimize the ROW-source
  case at the grammar stage when there's a fixed number of target columns,
  because that's a very easy transformation --- but you can't do it like
  that when there's not.  It's possible that that optimization should be
  delayed till later even in the existing case; in general, optimizing
  in gram.y is a bad habit that's better avoided ...
 Marking as returned with feedback based on those comments.


Thank you everybody for your comments.

I am indeed trying to avoid a new node since my intuitive feeling says that
we do not need a new node for a functionality which is present in other
commands albeit in different form. However, I agree that documentation
explaining parse representation is lacking and shall improve that. Also, in
accordance to Tom's comments, I shall look for not touching target list
directly and follow the path of MultiAssignRef.

I have moved patch to current CF and marked it as Waiting on Author since I
plan to resubmit after addressing the concerns.

Regards,

Atri


-- 
Regards,

Atri
*l'apprenant*


Re: [HACKERS] Support UPDATE table SET(*)=...

2014-12-07 Thread Michael Paquier
On Wed, Nov 26, 2014 at 4:26 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 I think what's likely missing here is a clear design for the raw parse
 tree representation (what's returned by the bison grammar).  The patch
 seems to be trying to skate by without creating any new parse node types
 or fields, but that may well be a bad idea.  At the very least there
 needs to be some commentary added to parsenodes.h explaining what the
 representation actually is (cf commentary there for MultiAssignRef).

 Also, I think it's a mistake not to be following the MultiAssignRef
 model for the case of (*) = ctext_row.  We optimize the ROW-source
 case at the grammar stage when there's a fixed number of target columns,
 because that's a very easy transformation --- but you can't do it like
 that when there's not.  It's possible that that optimization should be
 delayed till later even in the existing case; in general, optimizing
 in gram.y is a bad habit that's better avoided ...
Marking as returned with feedback based on those comments.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Support UPDATE table SET(*)=...

2014-11-25 Thread Tom Lane
Marko Tiikkaja ma...@joh.to writes:
 On 10/15/14, 10:02 AM, Atri Sharma wrote:
 Please find attached a patch which implements support for UPDATE table1
 SET(*)=...

 I've had a few looks at this patch and I have a few comments:

1) This doesn't work for the zero-column table case at all:
 CREATE TABLE foo();
 UPDATE foo SET (*) = (SELECT);
 ERROR:  number of columns does not match number of values
2) What's the purpose of the second condition here?
 if (!(origTarget) || !(origTarget-name))
3) The extra parentheses around everything make this code for some 
 reason very hard to read.
4) transformTargetList() is a mess right now.  If this is the 
 approach we want to take, the common code should probably be refactored 
 into a function.  But the usage of List as a somehow magical way to 
 represent the SET (*) case makes me feel weird inside.
5) The complete lack of regression tests make it hard to poke around 
 the code to try and figure out what each line/condition is trying to do.

 I feel like I understand what this code is doing and some details feel a 
 bit icky, but I'm not the right person to comment on whether the broad 
 strokes are on the right canvas or not.  Maybe someone else wants to 
 take a closer look before Atri spends too much time on this approach? 

FWIW, I opined upthread that transformTargetList was not the place to
be handling this; it should be done in the same place(s) that support
the existing MultiAssignRef feature, and transformTargetList is not
that.

I think what's likely missing here is a clear design for the raw parse
tree representation (what's returned by the bison grammar).  The patch
seems to be trying to skate by without creating any new parse node types
or fields, but that may well be a bad idea.  At the very least there
needs to be some commentary added to parsenodes.h explaining what the
representation actually is (cf commentary there for MultiAssignRef).

Also, I think it's a mistake not to be following the MultiAssignRef
model for the case of (*) = ctext_row.  We optimize the ROW-source
case at the grammar stage when there's a fixed number of target columns,
because that's a very easy transformation --- but you can't do it like
that when there's not.  It's possible that that optimization should be
delayed till later even in the existing case; in general, optimizing
in gram.y is a bad habit that's better avoided ...

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


Re: [HACKERS] Support UPDATE table SET(*)=...

2014-10-29 Thread Marko Tiikkaja

Hi Atri,

Sorry for the delay.  With pgconf.eu and all, it's been very hard to 
find the time to look at this.


On 10/15/14, 10:02 AM, Atri Sharma wrote:

Please find attached a patch which implements support for UPDATE table1
SET(*)=...
The patch supports both UPDATE table SET(*)=(a,b,c) and UPDATE table1
SET(*)=(SELECT a,b,c FROM...).
It solves the problem of doing UPDATE from a record variable of the same
type as the table e.g. update foo set (*) = (select foorec.*) where ...;


Excellent!  This is a very welcome change.

I've had a few looks at this patch and I have a few comments:

  1) This doesn't work for the zero-column table case at all:
   CREATE TABLE foo();
   UPDATE foo SET (*) = (SELECT);
   ERROR:  number of columns does not match number of values
  2) What's the purpose of the second condition here?
   if (!(origTarget) || !(origTarget-name))
  3) The extra parentheses around everything make this code for some 
reason very hard to read.
  4) transformTargetList() is a mess right now.  If this is the 
approach we want to take, the common code should probably be refactored 
into a function.  But the usage of List as a somehow magical way to 
represent the SET (*) case makes me feel weird inside.
  5) The complete lack of regression tests make it hard to poke around 
the code to try and figure out what each line/condition is trying to do.


I feel like I understand what this code is doing and some details feel a 
bit icky, but I'm not the right person to comment on whether the broad 
strokes are on the right canvas or not.  Maybe someone else wants to 
take a closer look before Atri spends too much time on this approach? 
After all, this patch has a very distinctive WIP feel to it, so I guess 
feedback on the general approach is what's being sought after here, and 
in that area I consider my skills and knowledge lacking.



The design is simple. It basically expands the * in transformation stage,
does the necessary type checking and adds it to the parse tree. This allows
for normal execution for the rest of the stages.


I can't poke any big holes into this approach (disregarding the details 
of this implementation), but perhaps someone else can?




.marko


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Support UPDATE table SET(*)=...

2014-10-22 Thread Merlin Moncure
On Fri, Oct 17, 2014 at 10:47 AM, Merlin Moncure mmonc...@gmail.com wrote:
 On Fri, Oct 17, 2014 at 10:30 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Merlin Moncure mmonc...@gmail.com writes:
 On Fri, Oct 17, 2014 at 10:16 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 I think it wouldn't; Merlin is proposing that f would be taken as the
 field name.  A more realistic objection goes like this:

 create table foo(f int, g int);
 update foo x set x = (1,2);  -- works
 alter table foo add column x int;
 update foo x set x = (1,2,3);  -- no longer works

 It's not a real good thing if a column addition or renaming can
 so fundamentally change the nature of a query.

 That's exactly how SELECT works.  I also dispute that the user should
 be surprised in such cases.

 Well, the reason we have a problem in SELECT is that we support an
 unholy miscegenation of SQL-spec and PostQUEL syntax: the idea that
 SELECT foo FROM foo could represent a whole-row selection is nowhere
 to be found in the SQL standard, for good reason IMO.  But we've never
 had the courage to break cleanly with this Berkeley leftover and
 insist that people spell it SQL-style as SELECT ROW(foo.*) FROM foo.
 I'd just as soon not introduce the same kind of ambiguity into UPDATE
 if we have a reasonable alternative.

 Ah, interesting point (I happen to like the terse syntax and use it
 often).  This is for posterity anyways since you guys seem to like
 Atri's proposal, which surprised me.  However, I think you're over
 simplifying things here.  Syntax aside: I think
 SELECT f FROM foo f;
 and a hypothetical
 SELECT row(f.*) FROM foo f;

 give different semantics.  The former gives an object of type 'f' and
 the latter gives type 'row'.  To get parity, you'd have to add an
 extra cast which means you'd have to play tricky games to avoid extra
 performance overhead besides being significantly more verbose.  I'm
 aware some of the other QUELisms are pretty dodgy and have burned us
 in the past (like that whole function as record reference thing) but
 pulling a record as a field in select is pretty nice.  It's also
 widely used and quite useful in json serialization.

Been thinking about this in the back of my mind the last couple of
days.  There are some things you can do with the QUEL 'table as
column' in select syntax that are impossible otherwise, at least
today, and its usage is going to proliferate because of that.  Row
construction via () or row() needs to be avoided whenever the column
names are important and there is no handy type to cast to. For
example, especially during json serialization it's convenient to do
things like:

select
  array_agg((select q from (select a, b) q) order by ...)
from foo;

...where a,b are fields of foo.  FWICT, this is the only way to do
json serialization of arbitrary runtime row constructions in a way
that does not anonymize the type.  Until I figured out this trick I
used to create lots of composite types that served no purpose other
than to give me a type to cast to which is understandably annoying.

if:
select (x).* from (select (1, 2) as x) q;

worked and properly expanded x to given names should they exist and:
SELECT row(f.*) FROM foo f;

worked and did same, and:
SELECT (row(f.*)).* FROM foo f;

was as reasonably performant and gave the same results as:
SELECT (f).* FROM foo f;

...then IMSNHO you'd have a reasonable path to deprecating the QUEL
inspired syntax.  Food for thought.

merlin


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Support UPDATE table SET(*)=...

2014-10-17 Thread Merlin Moncure
On Wed, Oct 15, 2014 at 3:48 AM, Atri Sharma atri.j...@gmail.com wrote:


 On Wednesday, October 15, 2014, Marti Raudsepp ma...@juffo.org wrote:

 Hi

 On Wed, Oct 15, 2014 at 11:02 AM, Atri Sharma atri.j...@gmail.com wrote:
  Please find attached a patch which implements support for UPDATE table1
  SET(*)=...

 I presume you haven't read Tom Lane's proposal and discussion about
 multiple column assignment in UPDATE:
 http://www.postgresql.org/message-id/1783.1399054...@sss.pgh.pa.us
 (Assigning all columns was also discussed there)

 And there's a WIP patch:
 http://www.postgresql.org/message-id/20930.1402931...@sss.pgh.pa.us

 Thanks for the links, but this patch only targets SET(*) case, which, if I
 understand correctly, the patch you mentioned doesn't directly handle (If I
 understand correctly, the target of the two patches is different).

Yeah -- in fact, there was some discussion about this exact case.
This patch solves a very important problem: when doing record
operations to move data between databases with identical schema
there's currently no way to 'update' in a generic way without building
out the entire field list via complicated and nasty dynamic SQL.  I'm
not sure about the proposed syntax though; it seems a little weird to
me.  Any particular reason why you couldn't have just done:

UPDATE table1 SET * = a,b,c, ...

also,

UPDATE table1 t SET t = (SELECT (a,b,c)::t FROM...);

seems cleaner than the proposed syntax for row assignment.  Tom
objected though IIRC.

merlin


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Support UPDATE table SET(*)=...

2014-10-17 Thread Atri Sharma
On Fri, Oct 17, 2014 at 7:45 PM, Merlin Moncure mmonc...@gmail.com wrote:

 On Wed, Oct 15, 2014 at 3:48 AM, Atri Sharma atri.j...@gmail.com wrote:
 
 
  On Wednesday, October 15, 2014, Marti Raudsepp ma...@juffo.org wrote:
 
  Hi
 
  On Wed, Oct 15, 2014 at 11:02 AM, Atri Sharma atri.j...@gmail.com
 wrote:
   Please find attached a patch which implements support for UPDATE
 table1
   SET(*)=...
 
  I presume you haven't read Tom Lane's proposal and discussion about
  multiple column assignment in UPDATE:
  http://www.postgresql.org/message-id/1783.1399054...@sss.pgh.pa.us
  (Assigning all columns was also discussed there)
 
  And there's a WIP patch:
  http://www.postgresql.org/message-id/20930.1402931...@sss.pgh.pa.us
 
  Thanks for the links, but this patch only targets SET(*) case, which, if
 I
  understand correctly, the patch you mentioned doesn't directly handle
 (If I
  understand correctly, the target of the two patches is different).

 Yeah -- in fact, there was some discussion about this exact case.
 This patch solves a very important problem: when doing record
 operations to move data between databases with identical schema
 there's currently no way to 'update' in a generic way without building
 out the entire field list via complicated and nasty dynamic SQL.


Thanks!


 I'm
 not sure about the proposed syntax though; it seems a little weird to
 me.  Any particular reason why you couldn't have just done:

 UPDATE table1 SET * = a,b,c, ...

 also,

 UPDATE table1 t SET t = (SELECT (a,b,c)::t FROM...);


I honestly have not spent a lot of time thinking about the exact syntax
that may be acceptable. If we have arguments for or against a specific
syntax, I will be glad to incorporate them.





Re: [HACKERS] Support UPDATE table SET(*)=...

2014-10-17 Thread Marko Tiikkaja

On 10/17/14 4:15 PM, Merlin Moncure wrote:

Any particular reason why you couldn't have just done:

UPDATE table1 SET * = a,b,c, ...


That just looks wrong to me.  I'd prefer  (*) = ..  over that any day.


UPDATE table1 t SET t = (SELECT (a,b,c)::t FROM...);

seems cleaner than the proposed syntax for row assignment.  Tom
objected though IIRC.


I don't know about Tom, but I didn't like that: 
http://www.postgresql.org/message-id/5364c982.7060...@joh.to



.marko


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Support UPDATE table SET(*)=...

2014-10-17 Thread Merlin Moncure
On Fri, Oct 17, 2014 at 9:55 AM, Marko Tiikkaja ma...@joh.to wrote:
 On 10/17/14 4:15 PM, Merlin Moncure wrote:

 Any particular reason why you couldn't have just done:

 UPDATE table1 SET * = a,b,c, ...


 That just looks wrong to me.  I'd prefer  (*) = ..  over that any day.

 UPDATE table1 t SET t = (SELECT (a,b,c)::t FROM...);

 seems cleaner than the proposed syntax for row assignment.  Tom
 objected though IIRC.

 I don't know about Tom, but I didn't like that:
 http://www.postgresql.org/message-id/5364c982.7060...@joh.to

Hm, I didn't understand your objection:

quoting
So e.g.:
   UPDATE foo f SET f = ..;

would resolve to the table, despite there being a column called f?
That would break backwards compatibility.
/quoting

That's not correct: it should work exactly as 'select' does; given a
conflict resolve the field name, so no backwards compatibility issue.

merlin


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Support UPDATE table SET(*)=...

2014-10-17 Thread Marko Tiikkaja

On 10/17/14 5:03 PM, Merlin Moncure wrote:

Hm, I didn't understand your objection:

quoting
So e.g.:
UPDATE foo f SET f = ..;

would resolve to the table, despite there being a column called f?
That would break backwards compatibility.
/quoting

That's not correct: it should work exactly as 'select' does; given a
conflict resolve the field name, so no backwards compatibility issue.


local:marko=# show server_version;
 server_version

 9.1.13
(1 row)

local:marko=#* create table foo(f int);
CREATE TABLE
local:marko=#* update foo f set f=1;
UPDATE 0

This query would change meaning with your suggestion.

I'm not saying it would be a massive problem in practice, but I think we 
should first consider options which don't break backwards compatibility, 
even if some consider them less clean.



.marko


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Support UPDATE table SET(*)=...

2014-10-17 Thread Tom Lane
Merlin Moncure mmonc...@gmail.com writes:
 On Fri, Oct 17, 2014 at 9:55 AM, Marko Tiikkaja ma...@joh.to wrote:
 I don't know about Tom, but I didn't like that:
 http://www.postgresql.org/message-id/5364c982.7060...@joh.to

 Hm, I didn't understand your objection:

 quoting
 So e.g.:
UPDATE foo f SET f = ..;

 would resolve to the table, despite there being a column called f?
 That would break backwards compatibility.
 /quoting

 That's not correct: it should work exactly as 'select' does; given a
 conflict resolve the field name, so no backwards compatibility issue.

The point is that it's fairly messy (and bug-prone) to have a syntax
where we have to make an arbitrary choice between two reasonable
interpretations.

If you look back at the whole thread Marko's above-cited message is in,
we'd considered a bunch of different possible syntaxes for this, and
none of them had much support.  The (*) idea actually is starting to
look pretty good to me.

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


Re: [HACKERS] Support UPDATE table SET(*)=...

2014-10-17 Thread Tom Lane
Merlin Moncure mmonc...@gmail.com writes:
 On Wed, Oct 15, 2014 at 3:48 AM, Atri Sharma atri.j...@gmail.com wrote:
 Thanks for the links, but this patch only targets SET(*) case, which, if I
 understand correctly, the patch you mentioned doesn't directly handle (If I
 understand correctly, the target of the two patches is different).

 Yeah -- in fact, there was some discussion about this exact case.
 This patch solves a very important problem: when doing record
 operations to move data between databases with identical schema
 there's currently no way to 'update' in a generic way without building
 out the entire field list via complicated and nasty dynamic SQL.  I'm
 not sure about the proposed syntax though; it seems a little weird to
 me.  Any particular reason why you couldn't have just done:

 UPDATE table1 SET * = a,b,c, ...

 also,

 UPDATE table1 t SET t = (SELECT (a,b,c)::t FROM...);

 seems cleaner than the proposed syntax for row assignment.  Tom
 objected though IIRC.

That last proposal is no good because it would be ambiguous if the
table contains a column by that name.  The (*) idea actually seems
not bad, since it's shorthand for a parenthesized column list.

I'm not sure about the patch itself though --- in particular, it
doesn't seem to me that it should be touching transformTargetList,
since that doesn't have anything to do with expansion of multiassignments
today.  Probably this is a symptom of having chosen a poor representation
of the construct in the raw grammar output.  However, I've not exactly
wrapped my head around what that representation is ... the lack of any
comments explaining it doesn't help.

A larger question is whether it's appropriate to do the *-expansion
at parse time, or whether we'd need to postpone it to later in order
to handle reasonable use-cases.  Since we expand SELECT * at parse
time (and are mandated to do so by the SQL spec, I believe), it seems
consistent to do this at parse time as well; but perhaps there is a
counter argument.

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


Re: [HACKERS] Support UPDATE table SET(*)=...

2014-10-17 Thread Merlin Moncure
On Fri, Oct 17, 2014 at 10:10 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Merlin Moncure mmonc...@gmail.com writes:
 On Fri, Oct 17, 2014 at 9:55 AM, Marko Tiikkaja ma...@joh.to wrote:
 I don't know about Tom, but I didn't like that:
 http://www.postgresql.org/message-id/5364c982.7060...@joh.to

 Hm, I didn't understand your objection:

 quoting
 So e.g.:
UPDATE foo f SET f = ..;

 would resolve to the table, despite there being a column called f?
 That would break backwards compatibility.
 /quoting

 That's not correct: it should work exactly as 'select' does; given a
 conflict resolve the field name, so no backwards compatibility issue.

 The point is that it's fairly messy (and bug-prone) to have a syntax
 where we have to make an arbitrary choice between two reasonable
 interpretations.

 If you look back at the whole thread Marko's above-cited message is in,
 we'd considered a bunch of different possible syntaxes for this, and
 none of them had much support.  The (*) idea actually is starting to
 look pretty good to me.

Hm, I'll take it then.

merlin


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Support UPDATE table SET(*)=...

2014-10-17 Thread Tom Lane
Marko Tiikkaja ma...@joh.to writes:
 local:marko=#* create table foo(f int);
 CREATE TABLE
 local:marko=#* update foo f set f=1;
 UPDATE 0

 This query would change meaning with your suggestion.

I think it wouldn't; Merlin is proposing that f would be taken as the
field name.  A more realistic objection goes like this:

create table foo(f int, g int);
update foo x set x = (1,2);  -- works
alter table foo add column x int;
update foo x set x = (1,2,3);  -- no longer works

It's not a real good thing if a column addition or renaming can
so fundamentally change the nature of a query.

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


Re: [HACKERS] Support UPDATE table SET(*)=...

2014-10-17 Thread Merlin Moncure
On Fri, Oct 17, 2014 at 10:16 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Marko Tiikkaja ma...@joh.to writes:
 local:marko=#* create table foo(f int);
 CREATE TABLE
 local:marko=#* update foo f set f=1;
 UPDATE 0

 This query would change meaning with your suggestion.

 I think it wouldn't; Merlin is proposing that f would be taken as the
 field name.  A more realistic objection goes like this:

 create table foo(f int, g int);
 update foo x set x = (1,2);  -- works
 alter table foo add column x int;
 update foo x set x = (1,2,3);  -- no longer works

 It's not a real good thing if a column addition or renaming can
 so fundamentally change the nature of a query.

That's exactly how SELECT works.  I also dispute that the user should
be surprised in such cases.

merlin


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Support UPDATE table SET(*)=...

2014-10-17 Thread Tom Lane
Merlin Moncure mmonc...@gmail.com writes:
 On Fri, Oct 17, 2014 at 10:16 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 I think it wouldn't; Merlin is proposing that f would be taken as the
 field name.  A more realistic objection goes like this:
 
 create table foo(f int, g int);
 update foo x set x = (1,2);  -- works
 alter table foo add column x int;
 update foo x set x = (1,2,3);  -- no longer works
 
 It's not a real good thing if a column addition or renaming can
 so fundamentally change the nature of a query.

 That's exactly how SELECT works.  I also dispute that the user should
 be surprised in such cases.

Well, the reason we have a problem in SELECT is that we support an
unholy miscegenation of SQL-spec and PostQUEL syntax: the idea that
SELECT foo FROM foo could represent a whole-row selection is nowhere
to be found in the SQL standard, for good reason IMO.  But we've never
had the courage to break cleanly with this Berkeley leftover and
insist that people spell it SQL-style as SELECT ROW(foo.*) FROM foo.
I'd just as soon not introduce the same kind of ambiguity into UPDATE
if we have a reasonable alternative.

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


Re: [HACKERS] Support UPDATE table SET(*)=...

2014-10-17 Thread Merlin Moncure
On Fri, Oct 17, 2014 at 10:30 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Merlin Moncure mmonc...@gmail.com writes:
 On Fri, Oct 17, 2014 at 10:16 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 I think it wouldn't; Merlin is proposing that f would be taken as the
 field name.  A more realistic objection goes like this:

 create table foo(f int, g int);
 update foo x set x = (1,2);  -- works
 alter table foo add column x int;
 update foo x set x = (1,2,3);  -- no longer works

 It's not a real good thing if a column addition or renaming can
 so fundamentally change the nature of a query.

 That's exactly how SELECT works.  I also dispute that the user should
 be surprised in such cases.

 Well, the reason we have a problem in SELECT is that we support an
 unholy miscegenation of SQL-spec and PostQUEL syntax: the idea that
 SELECT foo FROM foo could represent a whole-row selection is nowhere
 to be found in the SQL standard, for good reason IMO.  But we've never
 had the courage to break cleanly with this Berkeley leftover and
 insist that people spell it SQL-style as SELECT ROW(foo.*) FROM foo.
 I'd just as soon not introduce the same kind of ambiguity into UPDATE
 if we have a reasonable alternative.

Ah, interesting point (I happen to like the terse syntax and use it
often).  This is for posterity anyways since you guys seem to like
Atri's proposal, which surprised me.  However, I think you're over
simplifying things here.  Syntax aside: I think
SELECT f FROM foo f;
and a hypothetical
SELECT row(f.*) FROM foo f;

give different semantics.  The former gives an object of type 'f' and
the latter gives type 'row'.  To get parity, you'd have to add an
extra cast which means you'd have to play tricky games to avoid extra
performance overhead besides being significantly more verbose.  I'm
aware some of the other QUELisms are pretty dodgy and have burned us
in the past (like that whole function as record reference thing) but
pulling a record as a field in select is pretty nice.  It's also
widely used and quite useful in json serialization.

merlin


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Support UPDATE table SET(*)=...

2014-10-17 Thread Marti Raudsepp
On Oct 17, 2014 6:16 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 A more realistic objection goes like this:

 create table foo(f int, g int);
 update foo x set x = (1,2);  -- works
 alter table foo add column x int;
 update foo x set x = (1,2,3);  -- no longer works

 It's not a real good thing if a column addition or renaming can
 so fundamentally change the nature of a query.

I think a significant use case for this feature is when you already have a
row-value and want to persist it in the database, like you can do with
INSERT:
insert into foo select * from populate_record_json(null::foo, '{...}');

In this case the opposite is true: requiring explicit column names would
break the query if you add columns to the table. The fact that you can't
reasonably use populate_record/_json with UPDATE is a significant omission.
IMO this really speaks for supporting shorthand whole-row assignment,
whatever the syntax.

Regards,
Marti


Re: [HACKERS] Support UPDATE table SET(*)=...

2014-10-15 Thread Marti Raudsepp
Hi

On Wed, Oct 15, 2014 at 11:02 AM, Atri Sharma atri.j...@gmail.com wrote:
 Please find attached a patch which implements support for UPDATE table1
 SET(*)=...

I presume you haven't read Tom Lane's proposal and discussion about
multiple column assignment in UPDATE:
http://www.postgresql.org/message-id/1783.1399054...@sss.pgh.pa.us
(Assigning all columns was also discussed there)

And there's a WIP patch:
http://www.postgresql.org/message-id/20930.1402931...@sss.pgh.pa.us

Regards,
Marti


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Support UPDATE table SET(*)=...

2014-10-15 Thread Atri Sharma
On Wednesday, October 15, 2014, Marti Raudsepp ma...@juffo.org wrote:

 Hi

 On Wed, Oct 15, 2014 at 11:02 AM, Atri Sharma atri.j...@gmail.com
 javascript:; wrote:
  Please find attached a patch which implements support for UPDATE table1
  SET(*)=...

 I presume you haven't read Tom Lane's proposal and discussion about
 multiple column assignment in UPDATE:
 http://www.postgresql.org/message-id/1783.1399054...@sss.pgh.pa.us
 (Assigning all columns was also discussed there)

 And there's a WIP patch:
 http://www.postgresql.org/message-id/20930.1402931...@sss.pgh.pa.us



Thanks for the links, but this patch only targets SET(*) case, which, if I
understand correctly, the patch you mentioned doesn't directly handle (If I
understand correctly, the target of the two patches is different).

Regards,

Atri

-- 
Regards,

Atri
*l'apprenant*


Re: [HACKERS] Support UPDATE table SET(*)=...

2014-10-15 Thread Atri Sharma
On Wed, Oct 15, 2014 at 2:18 PM, Atri Sharma atri.j...@gmail.com wrote:





 On Wednesday, October 15, 2014, Marti Raudsepp ma...@juffo.org wrote:

 Hi

 On Wed, Oct 15, 2014 at 11:02 AM, Atri Sharma atri.j...@gmail.com
 wrote:
  Please find attached a patch which implements support for UPDATE table1
  SET(*)=...

 I presume you haven't read Tom Lane's proposal and discussion about
 multiple column assignment in UPDATE:
 http://www.postgresql.org/message-id/1783.1399054...@sss.pgh.pa.us
 (Assigning all columns was also discussed there)

 And there's a WIP patch:
 http://www.postgresql.org/message-id/20930.1402931...@sss.pgh.pa.us



 Thanks for the links, but this patch only targets SET(*) case, which, if I
 understand correctly, the patch you mentioned doesn't directly handle (If I
 understand correctly, the target of the two patches is different).


Digging more, I figured that the patch I posted builds on top of Tom's
patch,  since it did not add whole row cases.

Regards,

Atri