DELETE ... USING

Neil Conway <neilc@samurai.com> writes:

On a related note, UPDATE uses the FROM keyword to denote the list of
relations to join with, whereas DELETE uses USING. Should we make USING
an alias for FROM in UPDATE and if so, should we deprecate FROM? This
would be more consistent, which I suppose is a good thing.

Of course, the entire reason this didn't happen years ago is that we
couldn't agree on what keyword to use... you sure you want to reopen
that discussion?

I don't think changing UPDATE is a good idea. It's consistent with
SELECT and people are used to it.

You could argue that something like

DELETE FROM target [ { USING | FROM } othertables ] ...

is the best compromise. Those who like consistency can write FROM,
those who don't like "FROM a FROM b" can write something else.

regards, tom lane

BTW, this patch is lacking ruleutils.c support. Put a DELETE USING
into a rule and see whether pg_dump will dump the rule correctly ...

regards, tom lane

Tom Lane wrote:

BTW, this patch is lacking ruleutils.c support. Put a DELETE USING
into a rule and see whether pg_dump will dump the rule correctly ...

Good catch; a revised patch is attached.

-Neil

[ CC'ing hackers to see if anyone else wants to weigh in ]

Tom Lane wrote:

Of course, the entire reason this didn't happen years ago is that we
couldn't agree on what keyword to use... you sure you want to reopen
that discussion?

Sure, it doesn't seem too difficult to settle to me.

I don't think changing UPDATE is a good idea. It's consistent with
SELECT and people are used to it.

Fair enough, I can't get too excited about it either.

You could argue that something like

DELETE FROM target [ { USING | FROM } othertables ] ...

is the best compromise. Those who like consistency can write FROM,
those who don't like "FROM a FROM b" can write something else.

This would be fine with me. Are there any other opinions out there on
what syntax would be best for this feature? (For those on -hackers, the
feature in question is adding the ability to specify additional tables
to "join" against in a DELETE, as can be done using FROM in UPDATE.)

-Neil

Hi Neil,

BTW, this patch is lacking ruleutils.c support. Put a DELETE USING
into a rule and see whether pg_dump will dump the rule correctly

...

Good catch; a revised patch is attached.

Greate job. But I'm worried about add_missing_from enabled. See:

euler=# delete from t3 using t1 where b > 500;
DELETE 4
euler=# select * from t3;
x | y
---+---
(0 rows)

In this case, I 'forget' to do the join and it delete all rows from t3.
I know that user needs to pay attention, but ... What about default
add_missing_from to off?

The other case is:

euler=# select * from t1 where t1.a = t3.x;
NOTICE: adding missing FROM-clause entry for table "t3"
NOTICE: adding missing FROM-clause entry for table "t3"
a | b
---+----
5 | 10
(1 row)

euler=# delete from t1 where t1.a = t3.x;
DELETE 1
euler=#

I think we need at least a NOTICE here. Of course it could be extended
to UPDATE too.

BTW, what about regression tests for UPDATE ... FROM?

PS> all examples are extracted from regression database.

Euler Taveira de Oliveira
euler[at]yahoo_com_br

Yahoo! Acesso Gr�tis - Internet r�pida e gr�tis.
Instale o discador agora! http://br.acesso.yahoo.com/

Euler Taveira de Oliveira wrote:

I'm worried about add_missing_from enabled.

The plan is to make add_missing_from default to false in 8.1

euler=# delete from t3 using t1 where b > 500;
DELETE 4
euler=# select * from t3;
x | y
---+---
(0 rows)

In this case, I 'forget' to do the join and it delete all rows from t3.
I know that user needs to pay attention, but ... What about default
add_missing_from to off?

add_missing_from would not make any difference here. The problem is that
there is no join clause between t3 and t1, not that t1 is being
implicitly added to the range table (which is what add_missing_from
would warn you about).

The problem is analogous to a SELECT like:

SELECT * FROM t3, t1 WHERE b > 500;

i.e. forgetting to specify a join clause and therefore accidentally
computing the cartesian product. There has been some gripping recently
on -hackers about disabling this or emitting a warning of some kind.

euler=# select * from t1 where t1.a = t3.x;
NOTICE: adding missing FROM-clause entry for table "t3"
NOTICE: adding missing FROM-clause entry for table "t3"
a | b
---+----
5 | 10
(1 row)

euler=# delete from t1 where t1.a = t3.x;
DELETE 1
euler=#

I think we need at least a NOTICE here. Of course it could be extended
to UPDATE too.

I can see an argument for having a NOTICE here. On the other hand,
add_missing_from will default to false in 8.1, so presumably the only
people enabling it will be those who specifically need backward
compatibility for old applications that they cannot afford to change.
Filling the logs with bogus NOTICEs would be sufficiently annoying it
would probably force some people to modify their applications, thereby
defeating the point of having a backward compatibility GUC variable in
the first place.

BTW, what about regression tests for UPDATE ... FROM?

I agree regression tests would be useful -- you are welcome to send a
patch :)

-Neil

Neil Conway <neilc@samurai.com> writes:

Euler Taveira de Oliveira wrote:

euler=# delete from t1 where t1.a = t3.x;
DELETE 1
euler=#

I think we need at least a NOTICE here. Of course it could be extended
to UPDATE too.

I can see an argument for having a NOTICE here. On the other hand,
add_missing_from will default to false in 8.1, ...

... but when it is TRUE, there should be a notice, same as there is in
SELECT. UPDATE should produce such a notice too, IMHO. Probably we
omitted the message originally because there was no way to avoid it
in a DELETE, but now there will be.

regards, tom lane

Tom Lane wrote:

... but when it is TRUE, there should be a notice, same as there is in
SELECT. UPDATE should produce such a notice too, IMHO. Probably we
omitted the message originally because there was no way to avoid it
in a DELETE, but now there will be.

Well, my previous message described why I'm not sure that this line of
reasoning is correct. I think the only really proper configuration is
add_missing_from=false and an explicit USING/FROM list. Just about the
only reason to enable add_missing_from would be for compatibility with
previous releases of PostgreSQL -- and that "compatible" behavior is not
to issue a warning for UPDATE and DELETE in this situation. If the user
deliberately enables add_missing_from, I'm inclined to trust them that
they know what they're doing.

-Neil

Neil Conway <neilc@samurai.com> writes:

Well, my previous message described why I'm not sure that this line of
reasoning is correct. I think the only really proper configuration is
add_missing_from=false and an explicit USING/FROM list. Just about the
only reason to enable add_missing_from would be for compatibility with
previous releases of PostgreSQL -- and that "compatible" behavior is not
to issue a warning for UPDATE and DELETE in this situation.

Hmm. There's some merit in that position, but consider this: we are
encouraging people rather strongly to move to the add_missing_from=false
behavior. So add_missing_from=true could be seen as a testing situation
in which you'd like to know which of your queries have a problem, while
not actually causing your app to fail. Strict backwards compatibility
won't produce the warning but also won't help you find what will break.

regards, tom lane

Tom Lane wrote:

Hmm. There's some merit in that position, but consider this: we are
encouraging people rather strongly to move to the add_missing_from=false
behavior. So add_missing_from=true could be seen as a testing situation
in which you'd like to know which of your queries have a problem, while
not actually causing your app to fail. Strict backwards compatibility
won't produce the warning but also won't help you find what will break.

Hmm, ok, I can see where that would be useful.

Looking at how to implement this, there is some rather dodgy code in
warnAutoRange() in parse_relation.c that only emits the notice about
adding a missing FROM clause entry if the query already has at least one
range table entry in its FROM clause. The idea appears to be to not
issue warnings about queries like "SELECT foo.*;", but it also means we
don't end up warning about DELETE and UPDATE.

I think the right fix is to remove the "inFromCl" check, and always
issue a notice. With add_missing_from=true, all these queries are
rejected anyway, so I think it makes sense to warn about all of them
when add_missing_from is disabled. Objections?

-Neil

Hi Neil,

Looking at how to implement this, there is some rather dodgy code in
warnAutoRange() in parse_relation.c that only emits the notice about
adding a missing FROM clause entry if the query already has at least
one
range table entry in its FROM clause. The idea appears to be to not
issue warnings about queries like "SELECT foo.*;", but it also means
we
don't end up warning about DELETE and UPDATE.

I think the right fix is to remove the "inFromCl" check, and always
issue a notice. With add_missing_from=true, all these queries are
rejected anyway, so I think it makes sense to warn about all of them
when add_missing_from is disabled. Objections?

No. That's why I'm thinking now while looking at the code :) Could you
provide a patch?

Euler Taveira de Oliveira
euler[at]yahoo_com_br

Yahoo! Acesso Gr�tis - Internet r�pida e gr�tis.
Instale o discador agora! http://br.acesso.yahoo.com/

Euler Taveira de Oliveira wrote:

Could you provide a patch?

Sure, a revised patch is attached. Note that this change will also
require updating 25 (!) of the regression tests, since they use the
SELECT-without-FROM syntax. I will update the tests (by adding an
explicit FROM clause) before applying the patch -- which I'll do
tomorrow, barring any objections.

-Neil

Neil Conway wrote:

Sure, a revised patch is attached. Note that this change will also
require updating 25 (!) of the regression tests, since they use the
SELECT-without-FROM syntax.

I've applied the attached patch to HEAD. Due to the widespread updates
to the regression tests, the tests for some platforms may be (trivially)
broken, despite my efforts to make the necessary updates. If that's the
case, please let me know.

-Neil

Neil Conway wrote:

Euler Taveira de Oliveira wrote:

Could you provide a patch?

Sure, a revised patch is attached. Note that this change will also
require updating 25 (!) of the regression tests, since they use the
SELECT-without-FROM syntax. I will update the tests (by adding an
explicit FROM clause) before applying the patch -- which I'll do
tomorrow, barring any objections.

I just checked current CVS and see exactly what you describe:

test=> SELECT pg_class.* LIMIT 0;
ERROR: missing FROM-clause entry for table "pg_class"

test=> SET add_missing_from=true;
SET
test=> SELECT pg_class.* LIMIT 0;
NOTICE: adding missing FROM-clause entry for table "pg_class"

Is this what we want? I don't think so. I thought we wanted to
maintain the backward-compatible syntax of no FROM clause.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:

Is this what we want? I don't think so. I thought we wanted to
maintain the backward-compatible syntax of no FROM clause.

We do? Why?

It is just as noncompliant with the SQL spec as other variants of this
behavior. add_missing_from would *always* have rejected those queries,
so ISTM we have been discouraging this case for as long as
add_missing_from has existed. If we want to allow this syntax by
default, we will need to effectively redefine the meaning of
add_missing_from -- which is fine, I just didn't think anyone wanted that.

-Neil

Bruce Momjian <pgman@candle.pha.pa.us> writes:

test=> SELECT pg_class.* LIMIT 0;
NOTICE: adding missing FROM-clause entry for table "pg_class"

Is this what we want? I don't think so. I thought we wanted to
maintain the backward-compatible syntax of no FROM clause.

Well, the discussion earlier in the week concluded that
add_missing_from=true should emit a notice in every case where
add_missing_from=false would fail. Do you want to argue against
that conclusion?

regards, tom lane

Neil Conway wrote:

Bruce Momjian wrote:

I just checked current CVS and see exactly what you describe:

test=> SELECT pg_class.* LIMIT 0;
ERROR: missing FROM-clause entry for table "pg_class"

test=> SET add_missing_from=true;
SET
test=> SELECT pg_class.* LIMIT 0;
NOTICE: adding missing FROM-clause entry for table "pg_class"

Is this what we want? I don't think so. I thought we wanted to
maintain the backward-compatible syntax of no FROM clause.

We do? Why?

It is just as noncompliant with the SQL spec as other variants of this
behavior. add_missing_from would *always* have rejected those queries,
so ISTM we have been discouraging this case for as long as
add_missing_from has existed. If we want to allow this syntax by
default, we will need to effectively redefine the meaning of
add_missing_from -- which is fine, I just didn't think anyone wanted that.

Oh, so by setting add_missing_from to false, this query starts to fail.

I don't know how much people use that syntax. I use it sometimes as
hack in psql to avoid typing FROM, but that's hardly a reason to support
it.

If everyone else is OK with having it fail, that is fine with me, but I
wanted to make sure folks saw this was happening. I basically saw no
discussion that we were disabling that syntax. [CC moved to hackers.]

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

test=> SELECT pg_class.* LIMIT 0;
NOTICE: adding missing FROM-clause entry for table "pg_class"

Is this what we want? I don't think so. I thought we wanted to
maintain the backward-compatible syntax of no FROM clause.

Well, the discussion earlier in the week concluded that
add_missing_from=true should emit a notice in every case where
add_missing_from=false would fail. Do you want to argue against
that conclusion?

I didn't realize that "SELECT pg_class.*" was now going to fail because
add_missing_from is false. I didn't link those two together in my head,
probably because the warning is not emitted if there is no FROM clause.

Anyway, I am fine either way but wanted to publicise it at least.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

Well, the discussion earlier in the week concluded that
add_missing_from=true should emit a notice in every case where
add_missing_from=false would fail. Do you want to argue against
that conclusion?

I didn't realize that "SELECT pg_class.*" was now going to fail because
add_missing_from is false.

It always has, though. Neil hasn't changed the behavior when
add_missing_from is false ... he's only made add_missing_from=true
emit a notice in *every* case where add_missing_from=false would
fail.

regards, tom lane