Re: Queries using rules show no rows modified? - Mailing list pgsql-hackers

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