Обсуждение: Queries using rules show no rows modified?

I'm using 7.2.1 on a Debian system.

If I do an insert or update or delete on a table, postgres tells me
how many rows were affected.

Using the following input to psql, I got the results:

INSERT 0 0
UPDATE 0
DELETE 0

Is this expected?  The principle of least suprise suggests to me that
regardless of the query being rewritten, there is some number of
tuples being affected, and it would thus still be appropriate to
return that number.

I realize it's not technically a "bug", since there's no particular
guarantee that someone specified existing records or whatnot, but as
an additional fourth-string check in some web code I put together, I
was checking to see if stuff was returned or updated (since the system
should only being allowing changes to things that exist) as a
heuristic to guard against 1) bugs, and 2) attempts to maliciously
subvert the public interface.

I can find no mention of this issue in the documentation regarding the
rule system.  Anyone have any guidance?

Mike.

-----8<-----
drop sequence member_id_seq;
create sequence member_id_seq;

drop table member;
create table member ( id integer not null constraint member_id primary key default nextval('member_id_seq'), created
timestampnot null default now (), modified timestamp not null default now (), deleted timestamp default null, email
charactervarying (128) not null constraint member_email unique, password character varying (128) not null
 
);

drop view members;
create view members as select * from member m1 where m1.deleted is null;

drop rule members_delete;
create rule members_delete as on delete to members do instead update member set deleted = current_timestamp;

drop rule members_insert;
create rule members_insert as on insert to members do instead insert into member (email, password) values (new.email,
new.password);

drop rule members_update;
create rule members_update as on update to members do instead update member set email = new.email, password =
new.password;

insert into members (email, password) values ('mdorman@wombat.org','pinochle');

update members set email='mdorman@lemur.org', password='wombat' where id = 1;

delete from members where id = 1;
----->8-----

Michael Alan Dorman wrote:
> 
> I'm using 7.2.1 on a Debian system.
> 
> If I do an insert or update or delete on a table, postgres tells me
> how many rows were affected.
> 
> Using the following input to psql, I got the results:
> 
> INSERT 0 0
> UPDATE 0
> DELETE 0
> 
> Is this expected?  The principle of least suprise suggests to me that
> regardless of the query being rewritten, there is some number of
> tuples being affected, and it would thus still be appropriate to
> return that number.

You are right. It's a bug introduced in 7.2.
Please check the thread [GENERAL]([HACKERS]) 
Using views and MS access via odbc.
If there's no objection, I would commit the patch
in the thread to both 7.2-stable and the current.

regards,
Hiroshi Inouehttp://w2422.nsk.ne.jp/~inoue/

Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> If there's no objection, I would commit the patch
> in the thread to both 7.2-stable and the current.

Last I checked, I objected to your solution and you objected to mine
... so I think it's on hold until we get some more votes.
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:
> Last I checked, I objected to your solution and you objected to mine
> ... so I think it's on hold until we get some more votes.

Well, If I'm reading this code from DBD::Pg's dbdimp.c correctly, I
think that the perl module, at least, feels that the number is much
more important than the actual command that is returned:
   if (PGRES_TUPLES_OK == status) {       [...]   } else if (PGRES_COMMAND_OK == status) {       /* non-select
statement*/       if (! strncmp(cmdStatus, "DELETE", 6) || ! strncmp(cmdStatus, "INSERT", 6) || ! strncmp(cmdStatus,
"UPDATE",6)) {           ret = atoi(cmdTuples);       } else {           ret = -1;       }
 

It appears that while the implementation does look to make sure the
return string is recognizable, it doesn't care too much beyond that
which one it is---not suprising as that string is, as far as the DBI
interface is concerned, just "extra information" that has no defined
interface to get back out to the user.  More important, at least from
the standpoint of a user of the module seems to be that the cmdTuples
(gotten from PQcmdTuples) represents number affected so it can be
returned.

In fact, now that I look at it, this change has in fact broken the
DBD::Pg interface with respect to the DBI when used in the presence of
rules, because the DBI spec states that it will either return the
number of tuples affected or -1 if that is unknown, rather than 0,
which breaks as a result of this change.

I guess there's an argument to be made as to whether PostgreSQL
provides any guarantees about this number being correct or even valid,
but the fact that the library interface makes it available, and I see
nothing in the documentation of the function that suggests that that
number is unreliable suggests that it is not an error to depend on it.

So, If I understood the proposals correctly, I think that means that
this implementation argues for, or at least would work well with,
Hiroshi's solution, since yours, Tom, would return a false zero in
certain (perhaps rare) situations, arguably loosing information that
the perl module, at least, could use, and the library purports to make
available, in order to preserve information it does not.

I guess there is one other possibility, though I don't know how
radical it would be in either implementation or effects: return the
empty string from PQcmdTuples in this situation.  It serves as
something of an acknowledgement that what went on was not necessarily
fish or fowl, while still being, from my reading of the docs, a valid
return.  The perl module certainly regards it as one, albeit one that
transmits precious little information.  Well-written interfaces should
already be able to cope with it, given that it is documented as a
possiblity in the docs, right?

Mike.

Michael Alan Dorman <mdorman@debian.org> writes:
> So, If I understood the proposals correctly, I think that means that
> this implementation argues for, or at least would work well with,
> Hiroshi's solution, since yours, Tom, would return a false zero in
> certain (perhaps rare) situations,

IMHO Hiroshi's solution would return false information in more cases
than mine.

The basic argument in favor of a patch like this is that if a rule
replaces (DO INSTEAD) a command with another command of the same general
type, it is useful to return the tag for the replacement command not the
original.  I agree with that.  I do not agree with the claim that we
should return a tag from the underlying implementation when a rule
rewrites a query into a form totally unrecognizable to the client.
Consider again the example of transforming an UPDATE on a view into
an INSERT on some underlying table --- but let's reverse it now and
suppose it's the other way, the client sends INSERT and the rule
replaces it with an UPDATE.  If the client is expecting to get back
"INSERT m n" and actually gets back "UPDATE n", isn't that client
likely to break?

Another issue is that the whole thing falls down if the rewriting
generates more than one query; both Hiroshi's proposal and mine will
not return any substitute tag then.  This seems rather restrictive.
Maybe we could have behavior like this: if the original command is
replaced, then use the tag from the last substituted command of the
same class (eg, if you rewrite an UPDATE into an INSERT and an UPDATE,
you get the tag from the UPDATE).  If there is *no* substitute command
of the same class, I still believe that returning "UPDATE 0" is correct
behavior.  You sent an update, zero tuples were updated, end of story.
There is not scope in this API to tell you about how many tuples might
have been inserted or deleted.

Note that as of CVS tip, the firing order of rules is predictable,
so the rule author can control which substituted command is "the last
one".  Without this I don't think that the above would work, but with
it, it seems like a moderately clean answer.  Moreover it's at least
somewhat compatible with the pre-7.2.1 behavior --- where you got the
tag from the last command *executed* regardless of any other
considerations.  That was definitely broken.
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:
> The basic argument in favor of a patch like this is that if a rule
> replaces (DO INSTEAD) a command with another command of the same
> general type, it is useful to return the tag for the replacement
> command not the original.  I agree with that.

I would argue that the argument in favor of a patch is that there's no
documentation anywhere that behavior changed, or that PQcmdTuples will
not return the expected result in the presence of rules. :-)

Is the change behaviorou propose implementable as a patch to 7.2.1?

> If the client is expecting to get back "INSERT m n" and actually
> gets back "UPDATE n", isn't that client likely to break?

Perhaps.  How many clients are checking that the string returned
matches the query it sent?

I've checked DBD::Pg, it doesn't.  I've checked psycopg, it doesn't,
though it looks like its handling of the value might be a bit bogus.
ecpg doesn't, though it looks like it might choke on an empty string.
PHP doesn't.  QT3 doesn't.  PoPY (another Python interface) doesn't.
The TCL library doesn't even look at the return, it just passes it
back, so I suppose there might be applications doing a direct look.
The python lib included with postgresql doesn't.  In fact, the idiom
is either (in pseudocode):
 if (temp = PQcmdTuples (result)) {   numTuples = atoi (temp); } else {   numTuples = some other arbitrary value; }

or:
 numTuples = atoi (PQcmdTuples (result));

So, no, my *very* unscientific and non-comprehensive survey suggests
that your fears are mostly groundless.  But I haven't seen a single
interface that *is* depending on that being correct, but many of them
return misleading results if PQcmdTuples does.

Which is, if I haven't hammered this enough, not mentioned anywhere in
the documentation. 
> Another issue is that the whole thing falls down if the rewriting
> generates more than one query; both Hiroshi's proposal and mine will
> not return any substitute tag then.  This seems rather restrictive.

If, when you say, "will not return any substitute tag then.", you mean
that, as an end result PQcmdTuple would return an empty string, well,
that seems reasonable---it keeps the DB from returning bogus info, and
an empty string returned from PQcmdTuple _is_ documented as a valid
response, and it looks like most interfaces would handle it just fine
(except maybe for ecpg, which I would argue either has a bug or I'm
not reading right).

I guess there's the argument to be made that any overly-zealous
interface that might choke on getting a different tag back might also
choke on getting no tag back.  But, again, I don't see any doing any
of this.  And they *all* seem to expect PQcmdTuples to either return
legitimate data or nothing at all.

> Maybe we could have behavior like this: if the original command is
> replaced, then use the tag from the last substituted command of the
> same class (eg, if you rewrite an UPDATE into an INSERT and an
> UPDATE, you get the tag from the UPDATE).  If there is *no*
> substitute command of the same class, I still believe that returning
> "UPDATE 0" is correct behavior.  You sent an update, zero tuples
> were updated, end of story.

As long as you document that PQcmdTuples cannot be relied on when
using rules, since the rules might change the query sufficiently to
make it unrecognizable, that's probably OK, though it'll require
significant changes to just about all interface libraries.

> Note that as of CVS tip, the firing order of rules is predictable,
> so the rule author can control which substituted command is "the
> last one".  Without this I don't think that the above would work,
> but with it, it seems like a moderately clean answer.  Moreover it's
> at least somewhat compatible with the pre-7.2.1 behavior --- where
> you got the tag from the last command *executed* regardless of any
> other considerations.  That was definitely broken.

So should I interpret these references to CVS tip as suggesting that
the fix for this change in behavior is not going to be seen until 7.3,
or just that a most-complete fix that tries to deal with multi-rule
invocations would have to wait for 7.3, but that a fix for the simpler
'do instead' case could show up in a 7.2.X release?

Because it seems to me that if we're not going to see a release with a
fix for this change in behavior, we need to make sure that maintainers
of all interfaces know that all bets are off regarding PQcmdTuples in
the (I believe undetectable) presence of rules so they'll make no
effort to use it.

Mike.

Michael Alan Dorman <mdorman@debian.org> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> If the client is expecting to get back "INSERT m n" and actually
>> gets back "UPDATE n", isn't that client likely to break?

> Perhaps.  How many clients are checking that the string returned
> matches the query it sent?

> I've checked DBD::Pg, it doesn't.

You are confusing client behavior (by which I meant application)
with library behavior.  In libpq terms, an application that's sent
an INSERT command might expect to be able to retrieve an OID with
PQoidValue().  Whether the library avoids core-dumping doesn't mean
that the calling app will behave sanely.

> I would argue that the argument in favor of a patch is that there's no
> documentation anywhere that behavior changed, or that PQcmdTuples will
> not return the expected result in the presence of rules. :-)

The motivation for making a change was to try to *preserve* pre-7.2
behavior in the case of INSERTs, where formerly you got back an INSERT
tag even in the presence of ON INSERT DO not-INSTEAD rules.  7.2 broke
that; 7.2.1 fixed that case but changed the behavior for INSTEAD cases.
What we're realizing now is that we need an actually designed behavior,
rather than the implementation artifact that happened to yield pleasant
results most of the time before 7.2.

I'm arguing that the "designed behavior" ought to include the
stipulation that the tag you get back will match the command you sent.
I think that anything else is more likely to confuse clients than help
them.

> Which is, if I haven't hammered this enough, not mentioned anywhere in
> the documentation.

Mainly because no one ever designed the behavior; the pre-7.2
implementation didn't really think about what should happen.

> I guess there's the argument to be made that any overly-zealous
> interface that might choke on getting a different tag back might also
> choke on getting no tag back.  But, again, I don't see any doing any
> of this.  And they *all* seem to expect PQcmdTuples to either return
> legitimate data or nothing at all.

No, you're still missing the point.  PQcmdTuples isn't going to dump
core, because it has no context about what was expected: it sees a tag
and interprets it as best it can, without any idea about what the
calling app might be expecting.  What we need to think about here is
what linkage an *application* can reasonably expect between the command
it sends and the tag it gets back (and, hence, the info it can expect to
retrieve from the tag).

> As long as you document that PQcmdTuples cannot be relied on when
> using rules, since the rules might change the query sufficiently to
> make it unrecognizable, that's probably OK, though it'll require
> significant changes to just about all interface libraries.

One more time: there will be zero change in any interface library,
no matter what we do here.  The libraries operate at too low a level
to be affected; they have no idea what command you sent.  I'm not even
convinced that PQcmdTuples is where to document the issue --- it seems
to me to be a rule question, instead.

> So should I interpret these references to CVS tip as suggesting that
> the fix for this change in behavior is not going to be seen until 7.3,
> or just that a most-complete fix that tries to deal with multi-rule
> invocations would have to wait for 7.3, but that a fix for the simpler
> 'do instead' case could show up in a 7.2.X release?

Until we've decided what *should* happen, it's premature to discuss
whether we can fix it correctly in 7.2.X or should install a quick-hack
change instead.  I'd prefer to fix it correctly but we must not let
ourselves be seduced by a quick hack into not thinking about what the
behavior really ideally ought to be.  We've done that once too often
already ;-)

FWIW, I'm not at all sure that there will *be* any 7.2.2 release
before 7.3.  There hasn't so far been enough volume of fixes to
justify one (no, this problem doesn't justify one IMHO...)
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:
> You are confusing client behavior (by which I meant application)
> with library behavior.  In libpq terms, an application that's sent
> an INSERT command might expect to be able to retrieve an OID with
> PQoidValue().  Whether the library avoids core-dumping doesn't mean
> that the calling app will behave sanely.

No, Tom, I'm not confusing them.  I'm in no way concerned with
PQcmdTuple coredumping because the published interface specifies that
it can return a null string if it finds it necessary, which implies
that somewhere down there it's doing some decent error handling to
figure out if it's gotten something back it can make sense of and
acting appropriately.

You brought up core dumps.  My concern has been exclusively with the
potential change in behavior this can cause in applications.

So I've been doing is going and downloading the source to, and looking
at the behavior of, some of the libraries that some---probably many,
maybe even most---clients are using, those for perl and python and
php, and I am finding that most of them do not even expose the
information whose (mis-)interpretation concerns you.

So, for those interfaces, at least, there was no problem to be fixed
in the first place.

Still, you don't have to have something actively breaking to warrant
fixing a bug, so there's no reason to have not made the change that
was made.

The problem is that, at the same time, I am finding that the change to
postgresql 7.2 may make application code using those interfaces begin
to operate in new and different ways because, although they aren't
paying attention to the string, which you are concerned with, they
*are* paying attention to the numbers.

Many of those interfaces, where they used to return 1 or 10 or 5000 or
6432456, will now be returning 0, which thanks to the great C
tradition, is often interpreted to mean "false", which may lead an
application to question why "nothing happened."  As mine did.

And this isn't necessarily application programmers making bad choices;
the Perl interface, at least, documents the fact that it returns the
number of rows affected or -1 if that is unknowable---but the change
in behavior leads the perl interface to think it knows, when in fact
it doesn't.

If I knew java better, I'd check the JDBC driver.  I mean, imagine:
Perl, python, php and java, all with undocumented unpredictable
behavior in the presence of 'update do instead' rules.  Break all four
and you've just created a potential problem for everyone who does web
development.

That, I think, is one of the more egregious changes in behavior I've
seen in the few years I've been following PostgreSQL, and yet not only
is there any documentation, I feel like I'm having to fight to even
get it acknowledge that it is the bigger problem than the blasted
strings not matching because it affects a heck of a lot more stuff in
a much more direct manner.

Still, handle this however you want.  I'll go fix the Perl driver to
pretend PQcmdTuples doesn't exist, since it can't be trusted to
deliver reliable information, and just have it return -1, and *my*
apps will be OK.  Maybe some months down the road when 7.3 finally
straggles into view there will be a solution.  Hopefully no one will
have been burned.

Anyway, I'm done beating this dead horse, since the display is
obviously bothering people.

Mike.

Tom Lane wrote:
> 
> Michael Alan Dorman <mdorman@debian.org> writes:
> > So, If I understood the proposals correctly, I think that means that
> > this implementation argues for, or at least would work well with,
> > Hiroshi's solution, since yours, Tom, would return a false zero in
> > certain (perhaps rare) situations,
> 
> IMHO Hiroshi's solution would return false information in more cases
> than mine.


My solution never returns false information as to
patched cases though the returned result may be
different from the one clients expect.
Probably your solution doesn't return false
information either if 'UPDATE 0' means UPDATE 0
but unknown INSERT/DELETEs. But few(maybe no ?)
clients seem to think of it and what could clients
do with such infos in the first place ? 
Of cource it is nice to have a complete solution
immediately but it doesn't seem easy. My patch is
only a makeshift solution but fixes the most
siginificant case(typical updatable views). 

regards, 
Hiroshi Inouehttp://w2422.nsk.ne.jp/~inoue/

Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> Of cource it is nice to have a complete solution
> immediately but it doesn't seem easy. My patch is
> only a makeshift solution but fixes the most
> siginificant case(typical updatable views). 

I would like to devise a complete solution *before* we consider
installing makeshift solutions (which will institutionalize wrong
behavior).

There seems to be some feeling here that in the presence of rewrites
you only want to know that "something happened".  Are you suggesting
that the returned tuple count should be the sum of all counts from
insert, update, and delete actions that happened as a result of the
query?  We could certainly implement that, but it does not seem like
a good idea to me.

I'm also concerned about having an understandable definition for the
OID returned for an INSERT query --- if there are additional INSERTs
triggered by rules, does that mean you don't get to see the OID assigned
to the single row you tried to insert?  You'll definitely get push-back
if you propose that.  But if we add up all the actions for the generated
queries, we are quite likely to be returning an OID along with an insert
count greater than one --- which is certainly confusing, as well as
contrary to the existing documentation about how it works.

Let's please quit worrying about "can we install a hack today" and
instead try to figure out what a sensible behavior is.  I don't think
it's likely to be hard to implement anything we might come up with,
considering how tiny this API is.
        regards, tom lane

Tom Lane wrote:
> 
> Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> > Of cource it is nice to have a complete solution
> > immediately but it doesn't seem easy. My patch is
> > only a makeshift solution but fixes the most
> > siginificant case(typical updatable views).
> 
> I would like to devise a complete solution *before* we consider
> installing makeshift solutions (which will institutionalize wrong
> behavior).
> 
> There seems to be some feeling here that in the presence of rewrites
> you only want to know that "something happened".  Are you suggesting
> that the returned tuple count should be the sum of all counts from
> insert, update, and delete actions that happened as a result of the
> query?  We could certainly implement that, but it does not seem like
> a good idea to me.

What should the backends return for complicated rewrites ?
And how should/could clients handle the results ?
It doesn't seem easy to me and it seems a flaw of rule
system. Honestly I don't think that the psqlodbc driver
can guarantee to handle such cases properly.
However both Ron's case and Michael's one are ordinary
updatable views. If we can't handle the case properly, 
we could never recommend users to use (updatable) views.

regards, 
Hiroshi Inouehttp://w2422.nsk.ne.jp/~inoue/

On Fri, 2002-05-10 at 06:27, Tom Lane wrote:
> I'm also concerned about having an understandable definition for the
> OID returned for an INSERT query --- if there are additional INSERTs
> triggered by rules, does that mean you don't get to see the OID assigned
> to the single row you tried to insert? 

At least when there was actually no insert you don't

and if there actually was more than 1 insert then INSERT 0 N seems quite
reasonable to me.

It may even be that returning a concatenation of results would be
acceptable for current libs

INSERT OID 1 INSERT 0 3 UPDATE 2 DELETE 2

> You'll definitely get push-back
> if you propose that.  But if we add up all the actions for the generated
> queries, we are quite likely to be returning an OID along with an insert
> count greater than one --- which is certainly confusing, as well as
> contrary to the existing documentation about how it works.
> 
> Let's please quit worrying about "can we install a hack today" and
> instead try to figure out what a sensible behavior is.

The problem seems to be that recent changes broke updatable views for a
few users. So have these basic options:

1. revert the changes until we have a consensus on doing the right thing  (seems best to me)
2. change clients (client libraries) for 7.2 cycle at least
3. not revert but install a hack today so that it seems like things  still work ;)
4. figure out correct behaviour and do that for 7.2.2
5. do nothing and tell users to keep themselves busy with other things  until there is consensus about new behaviour.

option 5 seems to be worst, as it leaves users in a state with no clear
view of what is going to happen - we have just changed one arguably
broken behaviour for a new one and are probably going to change it again
soon when we figure out what the right behaviour should be.

> I don't think
> it's likely to be hard to implement anything we might come up with,
> considering how tiny this API is.

The sensible behaviour for updatable views would be to report ho many
rows visible through this view were changed, but this can be hard to do
in a generic way.

-----------------
Hannu

Hiroshi Inoue wrote:
> Tom Lane wrote:
> >
> > Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> > > Of cource it is nice to have a complete solution
> > > immediately but it doesn't seem easy. My patch is
> > > only a makeshift solution but fixes the most
> > > siginificant case(typical updatable views).
> >
> > I would like to devise a complete solution *before* we consider
> > installing makeshift solutions (which will institutionalize wrong
> > behavior).
> >
> > There seems to be some feeling here that in the presence of rewrites
> > you only want to know that "something happened".  Are you suggesting
> > that the returned tuple count should be the sum of all counts from
> > insert, update, and delete actions that happened as a result of the
> > query?  We could certainly implement that, but it does not seem like
> > a good idea to me.
>
> What should the backends return for complicated rewrites ?
> And how should/could clients handle the results ?
> It doesn't seem easy to me and it seems a flaw of rule
> system. Honestly I don't think that the psqlodbc driver
> can guarantee to handle such cases properly.
> However both Ron's case and Michael's one are ordinary
> updatable views. If we can't handle the case properly,
> we could never recommend users to use (updatable) views.
   The  fact  that our rule system is that powerful that you can   have multi-action rules is a flaw ... awe.
   Do you think that  if  a  trigger  suppresses  your  original   insert, but instead does 2 inserts somewhere else
andanother   update and delete here and  there,  then  0  is  the  correct   answer  to  the client?  Well, that's what
happensnow, so it   should irritate your client in exactly the same way.  So  not   only  our rule system, but our
triggersystem has a flaw too.
 


Jan

--

#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me.                                  #
#================================================== JanWieck@Yahoo.com #

Tom Lane wrote:
> Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> > Of cource it is nice to have a complete solution
> > immediately but it doesn't seem easy. My patch is
> > only a makeshift solution but fixes the most
> > siginificant case(typical updatable views).
>
> I would like to devise a complete solution *before* we consider
> installing makeshift solutions (which will institutionalize wrong
> behavior).
>
> There seems to be some feeling here that in the presence of rewrites
> you only want to know that "something happened".  Are you suggesting
> that the returned tuple count should be the sum of all counts from
> insert, update, and delete actions that happened as a result of the
> query?  We could certainly implement that, but it does not seem like
> a good idea to me.
   IMHO  the  answer  should  only  be a number if the rewritten   querytree list consists of one  query  of  the  same
command   type.  everything else has to lead into "unknown".
 


Jan

--

#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me.                                  #
#================================================== JanWieck@Yahoo.com #

Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> What should the backends return for complicated rewrites ?

Well, given that we have only two or three fields to work in,
it obviously has to be a very simplified view of what happened.
But we have to define *something*.

> And how should/could clients handle the results ?
> It doesn't seem easy to me and it seems a flaw of rule
> system.

No, the problem is that the command tag API was designed without any
thought for rule rewriting.  But I don't think it's worth revising
that API completely.  Even if we did, we'd still have to define what
behavior would be seen by clients that use the existing PQcmdTuples,
etc, calls; so we'd still have to solve these same issues.

Come on, guys, work with me a little here.  I've thrown out several
alternative suggestions already, and all I've gotten from either of
you is refusal to think about the problem.

I was thinking last night that it might help to break down the issue a
little bit.  We have either two or three result fields to think about:
the tag name, the tuple count, and in the case of INSERT the inserted
row OID.  Let's consider each one independently.

1. The tag name: AFAICS, this ought *always* to match the type of the
original command submitted by the client.  Doing otherwise could confuse
clients that are submitting multiple commands per query string.
Besides, the only possible downside from making this requirement is that
we couldn't send back an insertion OID when the original command was
an update or delete.  How likely is it that a client would expect to
be able to get an insertion OID from such a command?

2. The inserted row OID: per above, will be supplied only if the
original command was an INSERT.  If the original insert command is
not removed (no INSTEAD rule), then I think this result should clearly
come from the execution of the original command, regardless of any
additional INSERTs added by rules.  If the original command is removed
by INSTEAD, then we can distinguish three sub-cases: a. No INSERTs in rewriter output: easy, we must return 0. b.
Exactlyone INSERT in rewriter output: pretty easy to agree that    we should return this command's result. c: More than
oneINSERT in rewriter output: we have a couple of    possibilities here.  It'd be reasonable to directly use the
resultof the last INSERT, or we could total the results of    all the INSERTs (ie, if taken together they insert a sum
total   of one row, return that row OID; else return 0).  Maybe there    are other possible behaviors.  Any thoughts?
 

3. The tuple count: this seems the most contentious issue.  Again,
if there is no INSTEAD rule I'd be strongly inclined to say we
should just return the count from the original command, ignoring any
commands added by rules.  If there is an INSTEAD, we've discussed
several possibilities: use result of last command in the rewritten
series, use result of last command of same type as original command,
sum up the results of all the rewritten commands, maybe some others
that I forgot.

Given Michael's concern about being able to "tell that something
happened", I'm inclined to go with the summing-up behavior in the
INSTEAD cases.  This would lead to the following boiled-down behavior:

A. If original command is executed (no INSTEAD), return its tag as-is,
regardless of commands added by rules.

B. If original command is not executed, then return its tag name
plus required fields defined as follows: tuple count is sum of tuple
counts of all replacement commands.  For an INSERT, if the replacement
commands taken together inserted a grand total of exactly one tuple,
return that tuple's OID; else return 0.

This is not completely consistent in pathological cases: you could get
a tuple OID returned even when the returned tuple count is greater
than one, which is not a possible case currently.  (This would happen
given a rewrite consisting of a single-row INSERT plus additional
update or delete actions that affect some rows.)  But that seems
pretty oddball.  In all the simple cases I think this proposal gives
reasonable behavior.

A tighter definition for case B would use the sum of the tuple counts
of only the replacement actions that are of the same type as the
original command.  This would eliminate the possible inconsistency
between tuple count and insert OID results, and it's arguably saner
than the above proposal: "if it says UPDATE 4, that should mean that
four rows were updated, not that something else happened to four rows".
But it would not meet Michael's concern about using PQcmdTuples to
tell that "something happened".  I could live with either definition.

Thoughts, different proposals, alternative ways of breaking down
the problem?
        regards, tom lane

Jan Wieck <janwieck@yahoo.com> writes:
>     IMHO  the  answer  should  only  be a number if the rewritten
>     querytree list consists of one  query  of  the  same  command
>     type.  everything else has to lead into "unknown".

I think you can easily generalize that to the statement that the
result should be the sum of the rewritten operations of the same
type as the original query; requiring there to be exactly one
seems overly restrictive.

Michael seems to feel that the tuple count should be nonzero if any
of the replacement operations did anything at all.  This does not make
a lot of sense at the command tag level ("UPDATE 4" might not mean
that 4 tuples were updated) but if you look at the definition of
PQcmdTuples ("returns the number of rows affected by the SQL command")
it's not so unreasonable.  And I can see the point of wanting to
know whether anything happened.
        regards, tom lane

Hannu Krosing <hannu@tm.ee> writes:
> The problem seems to be that recent changes broke updatable views for a
> few users. So have these basic options:

> 1. revert the changes until we have a consensus on doing the right thing
>    (seems best to me)

Reverting is not an option, unless you want to also revert 7.2's change
of execution order of ON INSERT rules; which I would resist as the new
behavior is clearly better.  But given that, both 7.2 and 7.2.1 have
command-tag behavior that is making users unhappy ... in different ways.

I think we should first concentrate on defining what we think the right
behavior should be in the long term.  Only after we know that can we
devise a plan for getting there.  I believe all the concrete suggestions
that have been made so far could be implemented straight away in 7.2.2
(if there is a 7.2.2) ... but we might settle on something that
represents a bigger change with more backwards-compatibility problems.
        regards, tom lane

Jan Wieck wrote:
> 
> Hiroshi Inoue wrote:
> > Tom Lane wrote:
> > >
> >
> > What should the backends return for complicated rewrites ?
> > And how should/could clients handle the results ?
> > It doesn't seem easy to me and it seems a flaw of rule
> > system. Honestly I don't think that the psqlodbc driver
> > can guarantee to handle such cases properly.
> > However both Ron's case and Michael's one are ordinary
> > updatable views. If we can't handle the case properly,
> > we could never recommend users to use (updatable) views.
> 
>     The  fact  that our rule system is that powerful that you can
>     have multi-action rules is a flaw ... awe.

There's always a plus and a minus.
For generic applications the powerfulness is
a nuisance in a sense because it is difficult
for them to understand the intension of 
complicated rewrites( and triggers as you
pointed out). 
I don't think every application can handle
every case. The main point may be how the
applications can judge if they can handle
individual cases.

regards,
Hiroshi Inouehttp://w2422.nsk.ne.jp/~inoue/

Tom Lane <tgl@sss.pgh.pa.us> writes:
> Michael seems to feel that the tuple count should be nonzero if any
> of the replacement operations did anything at all.  This does not
> make a lot of sense at the command tag level ("UPDATE 4" might not
> mean that 4 tuples were updated) but if you look at the definition
> of PQcmdTuples ("returns the number of rows affected by the SQL
> command") it's not so unreasonable.  And I can see the point of
> wanting to know whether anything happened.

Close.

It's not so much that I want to know exactly what happened, it's that
I want to know that if PostgreSQL says nothing happened, then I can be
sure that nothing happened, rather than being told that nothing
happened when something happened, and vice versa.

In fact, my suggestion---which might suffer from issues that I am not
aware of, perhaps the ones that led to the patch in the first
place---would be that, given ambiguity, have the system return
something that would cause PQcmdTuples to return an empty string (I'm
assuing this would be a result string with no numbers attached at
all).

It is documented, after all, as being the return value when the system
cannot determine an otherwise correct number, and all of the code I
looked at would, I believe, cope gracefully with it, returning what
I'm guessing (except in the Perl case, where I'm sure) is a sentinel
value indicating, "it worked, but I have no idea how many tuples were
involved".

But I'm not wedded to that---I just don't want to get an answer back
that might lead me off into the woods.

As for the issue of whether the tag is the same or not, I am utterly
pragmatic---I don't use it, and don't really have a way to get to it
from the interfaces I use, so I think the best option is probably
something where the rules to describe it are straightforward to
minimize confusion and support issues.  And it should be documented
appropriately.

I mean, even when this is resolved, we should probably be putting
something in the documentation that says that PQcmdTuples can really
only really be depended upon as a tri-state value: 0 ("nothing
happened"), >0 ("something happened"), empty string ("heck if I
know").

Mike.

"Zeugswetter Andreas SB SD" <ZeugswetterA@spardat.at> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> Michael seems to feel that the tuple count should be nonzero if any
>> of the replacement operations did anything at all.

> Here we usually add triggers, for replication, accounting, setting of 
> calculated rows ... In all of our cases we want the addition of a trigger
> (or rule on a table) to be transparent to the client.

Yeah.  Triggers wouldn't affect this anyway, unless they tell the system
to suppress insertion/update/deletion of some tuples, in which case I
think it is correct not to count those tuples (certainly that's how the
code has always acted).  As far as rules go, the last proposal that I
made would return the tuple count of the original query as long as there
were no INSTEAD rules --- if you have only actions *added* by rules then
they are transparent.

The hard case is where the original query is not executed because of an
INSTEAD rule.  As the code presently stands, you get "UPDATE 0" (or
INSERT or DELETE 0) in that case, regardless of what else was done
instead by the rule.  I thought that was OK when we put the change in,
but it seems clear that people do not like that behavior.  The notion
of "keep it transparent" doesn't seem to help here.
        regards, tom lane

On Fri, 10 May 2002 10:51:05 -0400, Tom Lane <tgl@sss.pgh.pa.us>
wrote:
>Thoughts, different proposals, alternative ways of breaking down
>the problem?
Well, you asked for it, so here is my wishlist :-)

From a user POV I expect a command to return the number of "rows" it
has processed successfully.  By "rows" I mean rows of the table (or
view or whatever) my command (seemingly) handles,  I'd not be
interested in any side effects my command has because of triggers
and/or rules.

Suppose there is a user called Al B.  If, for example, his DB designer
gives him a table foo (id int, name text) to store his data, he may
consider this table as a black box.  Al does not want to (and probably
even should not) know about rules and triggers.  So when he entersINSERT INTO foo VALUES (10, 'ten');
he expects to getINSERT nnn 1
or an error message.  He doesn't care for any INSERTs into changelogs
or UPDATEs to accounting data, he just wants to know whether *his*
INSERT was successful.

Next, if Al entersINSERT INTO foo SELECT ... FROM bar WHERE ...
and the SELECT statement returns 47 rows, he expectsINSERT 0 47
if there is no problem.
UPDATE foo ... WHERE ...
Here the WHERE clause identifies a certain number of rows which are to
be updated.  Again this number should be returned as the tuple count.
Same for DELETE.

>A. If original command is executed (no INSTEAD), return its tag as-is,
>regardless of commands added by rules.
Yes, please.  This is fully compatible with my wishes.

>B. If original command is not executed, then return its tag name
Agreed.

>plus required fields defined as follows: tuple count is sum of tuple
>counts of all replacement commands.
No, please don't care about replacement commands.  If a rule can be
viewed as something that is executed "for each row", then simply let
"each row" that is processed successfully contribute 1 to the tuple
count.  (Well, I know, this is not always easy.  I guess it's easier
for INSERT and harder for UPDATE and DELETE.  But isn't it a nice
goal?)

While I'm fairly sure about my preferences up to here, there are some
points I don't have a strong opinion on:

OIDs:  With an ordinary table the OID returned by INSERT can be used
to retrieve the new row with SELECT ... WHERE oid=nnn.  Ideally this
would hold for tables and views with rules, but there is no easy way
for the backend to know the correct OID, when there are more than 1
INSERT statements in the rule.  So here's one more idea for your
sub-case 2c:  Let the programmer specify which OID to return, maybe by
an extension to the INSERT syntax, allowed only in rules:INSERT INTO ... VALUES (...) RETURNING OID ???

DO INSTEAD NOTHING:  Should this be considered successful execution or
should it contribute 0 to the tuple count?  I don't know which one is
less surprising.  I tend to the latter.

Just my 0.02.
ServusManfred

Has this been resolved and patched?

---------------------------------------------------------------------------

Tom Lane wrote:
> "Zeugswetter Andreas SB SD" <ZeugswetterA@spardat.at> writes:
> > Tom Lane <tgl@sss.pgh.pa.us> writes:
> >> Michael seems to feel that the tuple count should be nonzero if any
> >> of the replacement operations did anything at all.
> 
> > Here we usually add triggers, for replication, accounting, setting of 
> > calculated rows ... In all of our cases we want the addition of a trigger
> > (or rule on a table) to be transparent to the client.
> 
> Yeah.  Triggers wouldn't affect this anyway, unless they tell the system
> to suppress insertion/update/deletion of some tuples, in which case I
> think it is correct not to count those tuples (certainly that's how the
> code has always acted).  As far as rules go, the last proposal that I
> made would return the tuple count of the original query as long as there
> were no INSTEAD rules --- if you have only actions *added* by rules then
> they are transparent.
> 
> The hard case is where the original query is not executed because of an
> INSTEAD rule.  As the code presently stands, you get "UPDATE 0" (or
> INSERT or DELETE 0) in that case, regardless of what else was done
> instead by the rule.  I thought that was OK when we put the change in,
> but it seems clear that people do not like that behavior.  The notion
> of "keep it transparent" doesn't seem to help here.
> 
>             regards, tom lane
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Don't 'kill -9' the postmaster
> 

--  Bruce Momjian                        |  http://candle.pha.pa.us pgman@candle.pha.pa.us               |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026
 

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Has this been resolved and patched?

No, I think we were still debating how it should work when the
discussion died off...
        regards, tom lane

Any chance we can resolve this before 7.3?  I will add it to the TODO
list.


---------------------------------------------------------------------------

Jan Wieck wrote:
> Tom Lane wrote:
> > Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> > > Of cource it is nice to have a complete solution
> > > immediately but it doesn't seem easy. My patch is
> > > only a makeshift solution but fixes the most
> > > siginificant case(typical updatable views).
> >
> > I would like to devise a complete solution *before* we consider
> > installing makeshift solutions (which will institutionalize wrong
> > behavior).
> >
> > There seems to be some feeling here that in the presence of rewrites
> > you only want to know that "something happened".  Are you suggesting
> > that the returned tuple count should be the sum of all counts from
> > insert, update, and delete actions that happened as a result of the
> > query?  We could certainly implement that, but it does not seem like
> > a good idea to me.
> 
>     IMHO  the  answer  should  only  be a number if the rewritten
>     querytree list consists of one  query  of  the  same  command
>     type.  everything else has to lead into "unknown".
> 
> 
> Jan
> 
> --
> 
> #======================================================================#
> # It's easier to get forgiveness for being wrong than for being right. #
> # Let's break this rule - forgive me.                                  #
> #================================================== JanWieck@Yahoo.com #
> 
> 
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 3: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo@postgresql.org so that your
> message can get through to the mailing list cleanly
> 

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

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Any chance we can resolve this before 7.3?

I don't think so; the discussion trailed off without any agreement on
what the behavior should be, and so thinking about how to implement it
seems premature.  At this point I think we have more critical issues
to focus on for 7.3 ...
        regards, tom lane