Обсуждение: improve FOUND in PL/PgSQL
This patch improves the behavior of FOUND in PL/PgSQL. In Oracle, FOUND is set whenever a SELECT INTO returns > 0 rows, *or* when an INSERT, UPDATE, or DELETE affects > 0 rows. We implemented the first part of this behavior, but not the second. I also improved the documentation on the various situations in which FOUND can be set (excluding inside FOR loops, which I still need to think about), and added some regression tests for this behavior. Cheers, Neil -- Neil Conway <neilconway@rogers.com> PGP Key ID: DB3C29FC
Вложения
Your patch has been added to the PostgreSQL unapplied patches list at:
http://candle.pha.pa.us/cgi-bin/pgpatches
I will try to apply it within the next 48 hours.
---------------------------------------------------------------------------
Neil Conway wrote:
> This patch improves the behavior of FOUND in PL/PgSQL. In Oracle,
> FOUND is set whenever a SELECT INTO returns > 0 rows, *or* when an
> INSERT, UPDATE, or DELETE affects > 0 rows. We implemented the first
> part of this behavior, but not the second.
>
> I also improved the documentation on the various situations in which
> FOUND can be set (excluding inside FOR loops, which I still need to
> think about), and added some regression tests for this behavior.
>
> Cheers,
>
> Neil
>
> --
> Neil Conway <neilconway@rogers.com>
> PGP Key ID: DB3C29FC
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org
--
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: > Your patch has been added to the PostgreSQL unapplied patches list I've attached an updated patch. This patch also fixes some bugs in the setting of FOUND when executing FOR loops, as pointed out by Tom Lane, and documents this capability. I also made the regression test a little more compact. Unfortunately, the code is a bit less elegant than I'd like. If anyone has any suggestions on how to implement these changes in a more concise fashion, I'd be interested. Cheers, Neil -- Neil Conway <neilconway@rogers.com> PGP Key ID: DB3C29FC
Вложения
Neil Conway <nconway@klamath.dyndns.org> writes:
> <para>
> ! There is a special variable named <literal>FOUND</literal> of
> ! type <type>boolean</type>. It will be set to true if:
> ! <itemizedlist>
> ! <listitem>
> ! <para>
> ! A SELECT INTO statement is executed, and it returns one or
> ! more rows.
> ! </para>
> ... etc ...
This is better than what we had, but still seems confusing: it could
easily be read to mean that nothing happens to FOUND if SELECT returns
zero rows (ie, FOUND retains its previous value in that case). Perhaps
more like:
There is a special variable .... It is set by the following types
of statement:
SELECT INTO sets FOUND to true if at least one row is
returned, to false otherwise.
... etc ...
Also the initial state of FOUND should be documented (or is that in the
other patch?)
> Unfortunately, the code is a bit less elegant than I'd like.
Indeed. I don't like duplicating the loop condition test, and really
the behavior is still wrong: IMHO the FOR statements should not touch
FOUND at all before entering the loop. I think you could do this
much more cleanly by introducing a local boolean:
bool found = false;
<< remove existing exec_set_found(false) >>
<< within loop, do found = true at point where existing
code does exec_set_found(true) >>
<< after exiting loop, do exec_set_found(found) >>
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes: > Neil Conway <nconway@klamath.dyndns.org> writes: > > <para> > > ! There is a special variable named <literal>FOUND</literal> of > > ! type <type>boolean</type>. It will be set to true if: > > ! <itemizedlist> > > ! <listitem> > > ! <para> > > ! A SELECT INTO statement is executed, and it returns one or > > ! more rows. > > ! </para> > > ... etc ... > > This is better than what we had, but still seems confusing [...] Ok, I made this part of the documentation more clear. > Also the initial state of FOUND should be documented (or is that in the > other patch?) Nope, I forgot about that. For the moment, I've documented that the initial state of FOUND is false -- if we want to change that to be NULL (to match Oracle's behavior), it can be done later. > > Unfortunately, the code is a bit less elegant than I'd like. > > Indeed. I don't like duplicating the loop condition test, and really > the behavior is still wrong: IMHO the FOR statements should not touch > FOUND at all before entering the loop. I think you could do this > much more cleanly by introducing a local boolean: Ah, ok -- thanks for the suggestion. It required a fair amount of work, since I had to refactor a lot of the logic in the 3 functions that handle PL/PgSQL FOR loops. A revised patch is attached. Cheers, Neil -- Neil Conway <neilconway@rogers.com> PGP Key ID: DB3C29FC
Вложения
Neil Conway <nconway@klamath.dyndns.org> writes:
>> I think you could do this
>> much more cleanly by introducing a local boolean:
> Ah, ok -- thanks for the suggestion. It required a fair amount of
> work, since I had to refactor a lot of the logic in the 3 functions
> that handle PL/PgSQL FOR loops.
Then you're still doing it the hard way: all you need is to do
exec_set_found(found) immediately before anyplace that's going to
return. You don't need to move the returns.
Perhaps the refactoring is worth doing anyway, if it improves the
readability of the code; but if it makes it worse then there's no need.
It's hard to tell about this just from looking at the diff --- what
do you feel about what you did?
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes: > Neil Conway <nconway@klamath.dyndns.org> writes: > > Ah, ok -- thanks for the suggestion. It required a fair amount of > > work, since I had to refactor a lot of the logic in the 3 functions > > that handle PL/PgSQL FOR loops. > > Then you're still doing it the hard way: all you need is to do > exec_set_found(found) immediately before anyplace that's going to > return. You don't need to move the returns. Sure -- but those functions returned in six or seven different places, and I thought that adding identical code to each location would be too ugly. > Perhaps the refactoring is worth doing anyway, if it improves the > readability of the code; but if it makes it worse then there's no need. > It's hard to tell about this just from looking at the diff --- what > do you feel about what you did? Personally, I think my changes improve readability. Cheers, Neil -- Neil Conway <neilconway@rogers.com> PGP Key ID: DB3C29FC
Neil Conway <nconway@klamath.dyndns.org> writes:
>> This is better than what we had, but still seems confusing
> Ok, I made this part of the documentation more clear.
BTW, the text still fails to make it clear that the statements will in
fact reset FOUND to false when they process zero rows (or whatever).
One could easily read this text as saying that FOUND will be the logical
OR of whether all the statements-so-far found a row.
It might also be good to spell out that FOUND is local in each plpgsql
function, not a global variable.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes: > BTW, the text still fails to make it clear that the statements will in > fact reset FOUND to false when they process zero rows (or whatever). > One could easily read this text as saying that FOUND will be the logical > OR of whether all the statements-so-far found a row. > > It might also be good to spell out that FOUND is local in each plpgsql > function, not a global variable. Ok, a revised patch is attached. Thanks for the feedback. Cheers, Neil -- Neil Conway <neilconway@rogers.com> PGP Key ID: DB3C29FC
Вложения
Your patch has been added to the PostgreSQL unapplied patches list at:
http://candle.pha.pa.us/cgi-bin/pgpatches
I will try to apply it within the next 48 hours.
---------------------------------------------------------------------------
Neil Conway wrote:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
> > BTW, the text still fails to make it clear that the statements will in
> > fact reset FOUND to false when they process zero rows (or whatever).
> > One could easily read this text as saying that FOUND will be the logical
> > OR of whether all the statements-so-far found a row.
> >
> > It might also be good to spell out that FOUND is local in each plpgsql
> > function, not a global variable.
>
> Ok, a revised patch is attached. Thanks for the feedback.
>
> Cheers,
>
> Neil
>
> --
> Neil Conway <neilconway@rogers.com>
> PGP Key ID: DB3C29FC
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org
--
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: > Your patch has been added to the PostgreSQL unapplied patches [...] AFAICT this patch hasn't been applied, but the list of outstanding patches is empty. Cheers, Neil -- Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC
I seem to have lost it somehow. Would you please send it to me and I will apply right now? --------------------------------------------------------------------------- Neil Conway wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Your patch has been added to the PostgreSQL unapplied patches [...] > > AFAICT this patch hasn't been applied, but the list of outstanding > patches is empty. > > Cheers, > > Neil > > -- > Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC > > -- 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