Re: CREATE TEMP TABLE AS SELECT/ GET DIAGNOSTICS

Hello,
I was not able to find any traces from the previous discussion trend,
but I believe that finished when I replaced GET DIAGNOSTIC with SELECT COUNT().

Perfectly fine workaround, but more I look at that more I see why GET DIAGNOSTIC was so convenient not to mentioned that SELECT COUNT() is not a fastest
possible statement in PG.

Ideally what I would like are:
1. “Official” word whether that will be supported or not, ether way is fine,
but that will clear confusion for me and others.
2. Maybe some clause in docs clarifying behavior for the case

Best regards.

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Can someone in the community comment on this question? I don't know the
> answer.

I think it could be changed back without much work, but I have a feeling
that we'd deliberately decided on the change of behavior. Can anyone
recall a prior discussion, or want to vote with or against MLikharev?

Note that the change is actually at the SPI level, and would affect
SPI_processed for all code using CREATE AS/SELECT INTO through SPI,
not only plpgsql.

regards, tom lane

I found a discussion of this issue from December, 2004:

http://archives.postgresql.org/pgsql-general/2004-12/msg00070.php

The discussion trailed off with the idea that because no rows were
returned to the function, the row_count should be zero, but then there
was some discussion that FOUND was then inconsistent.

Anyway, perhaps we should read through this and make a final
determination.

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

MLikharev(at)micropat(dot)com wrote:
> Hello,
> I was not able to find any traces from the previous discussion trend,
> but I believe that finished when I replaced GET DIAGNOSTIC with SELECT COUNT().
>
> Perfectly fine workaround, but more I look at that more I see why GET DIAGNOSTIC was so convenient not to mentioned that SELECT COUNT() is not a fastest
> possible statement in PG.
>
> Ideally what I would like are:
> 1. ?Official? word whether that will be supported or not, ether way is fine,
> but that will clear confusion for me and others.
> 2. Maybe some clause in docs clarifying behavior for the case
>
> Best regards.
>
>
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Can someone in the community comment on this question? I don't know the
> > answer.
>
> I think it could be changed back without much work, but I have a feeling
> that we'd deliberately decided on the change of behavior. Can anyone
> recall a prior discussion, or want to vote with or against MLikharev?
>
> Note that the change is actually at the SPI level, and would affect
> SPI_processed for all code using CREATE AS/SELECT INTO through SPI,
> not only plpgsql.
>
> regards, tom lane
>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
>
> http://archives.postgresql.org
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)us> writes:
> I found a discussion of this issue from December, 2004:
> http://archives.postgresql.org/pgsql-general/2004-12/msg00070.php

That was the same complainant ;-)

I dug through the CVS history and determined that the behavior changed
at spi.c rev 1.87:

2003-03-09 22:53 tgl

* Restructure parsetree representation of
DECLARE CURSOR: now it's a utility statement (DeclareCursorStmt)
with a SELECT query dangling from it, rather than a SELECT query
with a few unusual fields in it. Add code to determine whether a
planned query can safely be run backwards. If DECLARE CURSOR
specifies SCROLL, ensure that the plan can be run backwards by
adding a Materialize plan node if it can't. Without SCROLL, you
get an error if you try to fetch backwards from a cursor that can't
handle it. (There is still some discussion about what the exact
behavior should be, but this is necessary infrastructure in any
case.) Along the way, make EXPLAIN DECLARE CURSOR work.

Looking at the code change, it may have just been a sloppy removal of a
local variable, ie checking queryDesc->dest rather than a previously
saved copy of same. The log message certainly doesn't suggest that I
intended to change the behavior of CREATE TABLE AS.

So the initial evidence is that this was not an intentional change.
Do we want to revert it? The behavior has been in the field now for
more than a full release cycle --- all 7.4.* releases behave this way
--- so one could argue that we should leave it be.

regards, tom lane

On Tue, May 31, 2005 at 01:03:30AM -0400, Tom Lane wrote:

> So the initial evidence is that this was not an intentional change.
> Do we want to revert it? The behavior has been in the field now for
> more than a full release cycle --- all 7.4.* releases behave this way
> --- so one could argue that we should leave it be.

Well, probably nobody has complained because nobody uses the behavior.
I think it's wrong to assume that people really depend on this behavior
-- they would be doing something like

SELECT some_columns ...
CREATE TABLE AS SELECT ...
GET DIAGNOSTICS row_count

and expect to get the row_count from the first SELECT rather than CREATE
TABLE AS. I wouldn't expect that, for one. Personally I think it
should be reverted.

One thing: I couldn't find GET DIAGNOSTICS documentation for Oracle's
PL/SQL by googling around. Is this PL/pgSQL-specific stuff?

--
Alvaro Herrera (<alvherre[a]surnet.cl>)
"Hay quien adquiere la mala costumbre de ser infeliz" (M. A. Evans)

Alvaro Herrera <alvherre(at)surnet(dot)cl> writes:
> On Tue, May 31, 2005 at 01:03:30AM -0400, Tom Lane wrote:
>> So the initial evidence is that this was not an intentional change.
>> Do we want to revert it? The behavior has been in the field now for
>> more than a full release cycle --- all 7.4.* releases behave this way
>> --- so one could argue that we should leave it be.

> ... Personally I think it should be reverted.

OK, next question: is this a bug fix we should back-patch into 7.4,
or just change it in HEAD?

> One thing: I couldn't find GET DIAGNOSTICS documentation for Oracle's
> PL/SQL by googling around. Is this PL/pgSQL-specific stuff?

It's in the SQL99 spec, though arguably plpgsql is misusing it.

regards, tom lane

On Tue, May 31, 2005 at 03:43:56PM -0400, Tom Lane wrote:
> Alvaro Herrera <alvherre(at)surnet(dot)cl> writes:
> > On Tue, May 31, 2005 at 01:03:30AM -0400, Tom Lane wrote:
> >> So the initial evidence is that this was not an intentional change.
> >> Do we want to revert it? The behavior has been in the field now for
> >> more than a full release cycle --- all 7.4.* releases behave this way
> >> --- so one could argue that we should leave it be.
>
> > ... Personally I think it should be reverted.
>
> OK, next question: is this a bug fix we should back-patch into 7.4,
> or just change it in HEAD?

I guess apply only in HEAD, and provide the patch for MLikharev so he
can solve his immediate problem.

--
Alvaro Herrera (<alvherre[a]surnet.cl>)
Thou shalt study thy libraries and strive not to reinvent them without
cause, that thy code may be short and readable and thy days pleasant
and productive. (7th Commandment for C Programmers)

On Tue, 2005-05-31 at 15:43 -0400, Tom Lane wrote:
> OK, next question: is this a bug fix we should back-patch into 7.4,
> or just change it in HEAD?

I agree with Alvaro: fix it in HEAD, but don't backport the change to
8.0 or 7.4.

-Neil

Alvaro Herrera <alvherre(at)surnet(dot)cl> writes:
> On Tue, May 31, 2005 at 03:43:56PM -0400, Tom Lane wrote:
>> OK, next question: is this a bug fix we should back-patch into 7.4,
>> or just change it in HEAD?

> I guess apply only in HEAD, and provide the patch for MLikharev so he
> can solve his immediate problem.

Done. Here is the patch (against CVS tip, but it should apply with
some fuzz in 8.0 or 7.4).

regards, tom lane

*** src/backend/executor/spi.c.orig Fri May 6 15:59:49 2005
--- src/backend/executor/spi.c Thu Jun 9 16:59:37 2005
***************
*** 1497,1502 ****
--- 1497,1503 ----
_SPI_pquery(QueryDesc *queryDesc, long tcount)
{
int operation = queryDesc->operation;
+ CommandDest origDest = queryDesc->dest->mydest;
int res;
Oid save_lastoid;

***************
*** 1548,1554 ****

ExecutorEnd(queryDesc);

! if (queryDesc->dest->mydest == SPI)
{
SPI_processed = _SPI_current->processed;
SPI_lastoid = save_lastoid;
--- 1549,1556 ----

ExecutorEnd(queryDesc);

! /* Test origDest here so that SPI_processed gets set in SELINTO case */
! if (origDest == SPI)
{
SPI_processed = _SPI_current->processed;
SPI_lastoid = save_lastoid;