Re: BUG #6572: The example of SPI_execute is bogus

The following bug has been logged on the website:

Bug reference: 6572
Logged by: Hitoshi Harada
Email address: umi(dot)tanuki(at)gmail(dot)com
PostgreSQL version: 9.1.3
Operating system: Any
Description:

http://www.postgresql.org/docs/9.1/static/spi-spi-execute.html

===
SPI_execute("INSERT INTO foo SELECT * FROM bar", false, 5);

will allow at most 5 rows to be inserted into the table.
===

This seems not true unless I'm missing something.

umi(dot)tanuki(at)gmail(dot)com writes:
> http://www.postgresql.org/docs/9.1/static/spi-spi-execute.html

> ===
> SPI_execute("INSERT INTO foo SELECT * FROM bar", false, 5);
> will allow at most 5 rows to be inserted into the table.
> ===

> This seems not true unless I'm missing something.

Hmm ... that did work as described, until we broke it :-(. This is an
oversight in the 9.0 changes that added separate ModifyTuple nodes to
plan trees. ModifyTuple doesn't return after each updated row, unless
there's a RETURNING clause; which means that the current_tuple_count
check logic in ExecutePlan() no longer stops execution as intended.

Given the lack of complaints since 9.0, maybe we should not fix this
but just redefine the new behavior as being correct? But it seems
mighty inconsistent that the tuple limit would apply if you have
RETURNING but not when you don't. In any case, the ramifications
are wider than one example in the SPI docs.

Thoughts?

regards, tom lane

On Wed, Apr 4, 2012 at 8:00 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> umi(dot)tanuki(at)gmail(dot)com writes:
>> http://www.postgresql.org/docs/9.1/static/spi-spi-execute.html
>
>> ===
>> SPI_execute("INSERT INTO foo SELECT * FROM bar", false, 5);
>> will allow at most 5 rows to be inserted into the table.
>> ===
>
>> This seems not true unless I'm missing something.
>
> Hmm ... that did work as described, until we broke it :-(.  This is an
> oversight in the 9.0 changes that added separate ModifyTuple nodes to
> plan trees.  ModifyTuple doesn't return after each updated row, unless
> there's a RETURNING clause; which means that the current_tuple_count
> check logic in ExecutePlan() no longer stops execution as intended.
>
> Given the lack of complaints since 9.0, maybe we should not fix this
> but just redefine the new behavior as being correct?  But it seems
> mighty inconsistent that the tuple limit would apply if you have
> RETURNING but not when you don't.  In any case, the ramifications
> are wider than one example in the SPI docs.
>
> Thoughts?

To be honest, I was surprised when I found tcount parameter is said to
be applied to even INSERT. I believe people think that parameter is
to limit memory consumption when returning tuples thus it'd be applied
for only SELECT or DML with RETURNING. So I'm +1 for non-fix but
redefine the behavior. Who wants to limit the number of rows
processed inside the backend, from SPI?

Thanks,
--
Hitoshi Harada

On Thu, Apr 5, 2012 at 2:39 AM, Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com> wrote:
> On Wed, Apr 4, 2012 at 8:00 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> umi(dot)tanuki(at)gmail(dot)com writes:
>>> http://www.postgresql.org/docs/9.1/static/spi-spi-execute.html
>>
>>> ===
>>> SPI_execute("INSERT INTO foo SELECT * FROM bar", false, 5);
>>> will allow at most 5 rows to be inserted into the table.
>>> ===
>>
>>> This seems not true unless I'm missing something.
>>
>> Hmm ... that did work as described, until we broke it :-(.  This is an
>> oversight in the 9.0 changes that added separate ModifyTuple nodes to
>> plan trees.  ModifyTuple doesn't return after each updated row, unless
>> there's a RETURNING clause; which means that the current_tuple_count
>> check logic in ExecutePlan() no longer stops execution as intended.
>>
>> Given the lack of complaints since 9.0, maybe we should not fix this
>> but just redefine the new behavior as being correct?  But it seems
>> mighty inconsistent that the tuple limit would apply if you have
>> RETURNING but not when you don't.  In any case, the ramifications
>> are wider than one example in the SPI docs.
>>
>> Thoughts?
>
> To be honest, I was surprised when I found tcount parameter is said to
> be applied to even INSERT.  I believe people think that parameter is
> to limit memory consumption when returning tuples thus it'd be applied
> for only SELECT or DML with RETURNING.  So I'm +1 for non-fix but
> redefine the behavior.  Who wants to limit the number of rows
> processed inside the backend, from SPI?

Yeah. I think it would be a good idea for UPDATE and DELETE to expose
a LIMIT option, but I can't really see the virtue in making that
functionality available only through SPI.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

>
> Yeah.  I think it would be a good idea for UPDATE and DELETE to expose
> a LIMIT option, but I can't really see the virtue in making that
> functionality available only through SPI.
>

I don't agree - LIMIT after UPDATE or DELETE has no sense. Clean
solution should be based on using updateable CTE.

Regards

Pavel

On Fri, Apr 13, 2012 at 10:43 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> Yeah.  I think it would be a good idea for UPDATE and DELETE to expose
>> a LIMIT option, but I can't really see the virtue in making that
>> functionality available only through SPI.
>
> I don't agree - LIMIT after UPDATE or DELETE has no sense. Clean
> solution should be based on using updateable CTE.

It has a lot of sense. Without it, it's very difficult to do logical
replication on a table with no primary key.

(Whether or not people should create such tables in the first place
is, of course, beside the point.)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

2012/4/14 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Fri, Apr 13, 2012 at 10:43 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>> Yeah.  I think it would be a good idea for UPDATE and DELETE to expose
>>> a LIMIT option, but I can't really see the virtue in making that
>>> functionality available only through SPI.
>>
>> I don't agree - LIMIT after UPDATE or DELETE has no sense. Clean
>> solution should be based on using updateable CTE.
>
> It has a lot of sense.  Without it, it's very difficult to do logical
> replication on a table with no primary key.
>
> (Whether or not people should create such tables in the first place
> is, of course, beside the point.)

I am not against to functionality - I am against just to syntax DELETE
FROM tab LIMIT x

because is it ambiguous what means: DELETE FROM tab RETURNING * LIMIT x

Regards

Pavel

>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company

On Sat, Apr 14, 2012 at 3:27 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> It has a lot of sense.  Without it, it's very difficult to do logical
>> replication on a table with no primary key.
>>
>> (Whether or not people should create such tables in the first place
>> is, of course, beside the point.)
>
> I am not against to functionality - I am against just to syntax DELETE
> FROM tab LIMIT x
>
> because is it ambiguous what means: DELETE FROM tab RETURNING * LIMIT x

What's ambiguous about that?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On lör, 2012-04-14 at 08:23 -0400, Robert Haas wrote:
> On Sat, Apr 14, 2012 at 3:27 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> >> It has a lot of sense. Without it, it's very difficult to do logical
> >> replication on a table with no primary key.
> >>
> >> (Whether or not people should create such tables in the first place
> >> is, of course, beside the point.)
> >
> > I am not against to functionality - I am against just to syntax DELETE
> > FROM tab LIMIT x
> >
> > because is it ambiguous what means: DELETE FROM tab RETURNING * LIMIT x
>
> What's ambiguous about that?

I suppose one could wonder whether the LIMIT applies to the deleting or
just the returning.

2012/4/14 Peter Eisentraut <peter_e(at)gmx(dot)net>:
> On lör, 2012-04-14 at 08:23 -0400, Robert Haas wrote:
>> On Sat, Apr 14, 2012 at 3:27 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> >> It has a lot of sense.  Without it, it's very difficult to do logical
>> >> replication on a table with no primary key.
>> >>
>> >> (Whether or not people should create such tables in the first place
>> >> is, of course, beside the point.)
>> >
>> > I am not against to functionality - I am against just to syntax DELETE
>> > FROM tab LIMIT x
>> >
>> > because is it ambiguous what means: DELETE FROM tab RETURNING * LIMIT x
>>
>> What's ambiguous about that?
>
> I suppose one could wonder whether the LIMIT applies to the deleting or
> just the returning.
>

yes, exactly

Regards

Pavel

On Sat, Apr 14, 2012 at 12:15 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On lör, 2012-04-14 at 08:23 -0400, Robert Haas wrote:
>> On Sat, Apr 14, 2012 at 3:27 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> >> It has a lot of sense.  Without it, it's very difficult to do logical
>> >> replication on a table with no primary key.
>> >>
>> >> (Whether or not people should create such tables in the first place
>> >> is, of course, beside the point.)
>> >
>> > I am not against to functionality - I am against just to syntax DELETE
>> > FROM tab LIMIT x
>> >
>> > because is it ambiguous what means: DELETE FROM tab RETURNING * LIMIT x
>>
>> What's ambiguous about that?
>
> I suppose one could wonder whether the LIMIT applies to the deleting or
> just the returning.

I suppose. I had in mind it would apply to both.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

2012-04-14 18:15 keltezéssel, Peter Eisentraut írta:
> On lör, 2012-04-14 at 08:23 -0400, Robert Haas wrote:
>> On Sat, Apr 14, 2012 at 3:27 AM, Pavel Stehule<pavel(dot)stehule(at)gmail(dot)com> wrote:
>>>> It has a lot of sense. Without it, it's very difficult to do logical
>>>> replication on a table with no primary key.
>>>>
>>>> (Whether or not people should create such tables in the first place
>>>> is, of course, beside the point.)
>>> I am not against to functionality - I am against just to syntax DELETE
>>> FROM tab LIMIT x
>>>
>>> because is it ambiguous what means: DELETE FROM tab RETURNING * LIMIT x
>> What's ambiguous about that?
> I suppose one could wonder whether the LIMIT applies to the deleting or
> just the returning.

Ambigous only in this order. LIMIT x RETURNING * wouldn't be.

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig& Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/

2012/4/15 Boszormenyi Zoltan <zb(at)cybertec(dot)at>:
> 2012-04-14 18:15 keltezéssel, Peter Eisentraut írta:
>
>> On lör, 2012-04-14 at 08:23 -0400, Robert Haas wrote:
>>>
>>> On Sat, Apr 14, 2012 at 3:27 AM, Pavel Stehule<pavel(dot)stehule(at)gmail(dot)com>
>>>  wrote:
>>>>>
>>>>> It has a lot of sense.  Without it, it's very difficult to do logical
>>>>> replication on a table with no primary key.
>>>>>
>>>>> (Whether or not people should create such tables in the first place
>>>>> is, of course, beside the point.)
>>>>
>>>> I am not against to functionality - I am against just to syntax DELETE
>>>> FROM tab LIMIT x
>>>>
>>>> because is it ambiguous what means: DELETE FROM tab RETURNING * LIMIT x
>>>
>>> What's ambiguous about that?
>>
>> I suppose one could wonder whether the LIMIT applies to the deleting or
>> just the returning.
>
>
> Ambigous only in this order. LIMIT x RETURNING * wouldn't be.

but theoretically you can has two LIMIT clauses in one SQL statements

DELETE FROM tab LIMIT n RETURNING * LIMIT m

without updatable CTE it is probably only one solution, but because we
have UCTE, then we don't need this construct.

Regards

Pavel

>
> --
> ----------------------------------
> Zoltán Böszörményi
> Cybertec Schönig&  Schönig GmbH
> Gröhrmühlgasse 26
> A-2700 Wiener Neustadt, Austria
> Web: http://www.postgresql-support.de
>     http://www.postgresql.at/
>

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Apr 5, 2012 at 2:39 AM, Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com> wrote:
>> On Wed, Apr 4, 2012 at 8:00 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Given the lack of complaints since 9.0, maybe we should not fix this
>>> but just redefine the new behavior as being correct? But it seems
>>> mighty inconsistent that the tuple limit would apply if you have
>>> RETURNING but not when you don't. In any case, the ramifications
>>> are wider than one example in the SPI docs.

>> To be honest, I was surprised when I found tcount parameter is said to
>> be applied to even INSERT. I believe people think that parameter is
>> to limit memory consumption when returning tuples thus it'd be applied
>> for only SELECT or DML with RETURNING. So I'm +1 for non-fix but
>> redefine the behavior. Who wants to limit the number of rows
>> processed inside the backend, from SPI?

> Yeah.

Okay, apparently nobody cares about RETURNING behaving differently from
non-RETURNING, so the consensus is to redefine the current behavior as
correct. That means what we need is to go through the docs and see what
places need to be updated (and, I guess, back-patch the changes to 9.0).
I will get to this if nobody else does, but not right away.

> I think it would be a good idea for UPDATE and DELETE to expose
> a LIMIT option, but I can't really see the virtue in making that
> functionality available only through SPI.

FWIW, I'm not excited about that. You can get well-defined behavior
today from a SELECT/LIMIT drawing from a writable CTE (namely, that
the UPDATE/DELETE runs to completion but you only see a subset of
its RETURNING result). LIMIT directly on the UPDATE/DELETE would be
ill-defined, unless perhaps you want to also invent a way of specifying
the order in which rows get selected for update; but I don't want to
go there.

regards, tom lane

On Sun, Apr 15, 2012 at 12:29 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I think it would be a good idea for UPDATE and DELETE to expose
>> a LIMIT option, but I can't really see the virtue in making that
>> functionality available only through SPI.
>
> FWIW, I'm not excited about that.  You can get well-defined behavior
> today from a SELECT/LIMIT drawing from a writable CTE (namely, that
> the UPDATE/DELETE runs to completion but you only see a subset of
> its RETURNING result).  LIMIT directly on the UPDATE/DELETE would be
> ill-defined, unless perhaps you want to also invent a way of specifying
> the order in which rows get selected for update; but I don't want to
> go there.

In the use cases I'm thinking of, it doesn't matter which row you
decide to update or delete, only that you pick a single one.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sun, Apr 15, 2012 at 12:29:39PM -0400, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Thu, Apr 5, 2012 at 2:39 AM, Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com> wrote:
> >> On Wed, Apr 4, 2012 at 8:00 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >>> Given the lack of complaints since 9.0, maybe we should not fix this
> >>> but just redefine the new behavior as being correct? But it seems
> >>> mighty inconsistent that the tuple limit would apply if you have
> >>> RETURNING but not when you don't. In any case, the ramifications
> >>> are wider than one example in the SPI docs.
>
> >> To be honest, I was surprised when I found tcount parameter is said to
> >> be applied to even INSERT. I believe people think that parameter is
> >> to limit memory consumption when returning tuples thus it'd be applied
> >> for only SELECT or DML with RETURNING. So I'm +1 for non-fix but
> >> redefine the behavior. Who wants to limit the number of rows
> >> processed inside the backend, from SPI?
>
> > Yeah.
>
> Okay, apparently nobody cares about RETURNING behaving differently from
> non-RETURNING, so the consensus is to redefine the current behavior as
> correct. That means what we need is to go through the docs and see what
> places need to be updated (and, I guess, back-patch the changes to 9.0).
> I will get to this if nobody else does, but not right away.

Would someone make the doc change outlined above? Thanks.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

________________________________________
From: pgsql-bugs-owner(at)postgresql(dot)org [pgsql-bugs-owner(at)postgresql(dot)org] on behalf of Bruce Momjian [bruce(at)momjian(dot)us]
Sent: Wednesday, August 29, 2012 8:46 AM
To: Tom Lane
Cc: Robert Haas; Hitoshi Harada; pgsql-bugs(at)postgresql(dot)org; pgsql-hackers(at)postgresql(dot)org
Subject: Re: [HACKERS] [BUGS] BUG #6572: The example of SPI_execute is bogus

On Sun, Apr 15, 2012 at 12:29:39PM -0400, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Thu, Apr 5, 2012 at 2:39 AM, Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com> wrote:
> >> On Wed, Apr 4, 2012 at 8:00 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >>> Given the lack of complaints since 9.0, maybe we should not fix this
> >>> but just redefine the new behavior as being correct? But it seems
> >>> mighty inconsistent that the tuple limit would apply if you have
> >>> RETURNING but not when you don't. In any case, the ramifications
> >>> are wider than one example in the SPI docs.
>
> >> To be honest, I was surprised when I found tcount parameter is said to
> >> be applied to even INSERT. I believe people think that parameter is
> >> to limit memory consumption when returning tuples thus it'd be applied
> >> for only SELECT or DML with RETURNING. So I'm +1 for non-fix but
> >> redefine the behavior. Who wants to limit the number of rows
> >> processed inside the backend, from SPI?
>
> > Yeah.
>
> Okay, apparently nobody cares about RETURNING behaving differently from
> non-RETURNING, so the consensus is to redefine the current behavior as
> correct. That means what we need is to go through the docs and see what
> places need to be updated (and, I guess, back-patch the changes to 9.0).
> I will get to this if nobody else does, but not right away.

> Would someone make the doc change outlined above? Thanks.

I would like to work on this documentation bug.
As per analysis I am planning to update following SPI function:
1. SPI_Execute: Here we will mention that argument count is used only for the kind of command which returns result i.e. all kind of SELECT and DML with returning clause. count is ignored for any other kind of commands. I will add one example also to indicate the difference.
2. SPI_execute_plan_with_paramlist: Here we can give just reference to SPI_execute i.e. I will mention that count has same interpretation as in SPI_execute.
3. SPI_execp: Here we can give just reference to SPI_execute i.e. I will mention that count has same interpretation as in SPI_execute.

Please provide your feedback.

Thanks and Regards,
Kumar Rajeev Rastogi
Cell No - +91 8971367787

On Wed, Aug 29, 2012 at 01:13:51PM +0000, Rajeev rastogi wrote:
> ________________________________________
> From: pgsql-bugs-owner(at)postgresql(dot)org [pgsql-bugs-owner(at)postgresql(dot)org] on behalf of Bruce Momjian [bruce(at)momjian(dot)us]
> Sent: Wednesday, August 29, 2012 8:46 AM
> To: Tom Lane
> Cc: Robert Haas; Hitoshi Harada; pgsql-bugs(at)postgresql(dot)org; pgsql-hackers(at)postgresql(dot)org
> Subject: Re: [HACKERS] [BUGS] BUG #6572: The example of SPI_execute is bogus
>
> On Sun, Apr 15, 2012 at 12:29:39PM -0400, Tom Lane wrote:
> > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > > On Thu, Apr 5, 2012 at 2:39 AM, Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com> wrote:
> > >> On Wed, Apr 4, 2012 at 8:00 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > >>> Given the lack of complaints since 9.0, maybe we should not fix this
> > >>> but just redefine the new behavior as being correct? But it seems
> > >>> mighty inconsistent that the tuple limit would apply if you have
> > >>> RETURNING but not when you don't. In any case, the ramifications
> > >>> are wider than one example in the SPI docs.
> >
> > >> To be honest, I was surprised when I found tcount parameter is said to
> > >> be applied to even INSERT. I believe people think that parameter is
> > >> to limit memory consumption when returning tuples thus it'd be applied
> > >> for only SELECT or DML with RETURNING. So I'm +1 for non-fix but
> > >> redefine the behavior. Who wants to limit the number of rows
> > >> processed inside the backend, from SPI?
> >
> > > Yeah.
> >
> > Okay, apparently nobody cares about RETURNING behaving differently from
> > non-RETURNING, so the consensus is to redefine the current behavior as
> > correct. That means what we need is to go through the docs and see what
> > places need to be updated (and, I guess, back-patch the changes to 9.0).
> > I will get to this if nobody else does, but not right away.
>
> > Would someone make the doc change outlined above? Thanks.
>
>
> I would like to work on this documentation bug.
> As per analysis I am planning to update following SPI function:
> 1. SPI_Execute: Here we will mention that argument count is used only for the kind of command which returns result i.e. all kind of SELECT and DML with returning clause. count is ignored for any other kind of commands. I will add one example also to indicate the difference.
> 2. SPI_execute_plan_with_paramlist: Here we can give just reference to SPI_execute i.e. I will mention that count has same interpretation as in SPI_execute.
> 3. SPI_execp: Here we can give just reference to SPI_execute i.e. I will mention that count has same interpretation as in SPI_execute.

Would someone please provide answers to these questions, or write a
patch?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Would someone make the doc change outlined above? Thanks.

Sorry, I'd nearly forgotten about this issue. Will see about fixing the
docs. (It looks like some of the comments in execMain.c could use work
too.)

regards, tom lane

On Thu, Jan 24, 2013 at 04:51:04PM -0500, Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Would someone make the doc change outlined above? Thanks.
>
> Sorry, I'd nearly forgotten about this issue. Will see about fixing the
> docs. (It looks like some of the comments in execMain.c could use work
> too.)

Thanks.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +