Re: Re: proposal and patch : support INSERT INTO...RETURNING with partitioned table using rule

First,  apologies for taking so long to reply to your post.

On Fri, 22 Jun 2012 09:55:13, Robert Haas wrote:

> On Wed, Jun 20, 2012 at 12:24 PM, John Lumby <johnlumby(at)hotmail(dot)com> wrote:
> >     An INSERT which has a RETURNING clause and which is to be rewritten based on
> >     a rule will be accepted if the rule is an "unconditional DO INSTEAD".
> >     In general I believe "unconditional" means "no WHERE clause", but in practice
> >     if the rule is of the form
> >        CREATE RULE insert_part_history as ON INSERT to history \
> >          DO INSTEAD SELECT history_insert_partitioned(NEW) returning NEW.id
> >     this is treated as conditional and the query is rejected.
>
> This isn't rejected because the query is treated as condition; it's
> rejected because it's not valid syntax.  A SELECT query can't have a
> RETURNING clause, because the target list (i.e. the part that
> immediately follows the SELECT) already serves that purpose. The fact
> that it's in a CREATE RULE statement is irrelevant.

Thanks for correcting me.   At the time,  it wasn't clear to me whether the RETURNING clause
in a CREATE RULE statement belonged to the CREATE RULE statement or the rule being created,
but now I see it's the latter.

> >   .  I propose to extend the rule system to recognize cases where the INSERT query specifies
> >      RETURNING and the rule promises to return a row,  and to then permit this query to run
> >      and return the expected row.   In effect,  to widen the definition of "unconditional"
> >      to handle cases such as my testcase.
>
> That already (kind of) works:
>
>  [...]
>
> I do notice that the RETURNING clause of the INSERT can't reference
> NEW, which seems like a restriction that we probably ought to lift,
> but it doesn't seem to have much to do with your patch.
>

The main use of my proposal is to be able to return the value of the sequence assigned
to the NEW.id column,  so yes   that is a serious restriction.

However,   even if that restriction is lifted,  it will not help with the case where
the rule is an invocation of a function,   which is the case I need.    For example,
in my testcase,  the function is building the SQL statement to be executed including the
table name of the partitioned child table,   based on the timestamp in the NEW record.

Your comments have helped me realize that my original subject line did not accurately state
my requirement,  which should read
     "support INSERT INTO...RETURNING with partitioned table using rule which invokes a function"

And for this requirement as re-stated,  as far as I can tell,   current postgresql has no solution:
  .  only way to invoke a function in a rewrite rule is with SELECT function()
  .  SELECT does not permit RETURNING clause
  .  my proposal provides a way of relaxing this restriction for the case of SELECT in a rewrite rule.

I hope this describes the proposal a bit better.

John

John Lumby <johnlumby(at)hotmail(dot)com> writes:
> On Fri, 22 Jun 2012 09:55:13, Robert Haas wrote:
>> I do notice that the RETURNING clause of the INSERT can't reference
>> NEW, which seems like a restriction that we probably ought to lift,
>> but it doesn't seem to have much to do with your patch.

> The main use of my proposal is to be able to return the value of the
> sequence assigned to the NEW.id column, so yes that is a serious
> restriction.

I think both of you are confused. What the RETURNING clause can see is
the inserted row's actual values. You can certainly get the assigned
sequence ID out of that. I would argue that being able to see the NEW.*
expressions is at best secondary, because that data doesn't necessarily
have anything to do with what went into the table (consider the
possibility that a BEFORE trigger changed it).

> However, even if that restriction is lifted, it will not help with the
> case where the rule is an invocation of a function, which is the case
> I need.

What you're requesting seems pretty much nonsensical to me. The point
of being able to write a RETURNING clause in a rule is to emulate what
would happen with RETURNING on a regular table. As an example, suppose
that I have

create table t (id serial, data1 text, data2 text);

and for whatever reason I write

insert into t(data1, data2) values('foo', 'bar') returning id, data2;

I should get back the generated sequence value and the data2 value, but
*not* the data1 value. Anything else is just wrong. Now, if t has a
rule "ON INSERT DO INSTEAD SELECT somefunction()", how is that going to
happen? The function doesn't know what the RETURNING clause looks like.
If we had a notional inserted-row-value then the executor could do the
RETURNING computation based on that, but there's no way to make a
connection between whatever the function does internally and the data
for RETURNING to chew on.

The whole concept of ON INSERT DO [INSTEAD/ALSO] SELECT seems pretty
shaky to me, as it *necessarily* involves a command substitution that
causes an INSERT to act in a strange fashion that the client application
will need special code to cope with. I won't argue to take the feature
out, because people do use it in custom applications --- but it doesn't
play nice with RETURNING, and I don't think it can be made to. It's
pretty much a legacy method of doing business IMO.

It seems to me that instead of lobbying to throw another kluge on top
of that pile, you'd be better off looking for alternative solutions.
Have you tried implementing this as an INSTEAD OF trigger, and not using
rules at all? That mechanism works just fine with RETURNING, and it
seems to me that it would let you do whatever you could do inside a
custom function. It would certainly be enough for the
dynamic-partition-redirection problem.

regards, tom lane

On 09/20/12 16:34, Tom Lane wrote:
> John Lumby <johnlumby(at)hotmail(dot)com> writes:
>> On Fri, 22 Jun 2012 09:55:13, Robert Haas wrote:
>>> I do notice that the RETURNING clause of the INSERT can't reference
>>> NEW, which seems like a restriction that we probably ought to lift,
>>> but it doesn't seem to have much to do with your patch.
>> The main use of my proposal is to be able to return the value of the
>> sequence assigned to the NEW.id column, so yes that is a serious
>> restriction.
> I think both of you are confused. What the RETURNING clause can see is
> the inserted row's actual values. You can certainly get the assigned
> sequence ID out of that. I would argue that being able to see the NEW.*
> expressions is at best secondary, because that data doesn't necessarily
> have anything to do with what went into the table (consider the
> possibility that a BEFORE trigger changed it).

I think this part of the discussion was a bit of a
(probably confused) red herring going off on a tangent.

>> However, even if that restriction is lifted, it will not help with the
>> case where the rule is an invocation of a function, which is the case
>> I need.
> What you're requesting seems pretty much nonsensical to me. The point
> of being able to write a RETURNING clause in a rule is to emulate what
> would happen with RETURNING on a regular table. As an example, suppose
> that I have
>
> create table t (id serial, data1 text, data2 text);
>
> and for whatever reason I write
>
> insert into t(data1, data2) values('foo', 'bar') returning id, data2;
>
> I should get back the generated sequence value and the data2 value, but
> *not* the data1 value. Anything else is just wrong. Now, if t has a
> rule "ON INSERT DO INSTEAD SELECT somefunction()", how is that going to
> happen? The function doesn't know what the RETURNING clause looks like.
> If we had a notional inserted-row-value then the executor could do the
> RETURNING computation based on that, but there's no way to make a
> connection between whatever the function does internally and the data
> for RETURNING to chew on.

Well since you raise the question -- surely the function could return
a tuple of the correct row type and the executor could then pick out
whatever the actual statement requested. This actually seems to
make my proposal more general and useful. And answers the point
you make about "doesn't play nice with RETURNING" in your next para.

>
> The whole concept of ON INSERT DO [INSTEAD/ALSO] SELECT seems pretty
> shaky to me, as it *necessarily* involves a command substitution that
> causes an INSERT to act in a strange fashion that the client application
> will need special code to cope with. I won't argue to take the feature
> out, because people do use it in custom applications --- but it doesn't
> play nice with RETURNING, and I don't think it can be made to. It's
> pretty much a legacy method of doing business IMO.
>
> It seems to me that instead of lobbying to throw another kluge on top
> of that pile, you'd be better off looking for alternative solutions.
> Have you tried implementing this as an INSTEAD OF trigger, and not using
> rules at all? That mechanism works just fine with RETURNING, and it
> seems to me that it would let you do whatever you could do inside a
> custom function. It would certainly be enough for the
> dynamic-partition-redirection problem.

It took me a little while to realize your implicit suggestion that
I should rename my inheritance-parent (true name 'history')
as 'something_else' and then
CREATE VIEW history as select * from something_else
amd then create the instead trigger on the view.
(This *is* what you are suggesting, right?)
I tried t and yes indeed it does exactly what I want -
for the INSERT. Now I also have to define instead triggers
for update and delete. And are there any other considerations
for changing the table into a view? I mean, any other ways in which
SQL or client interfaces could perceive some difference?

Anyhow, yes, this does indeed serve as a solution to the problem
without needing any kluges or hacks, so thank you.
But it gives me (and anyone else who tries it) more work than
one simple RULE on the table without needing to add the view.
By the way - what is the reason for the restiction
that INSTEAD OF triggers cannot be defined on "real" tables,
only on views? Could this be lifted?

John Lumby

>
> regards, tom lane
>
>

johnlumby escribió:
> On 09/20/12 16:34, Tom Lane wrote:

> >It seems to me that instead of lobbying to throw another kluge on top
> >of that pile, you'd be better off looking for alternative solutions.
> >Have you tried implementing this as an INSTEAD OF trigger, and not using
> >rules at all? That mechanism works just fine with RETURNING, and it
> >seems to me that it would let you do whatever you could do inside a
> >custom function. It would certainly be enough for the
> >dynamic-partition-redirection problem.

> Anyhow, yes, this does indeed serve as a solution to the problem
> without needing any kluges or hacks, so thank you.
> But it gives me (and anyone else who tries it) more work than
> one simple RULE on the table without needing to add the view.
> By the way - what is the reason for the restiction
> that INSTEAD OF triggers cannot be defined on "real" tables,
> only on views? Could this be lifted?

I read this as meaning that this patch is not necessary. You seem to be
saying that you'd like some other things patched in some other ways, but
they are unrelated to the original patch. If you are able to figure out
what you want to change and how, please submit another proposal and patch.

I'm marking this one "rejected" in the current commitfest. Thanks.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services