Rewrite, normal execution vs. EXPLAIN ANALYZE

Started by Marko Tiikkajaover 15 years ago34 messages
#1Marko Tiikkaja
marko.tiikkaja@cs.helsinki.fi

Hi,

From the code I understood that when executing a query "normally", in
READ COMMITTED mode, we take a new snapshot for every query that comes
out of rewrite. But in an EXPLAIN ANALYZE, we only update the CID of
the snapshot taken when the EXPLAIN started.

Did I misunderstand the code? And if I didn't, why do we do this
differently?

Regards,
Marko Tiikkaja

#2David Fetter
david@fetter.org
In reply to: Marko Tiikkaja (#1)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

On Thu, Jul 22, 2010 at 08:43:35PM +0300, Marko Tiikkaja wrote:

Hi,

From the code I understood that when executing a query "normally",
in READ COMMITTED mode, we take a new snapshot for every query that
comes out of rewrite. But in an EXPLAIN ANALYZE, we only update the
CID of the snapshot taken when the EXPLAIN started.

Did I misunderstand the code? And if I didn't, why do we do this
differently?

You mentioned in IRC that this was in aid of getting wCTEs going. How
are these things connected?

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

#3Marko Tiikkaja
marko.tiikkaja@cs.helsinki.fi
In reply to: David Fetter (#2)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

On 7/23/2010 8:52 PM, David Fetter wrote:

On Thu, Jul 22, 2010 at 08:43:35PM +0300, Marko Tiikkaja wrote:

Did I misunderstand the code? And if I didn't, why do we do this
differently?

You mentioned in IRC that this was in aid of getting wCTEs going. How
are these things connected?

Currently, I'm trying to make wCTEs behave a bit like RULEs do. But if
every rewrite product takes a new snapshot, wCTEs will behave very
unpredictably.

But because EXPLAIN ANALYZE does *not* take a new snapshot for every
rewrite product, I'm starting to think that maybe this isn't the
behaviour we wanted to begin with?

Regards,
Marko Tiikkaja

#4Robert Haas
robertmhaas@gmail.com
In reply to: Marko Tiikkaja (#3)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

On Fri, Jul 23, 2010 at 2:13 PM, Marko Tiikkaja
<marko.tiikkaja@cs.helsinki.fi> wrote:

On 7/23/2010 8:52 PM, David Fetter wrote:

On Thu, Jul 22, 2010 at 08:43:35PM +0300, Marko Tiikkaja wrote:

Did I misunderstand the code?  And if I didn't, why do we do this
differently?

You mentioned in IRC that this was in aid of getting wCTEs going.  How
are these things connected?

Currently, I'm trying to make wCTEs behave a bit like RULEs do.  But if
every rewrite product takes a new snapshot, wCTEs will behave very
unpredictably.

But because EXPLAIN ANALYZE does *not* take a new snapshot for every rewrite
product, I'm starting to think that maybe this isn't the behaviour we wanted
to begin with?

Where should I be looking in the code for this?

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

#5Marko Tiikkaja
marko.tiikkaja@cs.helsinki.fi
In reply to: Robert Haas (#4)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

On 7/23/2010 10:00 PM, Robert Haas wrote:

On Fri, Jul 23, 2010 at 2:13 PM, Marko Tiikkaja
<marko.tiikkaja@cs.helsinki.fi> wrote:

Currently, I'm trying to make wCTEs behave a bit like RULEs do. But if
every rewrite product takes a new snapshot, wCTEs will behave very
unpredictably.

But because EXPLAIN ANALYZE does *not* take a new snapshot for every rewrite
product, I'm starting to think that maybe this isn't the behaviour we wanted
to begin with?

Where should I be looking in the code for this?

ProcessQuery() and ExplainOnePlan(). ProcessQuery calls
PushActiveSnapshot(GetTransactionSnapshot()) for every statement while
ExplainOnePlan calls PushUpdatedSnapshot(GetActiveSnapshot()).

Regards,
Marko Tiikkaja

#6Marko Tiikkaja
marko.tiikkaja@cs.helsinki.fi
In reply to: Marko Tiikkaja (#5)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

On 7/23/2010 10:06 PM, I wrote:

ProcessQuery() and ExplainOnePlan(). ProcessQuery calls
PushActiveSnapshot(GetTransactionSnapshot()) for every statement while
ExplainOnePlan calls PushUpdatedSnapshot(GetActiveSnapshot()).

Here's a test case demonstrating the difference:

=# create table foo(a int);
CREATE TABLE
=# create table bar(a int);
CREATE TABLE
=# create table baz(a int);
CREATE TABLE

=# create rule foorule as on update to foo do instead (select
pg_sleep(5); insert into bar select * from baz);
CREATE RULE

=# insert into foo values(0);

no EXPLAIN:

=# truncate bar, baz;
TRUNCATE TABLE

T1=# begin; update foo set a=a;
BEGIN
-- sleeps..

T2=# insert into baz values(0);
INSERT 0 1

-- T1 returns
pg_sleep
----------

(1 row)

UPDATE 0

T1=# select * from bar;
a
---
0
(1 row)

EXPLAIN:

=# truncate bar, baz;
TRUNCATE TABLE

T1=# begin; explain analyze update foo set a=a;
BEGIN
-- sleeps..

T2=# insert into baz values(0);
INSERT 0 1

-- T1 returns
(QUERY PLAN)

T1=# select * from bar;
a
---
(0 rows)

This may be a bit hard to follow, but essentially what happens is that
in EXPLAIN ANALYZE, the INSERT in the rule does not see the changes made
by T2 to baz while in the regular execution scenario it does.

Regards,
Marko Tiikkaja

#7Robert Haas
robertmhaas@gmail.com
In reply to: Marko Tiikkaja (#6)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

On Fri, Jul 23, 2010 at 3:19 PM, Marko Tiikkaja
<marko.tiikkaja@cs.helsinki.fi> wrote:

This may be a bit hard to follow, but essentially what happens is that in
EXPLAIN ANALYZE, the INSERT in the rule does not see the changes made by T2
to baz while in the regular execution scenario it does.

Well that's gotta be a bug, but in what I'm not sure.

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

#8David Fetter
david@fetter.org
In reply to: Robert Haas (#7)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

On Fri, Jul 23, 2010 at 03:30:03PM -0400, Robert Haas wrote:

On Fri, Jul 23, 2010 at 3:19 PM, Marko Tiikkaja
<marko.tiikkaja@cs.helsinki.fi> wrote:

This may be a bit hard to follow, but essentially what happens is
that in EXPLAIN ANALYZE, the INSERT in the rule does not see the
changes made by T2 to baz while in the regular execution scenario
it does.

Well that's gotta be a bug, but in what I'm not sure.

I've said it before, and I'll say it again. User-accessible RULEs are
themselves a bug :P

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

#9Robert Haas
robertmhaas@gmail.com
In reply to: David Fetter (#8)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

On Fri, Jul 23, 2010 at 3:31 PM, David Fetter <david@fetter.org> wrote:

On Fri, Jul 23, 2010 at 03:30:03PM -0400, Robert Haas wrote:

On Fri, Jul 23, 2010 at 3:19 PM, Marko Tiikkaja
<marko.tiikkaja@cs.helsinki.fi> wrote:

This may be a bit hard to follow, but essentially what happens is
that in EXPLAIN ANALYZE, the INSERT in the rule does not see the
changes made by T2 to baz while in the regular execution scenario
it does.

Well that's gotta be a bug, but in what I'm not sure.

I've said it before, and I'll say it again.  User-accessible RULEs are
themselves a bug :P

Maybe so, but perhaps it would be more productive to concentrate on
solving the immediate problem first.

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

#10Andres Freund
andres@anarazel.de
In reply to: Robert Haas (#7)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

On Fri, Jul 23, 2010 at 03:30:03PM -0400, Robert Haas wrote:

On Fri, Jul 23, 2010 at 3:19 PM, Marko Tiikkaja
<marko.tiikkaja@cs.helsinki.fi> wrote:

This may be a bit hard to follow, but essentially what happens is that in
EXPLAIN ANALYZE, the INSERT in the rule does not see the changes made by T2
to baz while in the regular execution scenario it does.

Well that's gotta be a bug, but in what I'm not sure.

One could argue that its less of a semantic change changing explain's
behaviour than the normal executors way of working...

Andres

#11Marko Tiikkaja
marko.tiikkaja@cs.helsinki.fi
In reply to: Andres Freund (#10)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

On 7/23/2010 10:46 PM, Andres Freund wrote:

On Fri, Jul 23, 2010 at 03:30:03PM -0400, Robert Haas wrote:

On Fri, Jul 23, 2010 at 3:19 PM, Marko Tiikkaja
<marko.tiikkaja@cs.helsinki.fi> wrote:

This may be a bit hard to follow, but essentially what happens is that in
EXPLAIN ANALYZE, the INSERT in the rule does not see the changes made by T2
to baz while in the regular execution scenario it does.

Well that's gotta be a bug, but in what I'm not sure.

One could argue that its less of a semantic change changing explain's
behaviour than the normal executors way of working...

One could also argue that people usually test their code with EXPLAIN
ANALYZE and could've made the opposite conclusion based on its output. ;-)

But I really have no idea what we should do about this. It seems to me
that EXPLAIN ANALYZE's behaviour is less surprising (that's actually
what I, before yesterday, always thought happens), but I'm not too sure
about that either.

Regards,
Marko Tiikkaja

#12Alvaro Herrera
alvherre@commandprompt.com
In reply to: Marko Tiikkaja (#3)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

Excerpts from Marko Tiikkaja's message of vie jul 23 14:13:18 -0400 2010:

On 7/23/2010 8:52 PM, David Fetter wrote:

On Thu, Jul 22, 2010 at 08:43:35PM +0300, Marko Tiikkaja wrote:

Did I misunderstand the code? And if I didn't, why do we do this
differently?

You mentioned in IRC that this was in aid of getting wCTEs going. How
are these things connected?

Currently, I'm trying to make wCTEs behave a bit like RULEs do. But if
every rewrite product takes a new snapshot, wCTEs will behave very
unpredictably.

I don't think it's fair game to change the behavior of multiple-output
rules at this point. However, I also think that it's unwise to base
wCTEs on the behavior of rules -- rules are widely considered broken and
unusable for nontrivial cases.

Also, I think that having a moving snapshot for the different parts of a
wCTE is going to mean they're unpredictable. For predictable usage
you'll be forcing the user to always wrap them in SERIALIZABLE
transactions.

In short I think a wCTE should only advance the CID, not get a whole new
snapshot.

#13Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Alvaro Herrera (#12)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

Alvaro Herrera <alvherre@commandprompt.com> wrote:

In short I think a wCTE should only advance the CID, not get a
whole new snapshot.

Even after unblocking from a write conflict at the READ COMMITTED
isolation level?

-Kevin

#14Marko Tiikkaja
marko.tiikkaja@cs.helsinki.fi
In reply to: Alvaro Herrera (#12)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

On 7/24/10 12:37 AM +0300, Alvaro Herrera wrote:

Excerpts from Marko Tiikkaja's message of vie jul 23 14:13:18 -0400 2010:

On 7/23/2010 8:52 PM, David Fetter wrote:

On Thu, Jul 22, 2010 at 08:43:35PM +0300, Marko Tiikkaja wrote:

Did I misunderstand the code? And if I didn't, why do we do this
differently?

You mentioned in IRC that this was in aid of getting wCTEs going. How
are these things connected?

Currently, I'm trying to make wCTEs behave a bit like RULEs do. But if
every rewrite product takes a new snapshot, wCTEs will behave very
unpredictably.

I don't think it's fair game to change the behavior of multiple-output
rules at this point. However, I also think that it's unwise to base
wCTEs on the behavior of rules -- rules are widely considered broken and
unusable for nontrivial cases.

I don't want to change the behaviour either, but we have two different
behaviours right now. We need to change at least the other.

wCTEs are not going to be based on any of the broken behaviour of rules,
that's for sure. What I meant is expanding a single query into multiple
queries and running the executor separately for all of them.

Also, I think that having a moving snapshot for the different parts of a
wCTE is going to mean they're unpredictable. For predictable usage
you'll be forcing the user to always wrap them in SERIALIZABLE
transactions.

That is *not* what has been discussed for wCTEs. A wCTE will only
modify the CID of the snapshot in any isolation mode.

In short I think a wCTE should only advance the CID, not get a whole new
snapshot.

Agreed.

Regards,
Marko Tiikkaja

#15Marko Tiikkaja
marko.tiikkaja@cs.helsinki.fi
In reply to: Kevin Grittner (#13)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

On 7/24/10 12:42 AM +0300, Kevin Grittner wrote:

Alvaro Herrera<alvherre@commandprompt.com> wrote:

In short I think a wCTE should only advance the CID, not get a
whole new snapshot.

Even after unblocking from a write conflict at the READ COMMITTED
isolation level?

I'm not sure what you mean by this; UPDATE and DELETE can take a look at
the new tuple but that's completely separate from the snapshot.

Regards,
Marko Tiikkaja

#16Alvaro Herrera
alvherre@commandprompt.com
In reply to: Marko Tiikkaja (#14)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

Excerpts from Marko Tiikkaja's message of vie jul 23 17:44:21 -0400 2010:

On 7/24/10 12:37 AM +0300, Alvaro Herrera wrote:

Excerpts from Marko Tiikkaja's message of vie jul 23 14:13:18 -0400 2010:

I don't think it's fair game to change the behavior of multiple-output
rules at this point. However, I also think that it's unwise to base
wCTEs on the behavior of rules -- rules are widely considered broken and
unusable for nontrivial cases.

I don't want to change the behaviour either, but we have two different
behaviours right now. We need to change at least the other.

It seems like it's EXPLAIN ANALYZE that needs fixing.

wCTEs are not going to be based on any of the broken behaviour of rules,
that's for sure. What I meant is expanding a single query into multiple
queries and running the executor separately for all of them.

Is a wCTE going to be expanded into multiple queries?

If not, it sounds like we're all agreed.

#17Marko Tiikkaja
marko.tiikkaja@cs.helsinki.fi
In reply to: Alvaro Herrera (#16)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

On 7/24/10 1:20 AM +0300, Alvaro Herrera wrote:

Excerpts from Marko Tiikkaja's message of vie jul 23 17:44:21 -0400 2010:

wCTEs are not going to be based on any of the broken behaviour of rules,
that's for sure. What I meant is expanding a single query into multiple
queries and running the executor separately for all of them.

Is a wCTE going to be expanded into multiple queries?

If not, it sounds like we're all agreed.

Yes, it will have to be. I tried to make it work for 9.0 by not
expanding, and it didn't work out too well.

Regards,
Marko Tiikkaja

#18Marko Tiikkaja
marko.tiikkaja@cs.helsinki.fi
In reply to: Alvaro Herrera (#16)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

On 7/24/10 1:20 AM +0300, Alvaro Herrera wrote:

It seems like it's EXPLAIN ANALYZE that needs fixing.

Yeah, looks like it. I see SQL functions also take a new snapshot for
every query.

Regards,
Marko Tiikkaja

#19Robert Haas
robertmhaas@gmail.com
In reply to: Alvaro Herrera (#16)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

On Fri, Jul 23, 2010 at 6:20 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:

Excerpts from Marko Tiikkaja's message of vie jul 23 17:44:21 -0400 2010:

On 7/24/10 12:37 AM +0300, Alvaro Herrera wrote:

Excerpts from Marko Tiikkaja's message of vie jul 23 14:13:18 -0400 2010:

I don't think it's fair game to change the behavior of multiple-output
rules at this point.  However, I also think that it's unwise to base
wCTEs on the behavior of rules -- rules are widely considered broken and
unusable for nontrivial cases.

I don't want to change the behaviour either, but we have two different
behaviours right now.  We need to change at least the other.

It seems like it's EXPLAIN ANALYZE that needs fixing.

I would suggest that if we're going to change this, we back-patch it
to 9.0 before beta4.

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

#20Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Marko Tiikkaja (#15)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> wrote:

I'm not sure what you mean by this; UPDATE and DELETE can take a
look at the new tuple but that's completely separate from the
snapshot.

Never mind -- I remembered that those could operate against tuples
not in the original snapshot, but forgot that they did it without
generating an actual fresh snapshot.

-Kevin

#21Marko Tiikkaja
marko.tiikkaja@cs.helsinki.fi
In reply to: Robert Haas (#19)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

On 7/24/2010 1:43 AM, Robert Haas wrote:

On Fri, Jul 23, 2010 at 6:20 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:

It seems like it's EXPLAIN ANALYZE that needs fixing.

I would suggest that if we're going to change this, we back-patch it
to 9.0 before beta4.

I did some digging and found the commit that changed the behaviour:
http://archives.postgresql.org/pgsql-committers/2004-09/msg00155.php and
then later Tom fixed a bug in it:
http://archives.postgresql.org/pgsql-committers/2005-10/msg00316.php

According to the latter commit, not updating the snapshot could be
preferable for EXPLAIN ANALYZE, but I don't see why this is. Maybe we
should wait until we hear from Tom?

Regards,
Marko Tiikkaja

#22Tom Lane
tgl@sss.pgh.pa.us
In reply to: Marko Tiikkaja (#21)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> writes:

According to the latter commit, not updating the snapshot could be
preferable for EXPLAIN ANALYZE, but I don't see why this is. Maybe we
should wait until we hear from Tom?

Sorry for not catching up on my email sooner. On the whole I'm in
agreement with the argument that wCTEs should *not* take whole new
snapshots between queries, but only advance the CID. The points
that sway me that way are:

1. To do otherwise will decrease the predictability of wCTE results.

2. The argument for taking a new snapshot seems to be mostly
"because that's what rules do"; but it's agreed that rules have a
lot of surprising and undesirable behavior, and it's not clear that
this isn't part of that.

3. In particular I don't agree with the argument that functions taking
new snapshots between queries should be a precedent for this. In
a function, the user thinks of the successive lines as separate queries,
and it's reasonable for them to act similarly to what would happen if
they were issued as separate top-level commands. I do not see that that
argument applies to wCTEs, which by definition are parts of a single
command.

In any case the behavior is going to have to be documented, but I vote
for advance-CID-only so far as wCTEs are concerned.

The other thing that was being argued was whether rules should be
changed to act that way too, and if not whether EXPLAIN ANALYZE should
be fixed to make sure it emulates rule execution better. Personally
I'd be in favor of changing rule execution and leaving EXPLAIN ANALYZE
alone, though I'm not sure if that position can command a consensus.
I seriously doubt that there are many applications out there that are
actually depending on this aspect of rule execution; if anything, there
are probably more that will see it as a bug.

regards, tom lane

#23Robert Haas
robertmhaas@gmail.com
In reply to: Tom Lane (#22)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

On Wed, Aug 4, 2010 at 2:45 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

The other thing that was being argued was whether rules should be
changed to act that way too, and if not whether EXPLAIN ANALYZE should
be fixed to make sure it emulates rule execution better.  Personally
I'd be in favor of changing rule execution and leaving EXPLAIN ANALYZE
alone, though I'm not sure if that position can command a consensus.
I seriously doubt that there are many applications out there that are
actually depending on this aspect of rule execution; if anything, there
are probably more that will see it as a bug.

Changing EXPLAIN ANALYZE seems a bit less likely to break things for
anyone depending on current behavior; but, on the other hand, it seems
there's a broad consensus that the best thing to do about rule
execution is deprecate it, so maybe it doesn't really matter.

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

#24Marko Tiikkaja
marko.tiikkaja@cs.helsinki.fi
In reply to: Robert Haas (#23)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

On 8/4/2010 10:26 PM, Robert Haas wrote:

On Wed, Aug 4, 2010 at 2:45 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote:

The other thing that was being argued was whether rules should be
changed to act that way too, and if not whether EXPLAIN ANALYZE should
be fixed to make sure it emulates rule execution better. Personally
I'd be in favor of changing rule execution and leaving EXPLAIN ANALYZE
alone, though I'm not sure if that position can command a consensus.
I seriously doubt that there are many applications out there that are
actually depending on this aspect of rule execution; if anything, there
are probably more that will see it as a bug.

Changing EXPLAIN ANALYZE seems a bit less likely to break things for
anyone depending on current behavior; but, on the other hand, it seems
there's a broad consensus that the best thing to do about rule
execution is deprecate it, so maybe it doesn't really matter.

I'm having a hard time imagining that anyone would depend on a behaviour
like this.

Regards,
Marko Tiikkaja

#25Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#23)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Aug 4, 2010 at 2:45 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I seriously doubt that there are many applications out there that are
actually depending on this aspect of rule execution; if anything, there
are probably more that will see it as a bug.

Changing EXPLAIN ANALYZE seems a bit less likely to break things for
anyone depending on current behavior;

Well, the point I was trying to make is that there may well be fewer
people depending on the current behavior than there are people for whom
the current behavior is wrong, only they don't know it because they've
not seen a failure (or not seen one often enough to diagnose what's
happening).

This is of course merest speculation either way. But I don't feel that
we need to necessarily treat rule behavior as graven in stone.

regards, tom lane

#26Marko Tiikkaja
marko.tiikkaja@cs.helsinki.fi
In reply to: Tom Lane (#25)
1 attachment(s)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

Hi,

I looked at fixing this inconsistency by making all query list snapshot
handling work like EXPLAIN ANALYZE's code does. The only reason I went
this way was that implementing wCTEs on top of this behaviour is a lot
easier.

There were three places that needed fixing. The SPI and portal logic
changes were quite straightforward, but the SQL language function code
previously didn't know what query trees of the execution_state list
belonged to which query so there was no way to tell when we actually
needed to take a new snapshot. The approach I took was to change the
representation of the SQL function cache to a list of execution_state
lists, and grab a new snapshot between the lists.

The patch needs a bit more comments and some cleaning up, but I thought
I'd get your input first. Thoughts?

Regards,
Marko Tiikkaja

Attachments:

snapshot.patchtext/plain; charset=iso-8859-1; name=snapshot.patch; x-mac-creator=0; x-mac-type=0Download
*** a/src/backend/catalog/pg_proc.c
--- b/src/backend/catalog/pg_proc.c
***************
*** 832,838 **** fmgr_sql_validator(PG_FUNCTION_ARGS)
  												  proc->proargtypes.values,
  												  proc->pronargs);
  			(void) check_sql_fn_retval(funcoid, proc->prorettype,
! 									   querytree_list,
  									   NULL, NULL);
  		}
  		else
--- 832,838 ----
  												  proc->proargtypes.values,
  												  proc->pronargs);
  			(void) check_sql_fn_retval(funcoid, proc->prorettype,
! 									   llast(querytree_list),
  									   NULL, NULL);
  		}
  		else
*** a/src/backend/executor/functions.c
--- b/src/backend/executor/functions.c
***************
*** 90,107 **** typedef struct
  	ParamListInfo paramLI;		/* Param list representing current args */
  
  	Tuplestorestate *tstore;	/* where we accumulate result tuples */
  
  	JunkFilter *junkFilter;		/* will be NULL if function returns VOID */
  
! 	/* head of linked list of execution_state records */
! 	execution_state *func_state;
  } SQLFunctionCache;
  
  typedef SQLFunctionCache *SQLFunctionCachePtr;
  
  
  /* non-export function prototypes */
! static execution_state *init_execution_state(List *queryTree_list,
  					 SQLFunctionCachePtr fcache,
  					 bool lazyEvalOK);
  static void init_sql_fcache(FmgrInfo *finfo, bool lazyEvalOK);
--- 90,107 ----
  	ParamListInfo paramLI;		/* Param list representing current args */
  
  	Tuplestorestate *tstore;	/* where we accumulate result tuples */
+ 	Snapshot	snapshot;
  
  	JunkFilter *junkFilter;		/* will be NULL if function returns VOID */
  
! 	List *func_state;
  } SQLFunctionCache;
  
  typedef SQLFunctionCache *SQLFunctionCachePtr;
  
  
  /* non-export function prototypes */
! static List *init_execution_state(List *queryTree_list,
  					 SQLFunctionCachePtr fcache,
  					 bool lazyEvalOK);
  static void init_sql_fcache(FmgrInfo *finfo, bool lazyEvalOK);
***************
*** 123,183 **** static void sqlfunction_destroy(DestReceiver *self);
  
  
  /* Set up the list of per-query execution_state records for a SQL function */
! static execution_state *
  init_execution_state(List *queryTree_list,
  					 SQLFunctionCachePtr fcache,
  					 bool lazyEvalOK)
  {
! 	execution_state *firstes = NULL;
! 	execution_state *preves = NULL;
  	execution_state *lasttages = NULL;
! 	ListCell   *qtl_item;
  
! 	foreach(qtl_item, queryTree_list)
  	{
! 		Query	   *queryTree = (Query *) lfirst(qtl_item);
! 		Node	   *stmt;
! 		execution_state *newes;
  
! 		Assert(IsA(queryTree, Query));
  
! 		if (queryTree->commandType == CMD_UTILITY)
! 			stmt = queryTree->utilityStmt;
! 		else
! 			stmt = (Node *) pg_plan_query(queryTree, 0, NULL);
  
! 		/* Precheck all commands for validity in a function */
! 		if (IsA(stmt, TransactionStmt))
! 			ereport(ERROR,
! 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! 			/* translator: %s is a SQL statement name */
! 					 errmsg("%s is not allowed in a SQL function",
! 							CreateCommandTag(stmt))));
  
! 		if (fcache->readonly_func && !CommandIsReadOnly(stmt))
! 			ereport(ERROR,
! 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! 			/* translator: %s is a SQL statement name */
! 					 errmsg("%s is not allowed in a non-volatile function",
! 							CreateCommandTag(stmt))));
  
! 		newes = (execution_state *) palloc(sizeof(execution_state));
! 		if (preves)
! 			preves->next = newes;
! 		else
! 			firstes = newes;
  
! 		newes->next = NULL;
! 		newes->status = F_EXEC_START;
! 		newes->setsResult = false;		/* might change below */
! 		newes->lazyEval = false;	/* might change below */
! 		newes->stmt = stmt;
! 		newes->qd = NULL;
  
! 		if (queryTree->canSetTag)
! 			lasttages = newes;
  
! 		preves = newes;
  	}
  
  	/*
--- 123,200 ----
  
  
  /* Set up the list of per-query execution_state records for a SQL function */
! static List *
  init_execution_state(List *queryTree_list,
  					 SQLFunctionCachePtr fcache,
  					 bool lazyEvalOK)
  {
! 	execution_state *firstes;
! 	execution_state *preves;
  	execution_state *lasttages = NULL;
! 	List	   *eslist;
! 	ListCell   *lc1;
! 	ListCell   *lc2;
! 	List	   *qtlist;
! 	Query	   *queryTree;
! 
! 
! 	eslist = NIL;
  
! 	foreach(lc1, queryTree_list)
  	{
! 		qtlist = (List *) lfirst(lc1);
! 		firstes = NULL;
! 		preves = NULL;
  
! 		foreach(lc2, qtlist)
! 		{
! 			Node	   *stmt;
! 			execution_state *newes;
  
! 			queryTree = (Query *) lfirst(lc2);
  
! 			Assert(IsA(queryTree, Query));
  
! 			if (queryTree->commandType == CMD_UTILITY)
! 				stmt = queryTree->utilityStmt;
! 			else
! 				stmt = (Node *) pg_plan_query(queryTree, 0, NULL);
  
! 			/* Precheck all commands for validity in a function */
! 			if (IsA(stmt, TransactionStmt))
! 				ereport(ERROR,
! 						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! 				/* translator: %s is a SQL statement name */
! 						 errmsg("%s is not allowed in a SQL function",
! 								CreateCommandTag(stmt))));
! 
! 			if (fcache->readonly_func && !CommandIsReadOnly(stmt))
! 				ereport(ERROR,
! 						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! 				/* translator: %s is a SQL statement name */
! 						 errmsg("%s is not allowed in a non-volatile function",
! 								CreateCommandTag(stmt))));
! 
! 			newes = (execution_state *) palloc(sizeof(execution_state));
! 			if (preves)
! 				preves->next = newes;
! 			else
! 				firstes = newes;
  
! 			newes->next = NULL;
! 			newes->status = F_EXEC_START;
! 			newes->setsResult = false;		/* might change below */
! 			newes->lazyEval = false;	/* might change below */
! 			newes->stmt = stmt;
! 			newes->qd = NULL;
  
! 			if (queryTree->canSetTag)
! 				lasttages = newes;
! 
! 			preves = newes;
! 		}
  
! 		eslist = lappend(eslist, firstes);
  	}
  
  	/*
***************
*** 210,216 **** init_execution_state(List *queryTree_list,
  		}
  	}
  
! 	return firstes;
  }
  
  /* Initialize the SQLFunctionCache for a SQL function */
--- 227,233 ----
  		}
  	}
  
! 	return eslist;
  }
  
  /* Initialize the SQLFunctionCache for a SQL function */
***************
*** 342,348 **** init_sql_fcache(FmgrInfo *finfo, bool lazyEvalOK)
  	 */
  	fcache->returnsTuple = check_sql_fn_retval(foid,
  											   rettype,
! 											   queryTree_list,
  											   NULL,
  											   &fcache->junkFilter);
  
--- 359,365 ----
  	 */
  	fcache->returnsTuple = check_sql_fn_retval(foid,
  											   rettype,
! 											   llast(queryTree_list),
  											   NULL,
  											   &fcache->junkFilter);
  
***************
*** 374,397 **** init_sql_fcache(FmgrInfo *finfo, bool lazyEvalOK)
  static void
  postquel_start(execution_state *es, SQLFunctionCachePtr fcache)
  {
- 	Snapshot	snapshot;
  	DestReceiver *dest;
  
  	Assert(es->qd == NULL);
  
! 	/*
! 	 * In a read-only function, use the surrounding query's snapshot;
! 	 * otherwise take a new snapshot for each query.  The snapshot should
! 	 * include a fresh command ID so that all work to date in this transaction
! 	 * is visible.
! 	 */
! 	if (fcache->readonly_func)
! 		snapshot = GetActiveSnapshot();
! 	else
! 	{
! 		CommandCounterIncrement();
! 		snapshot = GetTransactionSnapshot();
! 	}
  
  	/*
  	 * If this query produces the function result, send its output to the
--- 391,401 ----
  static void
  postquel_start(execution_state *es, SQLFunctionCachePtr fcache)
  {
  	DestReceiver *dest;
  
  	Assert(es->qd == NULL);
  
! 	Assert(ActiveSnapshotSet());
  
  	/*
  	 * If this query produces the function result, send its output to the
***************
*** 415,427 **** postquel_start(execution_state *es, SQLFunctionCachePtr fcache)
  	if (IsA(es->stmt, PlannedStmt))
  		es->qd = CreateQueryDesc((PlannedStmt *) es->stmt,
  								 fcache->src,
! 								 snapshot, InvalidSnapshot,
  								 dest,
  								 fcache->paramLI, 0);
  	else
  		es->qd = CreateUtilityQueryDesc(es->stmt,
  										fcache->src,
! 										snapshot,
  										dest,
  										fcache->paramLI);
  
--- 419,432 ----
  	if (IsA(es->stmt, PlannedStmt))
  		es->qd = CreateQueryDesc((PlannedStmt *) es->stmt,
  								 fcache->src,
! 								 GetActiveSnapshot(),
! 								 InvalidSnapshot,
  								 dest,
  								 fcache->paramLI, 0);
  	else
  		es->qd = CreateUtilityQueryDesc(es->stmt,
  										fcache->src,
! 										GetActiveSnapshot(),
  										dest,
  										fcache->paramLI);
  
***************
*** 617,622 **** fmgr_sql(PG_FUNCTION_ARGS)
--- 622,629 ----
  	execution_state *es;
  	TupleTableSlot *slot;
  	Datum		result;
+ 	List		*eslist;
+ 	ListCell    *eslc;
  
  	/*
  	 * Switch to context in which the fcache lives.  This ensures that
***************
*** 668,680 **** fmgr_sql(PG_FUNCTION_ARGS)
  		init_sql_fcache(fcinfo->flinfo, lazyEvalOK);
  		fcache = (SQLFunctionCachePtr) fcinfo->flinfo->fn_extra;
  	}
! 	es = fcache->func_state;
  
  	/*
  	 * Convert params to appropriate format if starting a fresh execution. (If
  	 * continuing execution, we can re-use prior params.)
  	 */
! 	if (es && es->status == F_EXEC_START)
  		postquel_sub_params(fcache, fcinfo);
  
  	/*
--- 675,687 ----
  		init_sql_fcache(fcinfo->flinfo, lazyEvalOK);
  		fcache = (SQLFunctionCachePtr) fcinfo->flinfo->fn_extra;
  	}
! 	eslist = fcache->func_state;
  
  	/*
  	 * Convert params to appropriate format if starting a fresh execution. (If
  	 * continuing execution, we can re-use prior params.)
  	 */
! 	if (linitial(eslist) && ((execution_state *) linitial(eslist))->status == F_EXEC_START)
  		postquel_sub_params(fcache, fcinfo);
  
  	/*
***************
*** 687,694 **** fmgr_sql(PG_FUNCTION_ARGS)
  	/*
  	 * Find first unfinished query in function.
  	 */
! 	while (es && es->status == F_EXEC_DONE)
! 		es = es->next;
  
  	/*
  	 * Execute each command in the function one after another until we either
--- 694,709 ----
  	/*
  	 * Find first unfinished query in function.
  	 */
! 	foreach(eslc, eslist)
! 	{
! 		es = (execution_state *) lfirst(eslc);
! 
! 		while (es && es->status == F_EXEC_DONE)
! 			es = es->next;
! 
! 		if (es)
! 			break;
! 	}
  
  	/*
  	 * Execute each command in the function one after another until we either
***************
*** 699,706 **** fmgr_sql(PG_FUNCTION_ARGS)
--- 714,744 ----
  		bool		completed;
  
  		if (es->status == F_EXEC_START)
+ 		{
+ 			if (!fcache->readonly_func)
+ 			{
+ 				/*
+ 				 * In a read-only function, use the surrounding query's snapshot;
+ 				 * otherwise take a new snapshot if we don't have one yet.  The
+ 				 * snapshot should include a fresh command ID so that all work to
+ 				 * date in this transaction is visible.
+ 				 */
+ 				if (!fcache->snapshot)
+ 				{
+ 					CommandCounterIncrement();
+ 					fcache->snapshot = RegisterSnapshot(GetTransactionSnapshot());
+ 					PushActiveSnapshot(fcache->snapshot);
+ 				}
+ 				else
+ 					PushUpdatedSnapshot(fcache->snapshot);
+ 			}
+ 			
  			postquel_start(es, fcache);
  
+ 			if (!fcache->readonly_func)
+ 				PopActiveSnapshot();
+ 		}
+ 
  		completed = postquel_getnext(es, fcache);
  
  		/*
***************
*** 726,731 **** fmgr_sql(PG_FUNCTION_ARGS)
--- 764,783 ----
  		if (es->status != F_EXEC_DONE)
  			break;
  		es = es->next;
+ 
+ 		if (!es)
+ 		{
+ 			eslc = lnext(eslc);
+ 			if (!eslc)
+ 				break;
+ 
+ 			es = (execution_state *) lfirst(eslc);
+ 
+ 			/* make sure we take a new snapshot for this query list */
+ 			Assert(fcache->snapshot != InvalidSnapshot);
+ 			UnregisterSnapshot(fcache->snapshot);
+ 			fcache->snapshot = InvalidSnapshot;
+ 		}
  	}
  
  	/*
***************
*** 794,799 **** fmgr_sql(PG_FUNCTION_ARGS)
--- 846,856 ----
  											  PointerGetDatum(fcache));
  				fcache->shutdown_reg = false;
  			}
+ 
+ 			/* Unregister snapshot if we have one */
+ 			if (fcache->snapshot != InvalidSnapshot)
+ 				UnregisterSnapshot(fcache->snapshot);
+ 			fcache->snapshot = InvalidSnapshot;
  		}
  		else
  		{
***************
*** 820,825 **** fmgr_sql(PG_FUNCTION_ARGS)
--- 877,887 ----
  											  PointerGetDatum(fcache));
  				fcache->shutdown_reg = false;
  			}
+ 
+ 			/* Unregister snapshot if we have one */
+ 			if (fcache->snapshot != InvalidSnapshot)
+ 				UnregisterSnapshot(fcache->snapshot);
+ 			fcache->snapshot = InvalidSnapshot;
  		}
  	}
  	else
***************
*** 850,855 **** fmgr_sql(PG_FUNCTION_ARGS)
--- 912,922 ----
  
  		/* Clear the tuplestore, but keep it for next time */
  		tuplestore_clear(fcache->tstore);
+ 
+ 		/* Unregister snapshot if we have one */
+ 		if (fcache->snapshot != InvalidSnapshot)
+ 			UnregisterSnapshot(fcache->snapshot);
+ 		fcache->snapshot = InvalidSnapshot;
  	}
  
  	/*
***************
*** 858,868 **** fmgr_sql(PG_FUNCTION_ARGS)
  	 */
  	if (es == NULL)
  	{
! 		es = fcache->func_state;
! 		while (es)
  		{
! 			es->status = F_EXEC_START;
! 			es = es->next;
  		}
  	}
  
--- 925,938 ----
  	 */
  	if (es == NULL)
  	{
! 		foreach(eslc, fcache->func_state)
  		{
! 			es = (execution_state *) lfirst(eslc);
! 			while (es)
! 			{
! 				es->status = F_EXEC_START;
! 				es = es->next;
! 			}
  		}
  	}
  
***************
*** 913,931 **** sql_exec_error_callback(void *arg)
  	{
  		execution_state *es;
  		int			query_num;
  
- 		es = fcache->func_state;
  		query_num = 1;
! 		while (es)
  		{
! 			if (es->qd)
  			{
! 				errcontext("SQL function \"%s\" statement %d",
! 						   fcache->fname, query_num);
! 				break;
  			}
- 			es = es->next;
- 			query_num++;
  		}
  		if (es == NULL)
  		{
--- 983,1006 ----
  	{
  		execution_state *es;
  		int			query_num;
+ 		ListCell   *lc;
  
  		query_num = 1;
! 
! 		foreach(lc, fcache->func_state)
  		{
! 			es = (execution_state *) lfirst(lc);
! 			while (es)
  			{
! 				if (es->qd)
! 				{
! 					errcontext("SQL function \"%s\" statement %d",
! 							   fcache->fname, query_num);
! 					break;
! 				}
! 				es = es->next;
! 				query_num++;
  			}
  		}
  		if (es == NULL)
  		{
***************
*** 956,973 **** static void
  ShutdownSQLFunction(Datum arg)
  {
  	SQLFunctionCachePtr fcache = (SQLFunctionCachePtr) DatumGetPointer(arg);
! 	execution_state *es = fcache->func_state;
  
! 	while (es != NULL)
  	{
! 		/* Shut down anything still running */
! 		if (es->status == F_EXEC_RUN)
! 			postquel_end(es);
! 		/* Reset states to START in case we're called again */
! 		es->status = F_EXEC_START;
! 		es = es->next;
  	}
  
  	/* Release tuplestore if we have one */
  	if (fcache->tstore)
  		tuplestore_end(fcache->tstore);
--- 1031,1059 ----
  ShutdownSQLFunction(Datum arg)
  {
  	SQLFunctionCachePtr fcache = (SQLFunctionCachePtr) DatumGetPointer(arg);
! 	execution_state *es;
! 	ListCell		*lc;
  
! 	foreach(lc, fcache->func_state)
  	{
! 		es = (execution_state *) lfirst(lc);
! 
! 		while (es)
! 		{
! 			/* Shut down anything still running */
! 			if (es->status == F_EXEC_RUN)
! 				postquel_end(es);
! 			/* Reset states to START in case we're called again */
! 			es->status = F_EXEC_START;
! 			es = es->next;
! 		}
  	}
  
+ 	/* Unregister snapshot if we have one */
+ 	if (fcache->snapshot != InvalidSnapshot)
+ 		UnregisterSnapshot(fcache->snapshot);
+ 	fcache->snapshot = InvalidSnapshot;
+ 
  	/* Release tuplestore if we have one */
  	if (fcache->tstore)
  		tuplestore_end(fcache->tstore);
*** a/src/backend/executor/spi.c
--- b/src/backend/executor/spi.c
***************
*** 1769,1774 **** _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
--- 1769,1775 ----
  	SPITupleTable *my_tuptable = NULL;
  	int			res = 0;
  	bool		have_active_snap = ActiveSnapshotSet();
+ 	bool		registered_snap = false;
  	ErrorContextCallback spierrcontext;
  	CachedPlan *cplan = NULL;
  	ListCell   *lc1;
***************
*** 1872,1879 **** _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
  				}
  				else
  				{
! 					PushActiveSnapshot(GetTransactionSnapshot());
  					pushed_active_snap = true;
  				}
  			}
  			else
--- 1873,1882 ----
  				}
  				else
  				{
! 					snapshot = RegisterSnapshot(GetTransactionSnapshot());
! 					PushActiveSnapshot(snapshot);
  					pushed_active_snap = true;
+ 					registered_snap = true;
  				}
  			}
  			else
***************
*** 1966,1975 **** _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
--- 1969,1991 ----
  		 */
  		if (!read_only)
  			CommandCounterIncrement();
+ 
+ 		/*
+ 		 * If we took a new snapshot for this query list, unregister it and
+ 		 * make sure we take a new one for the next list.
+ 		 */
+ 		if (registered_snap)
+ 		{
+ 			UnregisterSnapshot(snapshot);
+ 			snapshot = InvalidSnapshot;
+ 		}
  	}
  
  fail:
  
+ 	if (registered_snap)
+ 		UnregisterSnapshot(snapshot);
+ 
  	/* We no longer need the cached plan refcount, if any */
  	if (cplan)
  		ReleaseCachedPlan(cplan, true);
*** a/src/backend/tcop/postgres.c
--- b/src/backend/tcop/postgres.c
***************
*** 537,547 **** pg_parse_and_rewrite(const char *query_string,	/* string to execute */
  	{
  		Node	   *parsetree = (Node *) lfirst(list_item);
  
! 		querytree_list = list_concat(querytree_list,
! 									 pg_analyze_and_rewrite(parsetree,
! 															query_string,
! 															paramTypes,
! 															numParams));
  	}
  
  	return querytree_list;
--- 537,547 ----
  	{
  		Node	   *parsetree = (Node *) lfirst(list_item);
  
! 		querytree_list = lappend(querytree_list,
! 								 pg_analyze_and_rewrite(parsetree,
! 														query_string,
! 														paramTypes,
! 														numParams));
  	}
  
  	return querytree_list;
*** a/src/backend/tcop/pquery.c
--- b/src/backend/tcop/pquery.c
***************
*** 170,180 **** ProcessQuery(PlannedStmt *plan,
  	elog(DEBUG3, "ProcessQuery");
  
  	/*
- 	 * Must always set a snapshot for plannable queries.
- 	 */
- 	PushActiveSnapshot(GetTransactionSnapshot());
- 
- 	/*
  	 * Create the QueryDesc object
  	 */
  	queryDesc = CreateQueryDesc(plan, sourceText,
--- 170,175 ----
***************
*** 234,241 **** ProcessQuery(PlannedStmt *plan,
  	/* Now take care of any queued AFTER triggers */
  	AfterTriggerEndQuery(queryDesc->estate);
  
- 	PopActiveSnapshot();
- 
  	/*
  	 * Now, we close down all the scans and free allocated resources.
  	 */
--- 229,234 ----
***************
*** 1220,1225 **** PortalRunMulti(Portal portal, bool isTopLevel,
--- 1213,1219 ----
  			   char *completionTag)
  {
  	ListCell   *stmtlist_item;
+ 	Snapshot	snapshot = InvalidSnapshot;
  
  	/*
  	 * If the destination is DestRemoteExecute, change to DestNone.  The
***************
*** 1262,1267 **** PortalRunMulti(Portal portal, bool isTopLevel,
--- 1256,1270 ----
  			if (log_executor_stats)
  				ResetUsage();
  
+ 			/* if no snapshot is set, grab a new one and register it */
+ 			if (snapshot == InvalidSnapshot)
+ 			{
+ 				snapshot = RegisterSnapshot(GetTransactionSnapshot());
+ 				PushActiveSnapshot(snapshot);
+ 			}
+ 			else
+ 				PushUpdatedSnapshot(snapshot);
+ 
  			if (pstmt->canSetTag)
  			{
  				/* statement can set tag string */
***************
*** 1279,1284 **** PortalRunMulti(Portal portal, bool isTopLevel,
--- 1282,1289 ----
  							 altdest, NULL);
  			}
  
+ 			PopActiveSnapshot();
+ 
  			if (log_executor_stats)
  				ShowUsage("EXECUTOR STATISTICS");
  
***************
*** 1291,1301 **** PortalRunMulti(Portal portal, bool isTopLevel,
--- 1296,1320 ----
  			 *
  			 * These are assumed canSetTag if they're the only stmt in the
  			 * portal.
+ 			 *
+ 			 * NotifyStmt is the only utility statement allowed in a list of
+ 			 * rewritten queries, and it doesn't need a snapshot so we don't
+ 			 * need to worry about it.  However, if the list has only one
+ 			 * statement and it's a utility statement, we are not allowed to
+ 			 * take a snapshot.  See the first comment in PortalRunUtility().
  			 */
  			if (list_length(portal->stmts) == 1)
+ 			{
+ 				Assert(snapshot == InvalidSnapshot);
+ 
  				PortalRunUtility(portal, stmt, isTopLevel, dest, completionTag);
+ 			}
  			else
+ 			{
+ 				Assert(IsA(stmt, NotifyStmt));
+ 
  				PortalRunUtility(portal, stmt, isTopLevel, altdest, NULL);
+ 			}
  		}
  
  		/*
***************
*** 1313,1318 **** PortalRunMulti(Portal portal, bool isTopLevel,
--- 1332,1340 ----
  		MemoryContextDeleteChildren(PortalGetHeapMemory(portal));
  	}
  
+ 	if (snapshot != InvalidSnapshot)
+ 		UnregisterSnapshot(snapshot);
+ 
  	/*
  	 * If a command completion tag was supplied, use it.  Otherwise use the
  	 * portal's commandTag as the default completion tag.
#27Marko Tiikkaja
marko.tiikkaja@cs.helsinki.fi
In reply to: Marko Tiikkaja (#26)
1 attachment(s)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

On 2010-08-31 12:07 AM +0300, I wrote:

I looked at fixing this..

The previous patch had a bug in fmgr_sql() our regression tests didn't
catch. Fixed version attached.

Regards,
Marko Tiikkaja

Attachments:

snapshot.patchtext/plain; charset=iso-8859-1; name=snapshot.patch; x-mac-creator=0; x-mac-type=0Download
*** a/src/backend/catalog/pg_proc.c
--- b/src/backend/catalog/pg_proc.c
***************
*** 832,838 **** fmgr_sql_validator(PG_FUNCTION_ARGS)
  												  proc->proargtypes.values,
  												  proc->pronargs);
  			(void) check_sql_fn_retval(funcoid, proc->prorettype,
! 									   querytree_list,
  									   NULL, NULL);
  		}
  		else
--- 832,838 ----
  												  proc->proargtypes.values,
  												  proc->pronargs);
  			(void) check_sql_fn_retval(funcoid, proc->prorettype,
! 									   llast(querytree_list),
  									   NULL, NULL);
  		}
  		else
*** a/src/backend/executor/functions.c
--- b/src/backend/executor/functions.c
***************
*** 90,107 **** typedef struct
  	ParamListInfo paramLI;		/* Param list representing current args */
  
  	Tuplestorestate *tstore;	/* where we accumulate result tuples */
  
  	JunkFilter *junkFilter;		/* will be NULL if function returns VOID */
  
! 	/* head of linked list of execution_state records */
! 	execution_state *func_state;
  } SQLFunctionCache;
  
  typedef SQLFunctionCache *SQLFunctionCachePtr;
  
  
  /* non-export function prototypes */
! static execution_state *init_execution_state(List *queryTree_list,
  					 SQLFunctionCachePtr fcache,
  					 bool lazyEvalOK);
  static void init_sql_fcache(FmgrInfo *finfo, bool lazyEvalOK);
--- 90,107 ----
  	ParamListInfo paramLI;		/* Param list representing current args */
  
  	Tuplestorestate *tstore;	/* where we accumulate result tuples */
+ 	Snapshot	snapshot;
  
  	JunkFilter *junkFilter;		/* will be NULL if function returns VOID */
  
! 	List *func_state;
  } SQLFunctionCache;
  
  typedef SQLFunctionCache *SQLFunctionCachePtr;
  
  
  /* non-export function prototypes */
! static List *init_execution_state(List *queryTree_list,
  					 SQLFunctionCachePtr fcache,
  					 bool lazyEvalOK);
  static void init_sql_fcache(FmgrInfo *finfo, bool lazyEvalOK);
***************
*** 123,183 **** static void sqlfunction_destroy(DestReceiver *self);
  
  
  /* Set up the list of per-query execution_state records for a SQL function */
! static execution_state *
  init_execution_state(List *queryTree_list,
  					 SQLFunctionCachePtr fcache,
  					 bool lazyEvalOK)
  {
! 	execution_state *firstes = NULL;
! 	execution_state *preves = NULL;
  	execution_state *lasttages = NULL;
! 	ListCell   *qtl_item;
  
! 	foreach(qtl_item, queryTree_list)
  	{
! 		Query	   *queryTree = (Query *) lfirst(qtl_item);
! 		Node	   *stmt;
! 		execution_state *newes;
  
! 		Assert(IsA(queryTree, Query));
  
! 		if (queryTree->commandType == CMD_UTILITY)
! 			stmt = queryTree->utilityStmt;
! 		else
! 			stmt = (Node *) pg_plan_query(queryTree, 0, NULL);
  
! 		/* Precheck all commands for validity in a function */
! 		if (IsA(stmt, TransactionStmt))
! 			ereport(ERROR,
! 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! 			/* translator: %s is a SQL statement name */
! 					 errmsg("%s is not allowed in a SQL function",
! 							CreateCommandTag(stmt))));
  
! 		if (fcache->readonly_func && !CommandIsReadOnly(stmt))
! 			ereport(ERROR,
! 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! 			/* translator: %s is a SQL statement name */
! 					 errmsg("%s is not allowed in a non-volatile function",
! 							CreateCommandTag(stmt))));
  
! 		newes = (execution_state *) palloc(sizeof(execution_state));
! 		if (preves)
! 			preves->next = newes;
! 		else
! 			firstes = newes;
  
! 		newes->next = NULL;
! 		newes->status = F_EXEC_START;
! 		newes->setsResult = false;		/* might change below */
! 		newes->lazyEval = false;	/* might change below */
! 		newes->stmt = stmt;
! 		newes->qd = NULL;
  
! 		if (queryTree->canSetTag)
! 			lasttages = newes;
  
! 		preves = newes;
  	}
  
  	/*
--- 123,200 ----
  
  
  /* Set up the list of per-query execution_state records for a SQL function */
! static List *
  init_execution_state(List *queryTree_list,
  					 SQLFunctionCachePtr fcache,
  					 bool lazyEvalOK)
  {
! 	execution_state *firstes;
! 	execution_state *preves;
  	execution_state *lasttages = NULL;
! 	List	   *eslist;
! 	ListCell   *lc1;
! 	ListCell   *lc2;
! 	List	   *qtlist;
! 	Query	   *queryTree;
! 
! 
! 	eslist = NIL;
  
! 	foreach(lc1, queryTree_list)
  	{
! 		qtlist = (List *) lfirst(lc1);
! 		firstes = NULL;
! 		preves = NULL;
  
! 		foreach(lc2, qtlist)
! 		{
! 			Node	   *stmt;
! 			execution_state *newes;
  
! 			queryTree = (Query *) lfirst(lc2);
  
! 			Assert(IsA(queryTree, Query));
  
! 			if (queryTree->commandType == CMD_UTILITY)
! 				stmt = queryTree->utilityStmt;
! 			else
! 				stmt = (Node *) pg_plan_query(queryTree, 0, NULL);
  
! 			/* Precheck all commands for validity in a function */
! 			if (IsA(stmt, TransactionStmt))
! 				ereport(ERROR,
! 						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! 				/* translator: %s is a SQL statement name */
! 						 errmsg("%s is not allowed in a SQL function",
! 								CreateCommandTag(stmt))));
  
! 			if (fcache->readonly_func && !CommandIsReadOnly(stmt))
! 				ereport(ERROR,
! 						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! 				/* translator: %s is a SQL statement name */
! 						 errmsg("%s is not allowed in a non-volatile function",
! 								CreateCommandTag(stmt))));
! 
! 			newes = (execution_state *) palloc(sizeof(execution_state));
! 			if (preves)
! 				preves->next = newes;
! 			else
! 				firstes = newes;
  
! 			newes->next = NULL;
! 			newes->status = F_EXEC_START;
! 			newes->setsResult = false;		/* might change below */
! 			newes->lazyEval = false;	/* might change below */
! 			newes->stmt = stmt;
! 			newes->qd = NULL;
  
! 			if (queryTree->canSetTag)
! 				lasttages = newes;
! 
! 			preves = newes;
! 		}
! 
! 		eslist = lappend(eslist, firstes);
  	}
  
  	/*
***************
*** 210,216 **** init_execution_state(List *queryTree_list,
  		}
  	}
  
! 	return firstes;
  }
  
  /* Initialize the SQLFunctionCache for a SQL function */
--- 227,233 ----
  		}
  	}
  
! 	return eslist;
  }
  
  /* Initialize the SQLFunctionCache for a SQL function */
***************
*** 342,348 **** init_sql_fcache(FmgrInfo *finfo, bool lazyEvalOK)
  	 */
  	fcache->returnsTuple = check_sql_fn_retval(foid,
  											   rettype,
! 											   queryTree_list,
  											   NULL,
  											   &fcache->junkFilter);
  
--- 359,365 ----
  	 */
  	fcache->returnsTuple = check_sql_fn_retval(foid,
  											   rettype,
! 											   llast(queryTree_list),
  											   NULL,
  											   &fcache->junkFilter);
  
***************
*** 374,397 **** init_sql_fcache(FmgrInfo *finfo, bool lazyEvalOK)
  static void
  postquel_start(execution_state *es, SQLFunctionCachePtr fcache)
  {
- 	Snapshot	snapshot;
  	DestReceiver *dest;
  
  	Assert(es->qd == NULL);
  
! 	/*
! 	 * In a read-only function, use the surrounding query's snapshot;
! 	 * otherwise take a new snapshot for each query.  The snapshot should
! 	 * include a fresh command ID so that all work to date in this transaction
! 	 * is visible.
! 	 */
! 	if (fcache->readonly_func)
! 		snapshot = GetActiveSnapshot();
! 	else
! 	{
! 		CommandCounterIncrement();
! 		snapshot = GetTransactionSnapshot();
! 	}
  
  	/*
  	 * If this query produces the function result, send its output to the
--- 391,401 ----
  static void
  postquel_start(execution_state *es, SQLFunctionCachePtr fcache)
  {
  	DestReceiver *dest;
  
  	Assert(es->qd == NULL);
  
! 	Assert(ActiveSnapshotSet());
  
  	/*
  	 * If this query produces the function result, send its output to the
***************
*** 415,427 **** postquel_start(execution_state *es, SQLFunctionCachePtr fcache)
  	if (IsA(es->stmt, PlannedStmt))
  		es->qd = CreateQueryDesc((PlannedStmt *) es->stmt,
  								 fcache->src,
! 								 snapshot, InvalidSnapshot,
  								 dest,
  								 fcache->paramLI, 0);
  	else
  		es->qd = CreateUtilityQueryDesc(es->stmt,
  										fcache->src,
! 										snapshot,
  										dest,
  										fcache->paramLI);
  
--- 419,432 ----
  	if (IsA(es->stmt, PlannedStmt))
  		es->qd = CreateQueryDesc((PlannedStmt *) es->stmt,
  								 fcache->src,
! 								 GetActiveSnapshot(),
! 								 InvalidSnapshot,
  								 dest,
  								 fcache->paramLI, 0);
  	else
  		es->qd = CreateUtilityQueryDesc(es->stmt,
  										fcache->src,
! 										GetActiveSnapshot(),
  										dest,
  										fcache->paramLI);
  
***************
*** 617,622 **** fmgr_sql(PG_FUNCTION_ARGS)
--- 622,629 ----
  	execution_state *es;
  	TupleTableSlot *slot;
  	Datum		result;
+ 	List		*eslist;
+ 	ListCell    *eslc;
  
  	/*
  	 * Switch to context in which the fcache lives.  This ensures that
***************
*** 668,680 **** fmgr_sql(PG_FUNCTION_ARGS)
  		init_sql_fcache(fcinfo->flinfo, lazyEvalOK);
  		fcache = (SQLFunctionCachePtr) fcinfo->flinfo->fn_extra;
  	}
! 	es = fcache->func_state;
  
  	/*
  	 * Convert params to appropriate format if starting a fresh execution. (If
  	 * continuing execution, we can re-use prior params.)
  	 */
! 	if (es && es->status == F_EXEC_START)
  		postquel_sub_params(fcache, fcinfo);
  
  	/*
--- 675,687 ----
  		init_sql_fcache(fcinfo->flinfo, lazyEvalOK);
  		fcache = (SQLFunctionCachePtr) fcinfo->flinfo->fn_extra;
  	}
! 	eslist = fcache->func_state;
  
  	/*
  	 * Convert params to appropriate format if starting a fresh execution. (If
  	 * continuing execution, we can re-use prior params.)
  	 */
! 	if (linitial(eslist) && ((execution_state *) linitial(eslist))->status == F_EXEC_START)
  		postquel_sub_params(fcache, fcinfo);
  
  	/*
***************
*** 687,694 **** fmgr_sql(PG_FUNCTION_ARGS)
  	/*
  	 * Find first unfinished query in function.
  	 */
! 	while (es && es->status == F_EXEC_DONE)
! 		es = es->next;
  
  	/*
  	 * Execute each command in the function one after another until we either
--- 694,709 ----
  	/*
  	 * Find first unfinished query in function.
  	 */
! 	foreach(eslc, eslist)
! 	{
! 		es = (execution_state *) lfirst(eslc);
! 
! 		while (es && es->status == F_EXEC_DONE)
! 			es = es->next;
! 
! 		if (es)
! 			break;
! 	}
  
  	/*
  	 * Execute each command in the function one after another until we either
***************
*** 699,706 **** fmgr_sql(PG_FUNCTION_ARGS)
--- 714,744 ----
  		bool		completed;
  
  		if (es->status == F_EXEC_START)
+ 		{
+ 			if (!fcache->readonly_func)
+ 			{
+ 				/*
+ 				 * In a read-only function, use the surrounding query's snapshot;
+ 				 * otherwise take a new snapshot if we don't have one yet.  The
+ 				 * snapshot should include a fresh command ID so that all work to
+ 				 * date in this transaction is visible.
+ 				 */
+ 				if (!fcache->snapshot)
+ 				{
+ 					CommandCounterIncrement();
+ 					fcache->snapshot = RegisterSnapshot(GetTransactionSnapshot());
+ 					PushActiveSnapshot(fcache->snapshot);
+ 				}
+ 				else
+ 					PushUpdatedSnapshot(fcache->snapshot);
+ 			}
+ 			
  			postquel_start(es, fcache);
  
+ 			if (!fcache->readonly_func)
+ 				PopActiveSnapshot();
+ 		}
+ 
  		completed = postquel_getnext(es, fcache);
  
  		/*
***************
*** 726,731 **** fmgr_sql(PG_FUNCTION_ARGS)
--- 764,788 ----
  		if (es->status != F_EXEC_DONE)
  			break;
  		es = es->next;
+ 
+ 		if (!es)
+ 		{
+ 			eslc = lnext(eslc);
+ 			if (!eslc)
+ 				break;
+ 
+ 			es = (execution_state *) lfirst(eslc);
+ 
+ 			/* make sure we take a new snapshot for this query list */
+ 			if (!fcache->readonly_func)
+ 			{
+ 				Assert(fcache->snapshot != InvalidSnapshot);
+ 				UnregisterSnapshot(fcache->snapshot);
+ 				fcache->snapshot = InvalidSnapshot;
+ 			}
+ 			else
+ 				Assert(fcache->snapshot == InvalidSnapshot);
+ 		}
  	}
  
  	/*
***************
*** 794,799 **** fmgr_sql(PG_FUNCTION_ARGS)
--- 851,861 ----
  											  PointerGetDatum(fcache));
  				fcache->shutdown_reg = false;
  			}
+ 
+ 			/* Unregister snapshot if we have one */
+ 			if (fcache->snapshot != InvalidSnapshot)
+ 				UnregisterSnapshot(fcache->snapshot);
+ 			fcache->snapshot = InvalidSnapshot;
  		}
  		else
  		{
***************
*** 820,825 **** fmgr_sql(PG_FUNCTION_ARGS)
--- 882,892 ----
  											  PointerGetDatum(fcache));
  				fcache->shutdown_reg = false;
  			}
+ 
+ 			/* Unregister snapshot if we have one */
+ 			if (fcache->snapshot != InvalidSnapshot)
+ 				UnregisterSnapshot(fcache->snapshot);
+ 			fcache->snapshot = InvalidSnapshot;
  		}
  	}
  	else
***************
*** 850,855 **** fmgr_sql(PG_FUNCTION_ARGS)
--- 917,927 ----
  
  		/* Clear the tuplestore, but keep it for next time */
  		tuplestore_clear(fcache->tstore);
+ 
+ 		/* Unregister snapshot if we have one */
+ 		if (fcache->snapshot != InvalidSnapshot)
+ 			UnregisterSnapshot(fcache->snapshot);
+ 		fcache->snapshot = InvalidSnapshot;
  	}
  
  	/*
***************
*** 858,868 **** fmgr_sql(PG_FUNCTION_ARGS)
  	 */
  	if (es == NULL)
  	{
! 		es = fcache->func_state;
! 		while (es)
  		{
! 			es->status = F_EXEC_START;
! 			es = es->next;
  		}
  	}
  
--- 930,943 ----
  	 */
  	if (es == NULL)
  	{
! 		foreach(eslc, fcache->func_state)
  		{
! 			es = (execution_state *) lfirst(eslc);
! 			while (es)
! 			{
! 				es->status = F_EXEC_START;
! 				es = es->next;
! 			}
  		}
  	}
  
***************
*** 913,931 **** sql_exec_error_callback(void *arg)
  	{
  		execution_state *es;
  		int			query_num;
  
- 		es = fcache->func_state;
  		query_num = 1;
! 		while (es)
  		{
! 			if (es->qd)
  			{
! 				errcontext("SQL function \"%s\" statement %d",
! 						   fcache->fname, query_num);
! 				break;
  			}
- 			es = es->next;
- 			query_num++;
  		}
  		if (es == NULL)
  		{
--- 988,1011 ----
  	{
  		execution_state *es;
  		int			query_num;
+ 		ListCell   *lc;
  
  		query_num = 1;
! 
! 		foreach(lc, fcache->func_state)
  		{
! 			es = (execution_state *) lfirst(lc);
! 			while (es)
  			{
! 				if (es->qd)
! 				{
! 					errcontext("SQL function \"%s\" statement %d",
! 							   fcache->fname, query_num);
! 					break;
! 				}
! 				es = es->next;
! 				query_num++;
  			}
  		}
  		if (es == NULL)
  		{
***************
*** 956,973 **** static void
  ShutdownSQLFunction(Datum arg)
  {
  	SQLFunctionCachePtr fcache = (SQLFunctionCachePtr) DatumGetPointer(arg);
! 	execution_state *es = fcache->func_state;
  
! 	while (es != NULL)
  	{
! 		/* Shut down anything still running */
! 		if (es->status == F_EXEC_RUN)
! 			postquel_end(es);
! 		/* Reset states to START in case we're called again */
! 		es->status = F_EXEC_START;
! 		es = es->next;
  	}
  
  	/* Release tuplestore if we have one */
  	if (fcache->tstore)
  		tuplestore_end(fcache->tstore);
--- 1036,1064 ----
  ShutdownSQLFunction(Datum arg)
  {
  	SQLFunctionCachePtr fcache = (SQLFunctionCachePtr) DatumGetPointer(arg);
! 	execution_state *es;
! 	ListCell		*lc;
  
! 	foreach(lc, fcache->func_state)
  	{
! 		es = (execution_state *) lfirst(lc);
! 
! 		while (es)
! 		{
! 			/* Shut down anything still running */
! 			if (es->status == F_EXEC_RUN)
! 				postquel_end(es);
! 			/* Reset states to START in case we're called again */
! 			es->status = F_EXEC_START;
! 			es = es->next;
! 		}
  	}
  
+ 	/* Unregister snapshot if we have one */
+ 	if (fcache->snapshot != InvalidSnapshot)
+ 		UnregisterSnapshot(fcache->snapshot);
+ 	fcache->snapshot = InvalidSnapshot;
+ 
  	/* Release tuplestore if we have one */
  	if (fcache->tstore)
  		tuplestore_end(fcache->tstore);
*** a/src/backend/executor/spi.c
--- b/src/backend/executor/spi.c
***************
*** 1769,1774 **** _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
--- 1769,1775 ----
  	SPITupleTable *my_tuptable = NULL;
  	int			res = 0;
  	bool		have_active_snap = ActiveSnapshotSet();
+ 	bool		registered_snap = false;
  	ErrorContextCallback spierrcontext;
  	CachedPlan *cplan = NULL;
  	ListCell   *lc1;
***************
*** 1872,1879 **** _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
  				}
  				else
  				{
! 					PushActiveSnapshot(GetTransactionSnapshot());
  					pushed_active_snap = true;
  				}
  			}
  			else
--- 1873,1882 ----
  				}
  				else
  				{
! 					snapshot = RegisterSnapshot(GetTransactionSnapshot());
! 					PushActiveSnapshot(snapshot);
  					pushed_active_snap = true;
+ 					registered_snap = true;
  				}
  			}
  			else
***************
*** 1966,1975 **** _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
--- 1969,1991 ----
  		 */
  		if (!read_only)
  			CommandCounterIncrement();
+ 
+ 		/*
+ 		 * If we took a new snapshot for this query list, unregister it and
+ 		 * make sure we take a new one for the next list.
+ 		 */
+ 		if (registered_snap)
+ 		{
+ 			UnregisterSnapshot(snapshot);
+ 			snapshot = InvalidSnapshot;
+ 		}
  	}
  
  fail:
  
+ 	if (registered_snap)
+ 		UnregisterSnapshot(snapshot);
+ 
  	/* We no longer need the cached plan refcount, if any */
  	if (cplan)
  		ReleaseCachedPlan(cplan, true);
*** a/src/backend/tcop/postgres.c
--- b/src/backend/tcop/postgres.c
***************
*** 537,547 **** pg_parse_and_rewrite(const char *query_string,	/* string to execute */
  	{
  		Node	   *parsetree = (Node *) lfirst(list_item);
  
! 		querytree_list = list_concat(querytree_list,
! 									 pg_analyze_and_rewrite(parsetree,
! 															query_string,
! 															paramTypes,
! 															numParams));
  	}
  
  	return querytree_list;
--- 537,547 ----
  	{
  		Node	   *parsetree = (Node *) lfirst(list_item);
  
! 		querytree_list = lappend(querytree_list,
! 								 pg_analyze_and_rewrite(parsetree,
! 														query_string,
! 														paramTypes,
! 														numParams));
  	}
  
  	return querytree_list;
*** a/src/backend/tcop/pquery.c
--- b/src/backend/tcop/pquery.c
***************
*** 170,180 **** ProcessQuery(PlannedStmt *plan,
  	elog(DEBUG3, "ProcessQuery");
  
  	/*
- 	 * Must always set a snapshot for plannable queries.
- 	 */
- 	PushActiveSnapshot(GetTransactionSnapshot());
- 
- 	/*
  	 * Create the QueryDesc object
  	 */
  	queryDesc = CreateQueryDesc(plan, sourceText,
--- 170,175 ----
***************
*** 234,241 **** ProcessQuery(PlannedStmt *plan,
  	/* Now take care of any queued AFTER triggers */
  	AfterTriggerEndQuery(queryDesc->estate);
  
- 	PopActiveSnapshot();
- 
  	/*
  	 * Now, we close down all the scans and free allocated resources.
  	 */
--- 229,234 ----
***************
*** 1220,1225 **** PortalRunMulti(Portal portal, bool isTopLevel,
--- 1213,1219 ----
  			   char *completionTag)
  {
  	ListCell   *stmtlist_item;
+ 	Snapshot	snapshot = InvalidSnapshot;
  
  	/*
  	 * If the destination is DestRemoteExecute, change to DestNone.  The
***************
*** 1262,1267 **** PortalRunMulti(Portal portal, bool isTopLevel,
--- 1256,1270 ----
  			if (log_executor_stats)
  				ResetUsage();
  
+ 			/* if no snapshot is set, grab a new one and register it */
+ 			if (snapshot == InvalidSnapshot)
+ 			{
+ 				snapshot = RegisterSnapshot(GetTransactionSnapshot());
+ 				PushActiveSnapshot(snapshot);
+ 			}
+ 			else
+ 				PushUpdatedSnapshot(snapshot);
+ 
  			if (pstmt->canSetTag)
  			{
  				/* statement can set tag string */
***************
*** 1279,1284 **** PortalRunMulti(Portal portal, bool isTopLevel,
--- 1282,1289 ----
  							 altdest, NULL);
  			}
  
+ 			PopActiveSnapshot();
+ 
  			if (log_executor_stats)
  				ShowUsage("EXECUTOR STATISTICS");
  
***************
*** 1291,1301 **** PortalRunMulti(Portal portal, bool isTopLevel,
--- 1296,1320 ----
  			 *
  			 * These are assumed canSetTag if they're the only stmt in the
  			 * portal.
+ 			 *
+ 			 * NotifyStmt is the only utility statement allowed in a list of
+ 			 * rewritten queries, and it doesn't need a snapshot so we don't
+ 			 * need to worry about it.  However, if the list has only one
+ 			 * statement and it's a utility statement, we are not allowed to
+ 			 * take a snapshot.  See the first comment in PortalRunUtility().
  			 */
  			if (list_length(portal->stmts) == 1)
+ 			{
+ 				Assert(snapshot == InvalidSnapshot);
+ 
  				PortalRunUtility(portal, stmt, isTopLevel, dest, completionTag);
+ 			}
  			else
+ 			{
+ 				Assert(IsA(stmt, NotifyStmt));
+ 
  				PortalRunUtility(portal, stmt, isTopLevel, altdest, NULL);
+ 			}
  		}
  
  		/*
***************
*** 1313,1318 **** PortalRunMulti(Portal portal, bool isTopLevel,
--- 1332,1340 ----
  		MemoryContextDeleteChildren(PortalGetHeapMemory(portal));
  	}
  
+ 	if (snapshot != InvalidSnapshot)
+ 		UnregisterSnapshot(snapshot);
+ 
  	/*
  	 * If a command completion tag was supplied, use it.  Otherwise use the
  	 * portal's commandTag as the default completion tag.
#28Robert Haas
robertmhaas@gmail.com
In reply to: Marko Tiikkaja (#27)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

On Mon, Aug 30, 2010 at 5:30 PM, Marko Tiikkaja
<marko.tiikkaja@cs.helsinki.fi> wrote:

On 2010-08-31 12:07 AM +0300, I wrote:

I looked at fixing this..

The previous patch had a bug in fmgr_sql() our regression tests didn't
catch.  Fixed version attached.

It would probably be a good idea to add this to the currently open CommitFest.

https://commitfest.postgresql.org/action/commitfest_view/open

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

#29Marko Tiikkaja
marko.tiikkaja@cs.helsinki.fi
In reply to: Marko Tiikkaja (#26)
1 attachment(s)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

On 2010-08-31 12:07 AM +0300, I wrote:

The patch needs a bit more comments and some cleaning up ..

Here's a cleaned up version with a bit more comments.

This patch still silently breaks pg_parse_and_rewrite(). We only use it
in our source code for SQL-language functions, so I think we should
deprecate it in favor of a function which would do the right thing for
SQL functions. Thoughts?

Regards,
Marko Tiikkaja

Attachments:

snapshot.patchtext/plain; charset=iso-8859-1; name=snapshot.patch; x-mac-creator=0; x-mac-type=0Download
*** a/src/backend/catalog/pg_proc.c
--- b/src/backend/catalog/pg_proc.c
***************
*** 832,838 **** fmgr_sql_validator(PG_FUNCTION_ARGS)
  												  proc->proargtypes.values,
  												  proc->pronargs);
  			(void) check_sql_fn_retval(funcoid, proc->prorettype,
! 									   querytree_list,
  									   NULL, NULL);
  		}
  		else
--- 832,838 ----
  												  proc->proargtypes.values,
  												  proc->pronargs);
  			(void) check_sql_fn_retval(funcoid, proc->prorettype,
! 									   llast(querytree_list),
  									   NULL, NULL);
  		}
  		else
*** a/src/backend/executor/functions.c
--- b/src/backend/executor/functions.c
***************
*** 90,107 **** typedef struct
  	ParamListInfo paramLI;		/* Param list representing current args */
  
  	Tuplestorestate *tstore;	/* where we accumulate result tuples */
  
  	JunkFilter *junkFilter;		/* will be NULL if function returns VOID */
  
! 	/* head of linked list of execution_state records */
! 	execution_state *func_state;
  } SQLFunctionCache;
  
  typedef SQLFunctionCache *SQLFunctionCachePtr;
  
  
  /* non-export function prototypes */
! static execution_state *init_execution_state(List *queryTree_list,
  					 SQLFunctionCachePtr fcache,
  					 bool lazyEvalOK);
  static void init_sql_fcache(FmgrInfo *finfo, bool lazyEvalOK);
--- 90,107 ----
  	ParamListInfo paramLI;		/* Param list representing current args */
  
  	Tuplestorestate *tstore;	/* where we accumulate result tuples */
+ 	Snapshot	snapshot;
  
  	JunkFilter *junkFilter;		/* will be NULL if function returns VOID */
  
! 	List *func_state;
  } SQLFunctionCache;
  
  typedef SQLFunctionCache *SQLFunctionCachePtr;
  
  
  /* non-export function prototypes */
! static List *init_execution_state(List *queryTree_list,
  					 SQLFunctionCachePtr fcache,
  					 bool lazyEvalOK);
  static void init_sql_fcache(FmgrInfo *finfo, bool lazyEvalOK);
***************
*** 123,183 **** static void sqlfunction_destroy(DestReceiver *self);
  
  
  /* Set up the list of per-query execution_state records for a SQL function */
! static execution_state *
  init_execution_state(List *queryTree_list,
  					 SQLFunctionCachePtr fcache,
  					 bool lazyEvalOK)
  {
! 	execution_state *firstes = NULL;
! 	execution_state *preves = NULL;
  	execution_state *lasttages = NULL;
! 	ListCell   *qtl_item;
  
! 	foreach(qtl_item, queryTree_list)
  	{
! 		Query	   *queryTree = (Query *) lfirst(qtl_item);
! 		Node	   *stmt;
! 		execution_state *newes;
  
! 		Assert(IsA(queryTree, Query));
  
! 		if (queryTree->commandType == CMD_UTILITY)
! 			stmt = queryTree->utilityStmt;
! 		else
! 			stmt = (Node *) pg_plan_query(queryTree, 0, NULL);
  
! 		/* Precheck all commands for validity in a function */
! 		if (IsA(stmt, TransactionStmt))
! 			ereport(ERROR,
! 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! 			/* translator: %s is a SQL statement name */
! 					 errmsg("%s is not allowed in a SQL function",
! 							CreateCommandTag(stmt))));
  
! 		if (fcache->readonly_func && !CommandIsReadOnly(stmt))
! 			ereport(ERROR,
! 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! 			/* translator: %s is a SQL statement name */
! 					 errmsg("%s is not allowed in a non-volatile function",
! 							CreateCommandTag(stmt))));
  
! 		newes = (execution_state *) palloc(sizeof(execution_state));
! 		if (preves)
! 			preves->next = newes;
! 		else
! 			firstes = newes;
  
! 		newes->next = NULL;
! 		newes->status = F_EXEC_START;
! 		newes->setsResult = false;		/* might change below */
! 		newes->lazyEval = false;	/* might change below */
! 		newes->stmt = stmt;
! 		newes->qd = NULL;
  
! 		if (queryTree->canSetTag)
! 			lasttages = newes;
  
! 		preves = newes;
  	}
  
  	/*
--- 123,200 ----
  
  
  /* Set up the list of per-query execution_state records for a SQL function */
! static List *
  init_execution_state(List *queryTree_list,
  					 SQLFunctionCachePtr fcache,
  					 bool lazyEvalOK)
  {
! 	execution_state *firstes;
! 	execution_state *preves;
  	execution_state *lasttages = NULL;
! 	List	   *eslist;
! 	ListCell   *lc1;
! 	ListCell   *lc2;
! 	List	   *qtlist;
! 	Query	   *queryTree;
! 
! 
! 	eslist = NIL;
  
! 	foreach(lc1, queryTree_list)
  	{
! 		qtlist = (List *) lfirst(lc1);
! 		firstes = NULL;
! 		preves = NULL;
  
! 		foreach(lc2, qtlist)
! 		{
! 			Node	   *stmt;
! 			execution_state *newes;
  
! 			queryTree = (Query *) lfirst(lc2);
  
! 			Assert(IsA(queryTree, Query));
  
! 			if (queryTree->commandType == CMD_UTILITY)
! 				stmt = queryTree->utilityStmt;
! 			else
! 				stmt = (Node *) pg_plan_query(queryTree, 0, NULL);
  
! 			/* Precheck all commands for validity in a function */
! 			if (IsA(stmt, TransactionStmt))
! 				ereport(ERROR,
! 						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! 				/* translator: %s is a SQL statement name */
! 						 errmsg("%s is not allowed in a SQL function",
! 								CreateCommandTag(stmt))));
  
! 			if (fcache->readonly_func && !CommandIsReadOnly(stmt))
! 				ereport(ERROR,
! 						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
! 				/* translator: %s is a SQL statement name */
! 						 errmsg("%s is not allowed in a non-volatile function",
! 								CreateCommandTag(stmt))));
! 
! 			newes = (execution_state *) palloc(sizeof(execution_state));
! 			if (preves)
! 				preves->next = newes;
! 			else
! 				firstes = newes;
  
! 			newes->next = NULL;
! 			newes->status = F_EXEC_START;
! 			newes->setsResult = false;		/* might change below */
! 			newes->lazyEval = false;	/* might change below */
! 			newes->stmt = stmt;
! 			newes->qd = NULL;
  
! 			if (queryTree->canSetTag)
! 				lasttages = newes;
! 
! 			preves = newes;
! 		}
! 
! 		eslist = lappend(eslist, firstes);
  	}
  
  	/*
***************
*** 210,216 **** init_execution_state(List *queryTree_list,
  		}
  	}
  
! 	return firstes;
  }
  
  /* Initialize the SQLFunctionCache for a SQL function */
--- 227,233 ----
  		}
  	}
  
! 	return eslist;
  }
  
  /* Initialize the SQLFunctionCache for a SQL function */
***************
*** 342,348 **** init_sql_fcache(FmgrInfo *finfo, bool lazyEvalOK)
  	 */
  	fcache->returnsTuple = check_sql_fn_retval(foid,
  											   rettype,
! 											   queryTree_list,
  											   NULL,
  											   &fcache->junkFilter);
  
--- 359,365 ----
  	 */
  	fcache->returnsTuple = check_sql_fn_retval(foid,
  											   rettype,
! 											   llast(queryTree_list),
  											   NULL,
  											   &fcache->junkFilter);
  
***************
*** 374,397 **** init_sql_fcache(FmgrInfo *finfo, bool lazyEvalOK)
  static void
  postquel_start(execution_state *es, SQLFunctionCachePtr fcache)
  {
- 	Snapshot	snapshot;
  	DestReceiver *dest;
  
  	Assert(es->qd == NULL);
  
! 	/*
! 	 * In a read-only function, use the surrounding query's snapshot;
! 	 * otherwise take a new snapshot for each query.  The snapshot should
! 	 * include a fresh command ID so that all work to date in this transaction
! 	 * is visible.
! 	 */
! 	if (fcache->readonly_func)
! 		snapshot = GetActiveSnapshot();
! 	else
! 	{
! 		CommandCounterIncrement();
! 		snapshot = GetTransactionSnapshot();
! 	}
  
  	/*
  	 * If this query produces the function result, send its output to the
--- 391,401 ----
  static void
  postquel_start(execution_state *es, SQLFunctionCachePtr fcache)
  {
  	DestReceiver *dest;
  
  	Assert(es->qd == NULL);
  
! 	Assert(ActiveSnapshotSet());
  
  	/*
  	 * If this query produces the function result, send its output to the
***************
*** 415,427 **** postquel_start(execution_state *es, SQLFunctionCachePtr fcache)
  	if (IsA(es->stmt, PlannedStmt))
  		es->qd = CreateQueryDesc((PlannedStmt *) es->stmt,
  								 fcache->src,
! 								 snapshot, InvalidSnapshot,
  								 dest,
  								 fcache->paramLI, 0);
  	else
  		es->qd = CreateUtilityQueryDesc(es->stmt,
  										fcache->src,
! 										snapshot,
  										dest,
  										fcache->paramLI);
  
--- 419,432 ----
  	if (IsA(es->stmt, PlannedStmt))
  		es->qd = CreateQueryDesc((PlannedStmt *) es->stmt,
  								 fcache->src,
! 								 GetActiveSnapshot(),
! 								 InvalidSnapshot,
  								 dest,
  								 fcache->paramLI, 0);
  	else
  		es->qd = CreateUtilityQueryDesc(es->stmt,
  										fcache->src,
! 										GetActiveSnapshot(),
  										dest,
  										fcache->paramLI);
  
***************
*** 617,622 **** fmgr_sql(PG_FUNCTION_ARGS)
--- 622,629 ----
  	execution_state *es;
  	TupleTableSlot *slot;
  	Datum		result;
+ 	List		*eslist;
+ 	ListCell    *eslc;
  
  	/*
  	 * Switch to context in which the fcache lives.  This ensures that
***************
*** 668,680 **** fmgr_sql(PG_FUNCTION_ARGS)
  		init_sql_fcache(fcinfo->flinfo, lazyEvalOK);
  		fcache = (SQLFunctionCachePtr) fcinfo->flinfo->fn_extra;
  	}
! 	es = fcache->func_state;
  
  	/*
  	 * Convert params to appropriate format if starting a fresh execution. (If
  	 * continuing execution, we can re-use prior params.)
  	 */
! 	if (es && es->status == F_EXEC_START)
  		postquel_sub_params(fcache, fcinfo);
  
  	/*
--- 675,687 ----
  		init_sql_fcache(fcinfo->flinfo, lazyEvalOK);
  		fcache = (SQLFunctionCachePtr) fcinfo->flinfo->fn_extra;
  	}
! 	eslist = fcache->func_state;
  
  	/*
  	 * Convert params to appropriate format if starting a fresh execution. (If
  	 * continuing execution, we can re-use prior params.)
  	 */
! 	if (linitial(eslist) && ((execution_state *) linitial(eslist))->status == F_EXEC_START)
  		postquel_sub_params(fcache, fcinfo);
  
  	/*
***************
*** 687,694 **** fmgr_sql(PG_FUNCTION_ARGS)
  	/*
  	 * Find first unfinished query in function.
  	 */
! 	while (es && es->status == F_EXEC_DONE)
! 		es = es->next;
  
  	/*
  	 * Execute each command in the function one after another until we either
--- 694,709 ----
  	/*
  	 * Find first unfinished query in function.
  	 */
! 	foreach(eslc, eslist)
! 	{
! 		es = (execution_state *) lfirst(eslc);
! 
! 		while (es && es->status == F_EXEC_DONE)
! 			es = es->next;
! 
! 		if (es)
! 			break;
! 	}
  
  	/*
  	 * Execute each command in the function one after another until we either
***************
*** 699,706 **** fmgr_sql(PG_FUNCTION_ARGS)
--- 714,744 ----
  		bool		completed;
  
  		if (es->status == F_EXEC_START)
+ 		{
+ 			if (!fcache->readonly_func)
+ 			{
+ 				/*
+ 				 * In a read-only function, use the surrounding query's snapshot;
+ 				 * otherwise take a new snapshot if we don't have one yet.  The
+ 				 * snapshot should include a fresh command ID so that all work to
+ 				 * date in this transaction is visible.
+ 				 */
+ 				if (!fcache->snapshot)
+ 				{
+ 					CommandCounterIncrement();
+ 					fcache->snapshot = RegisterSnapshot(GetTransactionSnapshot());
+ 					PushActiveSnapshot(fcache->snapshot);
+ 				}
+ 				else
+ 					PushUpdatedSnapshot(fcache->snapshot);
+ 			}
+ 			
  			postquel_start(es, fcache);
  
+ 			if (!fcache->readonly_func)
+ 				PopActiveSnapshot();
+ 		}
+ 
  		completed = postquel_getnext(es, fcache);
  
  		/*
***************
*** 726,731 **** fmgr_sql(PG_FUNCTION_ARGS)
--- 764,788 ----
  		if (es->status != F_EXEC_DONE)
  			break;
  		es = es->next;
+ 
+ 		if (!es)
+ 		{
+ 			eslc = lnext(eslc);
+ 			if (!eslc)
+ 				break;
+ 
+ 			es = (execution_state *) lfirst(eslc);
+ 
+ 			/* make sure we take a new snapshot for this query list */
+ 			if (!fcache->readonly_func)
+ 			{
+ 				Assert(fcache->snapshot != InvalidSnapshot);
+ 				UnregisterSnapshot(fcache->snapshot);
+ 				fcache->snapshot = InvalidSnapshot;
+ 			}
+ 			else
+ 				Assert(fcache->snapshot == InvalidSnapshot);
+ 		}
  	}
  
  	/*
***************
*** 794,799 **** fmgr_sql(PG_FUNCTION_ARGS)
--- 851,861 ----
  											  PointerGetDatum(fcache));
  				fcache->shutdown_reg = false;
  			}
+ 
+ 			/* Unregister snapshot if we have one */
+ 			if (fcache->snapshot != InvalidSnapshot)
+ 				UnregisterSnapshot(fcache->snapshot);
+ 			fcache->snapshot = InvalidSnapshot;
  		}
  		else
  		{
***************
*** 820,825 **** fmgr_sql(PG_FUNCTION_ARGS)
--- 882,892 ----
  											  PointerGetDatum(fcache));
  				fcache->shutdown_reg = false;
  			}
+ 
+ 			/* Unregister snapshot if we have one */
+ 			if (fcache->snapshot != InvalidSnapshot)
+ 				UnregisterSnapshot(fcache->snapshot);
+ 			fcache->snapshot = InvalidSnapshot;
  		}
  	}
  	else
***************
*** 850,855 **** fmgr_sql(PG_FUNCTION_ARGS)
--- 917,927 ----
  
  		/* Clear the tuplestore, but keep it for next time */
  		tuplestore_clear(fcache->tstore);
+ 
+ 		/* Unregister snapshot if we have one */
+ 		if (fcache->snapshot != InvalidSnapshot)
+ 			UnregisterSnapshot(fcache->snapshot);
+ 		fcache->snapshot = InvalidSnapshot;
  	}
  
  	/*
***************
*** 858,868 **** fmgr_sql(PG_FUNCTION_ARGS)
  	 */
  	if (es == NULL)
  	{
! 		es = fcache->func_state;
! 		while (es)
  		{
! 			es->status = F_EXEC_START;
! 			es = es->next;
  		}
  	}
  
--- 930,943 ----
  	 */
  	if (es == NULL)
  	{
! 		foreach(eslc, fcache->func_state)
  		{
! 			es = (execution_state *) lfirst(eslc);
! 			while (es)
! 			{
! 				es->status = F_EXEC_START;
! 				es = es->next;
! 			}
  		}
  	}
  
***************
*** 913,931 **** sql_exec_error_callback(void *arg)
  	{
  		execution_state *es;
  		int			query_num;
  
- 		es = fcache->func_state;
  		query_num = 1;
! 		while (es)
  		{
! 			if (es->qd)
  			{
! 				errcontext("SQL function \"%s\" statement %d",
! 						   fcache->fname, query_num);
! 				break;
  			}
- 			es = es->next;
- 			query_num++;
  		}
  		if (es == NULL)
  		{
--- 988,1011 ----
  	{
  		execution_state *es;
  		int			query_num;
+ 		ListCell   *lc;
  
  		query_num = 1;
! 
! 		foreach(lc, fcache->func_state)
  		{
! 			es = (execution_state *) lfirst(lc);
! 			while (es)
  			{
! 				if (es->qd)
! 				{
! 					errcontext("SQL function \"%s\" statement %d",
! 							   fcache->fname, query_num);
! 					break;
! 				}
! 				es = es->next;
! 				query_num++;
  			}
  		}
  		if (es == NULL)
  		{
***************
*** 956,973 **** static void
  ShutdownSQLFunction(Datum arg)
  {
  	SQLFunctionCachePtr fcache = (SQLFunctionCachePtr) DatumGetPointer(arg);
! 	execution_state *es = fcache->func_state;
  
! 	while (es != NULL)
  	{
! 		/* Shut down anything still running */
! 		if (es->status == F_EXEC_RUN)
! 			postquel_end(es);
! 		/* Reset states to START in case we're called again */
! 		es->status = F_EXEC_START;
! 		es = es->next;
  	}
  
  	/* Release tuplestore if we have one */
  	if (fcache->tstore)
  		tuplestore_end(fcache->tstore);
--- 1036,1064 ----
  ShutdownSQLFunction(Datum arg)
  {
  	SQLFunctionCachePtr fcache = (SQLFunctionCachePtr) DatumGetPointer(arg);
! 	execution_state *es;
! 	ListCell		*lc;
  
! 	foreach(lc, fcache->func_state)
  	{
! 		es = (execution_state *) lfirst(lc);
! 
! 		while (es)
! 		{
! 			/* Shut down anything still running */
! 			if (es->status == F_EXEC_RUN)
! 				postquel_end(es);
! 			/* Reset states to START in case we're called again */
! 			es->status = F_EXEC_START;
! 			es = es->next;
! 		}
  	}
  
+ 	/* Unregister snapshot if we have one */
+ 	if (fcache->snapshot != InvalidSnapshot)
+ 		UnregisterSnapshot(fcache->snapshot);
+ 	fcache->snapshot = InvalidSnapshot;
+ 
  	/* Release tuplestore if we have one */
  	if (fcache->tstore)
  		tuplestore_end(fcache->tstore);
*** a/src/backend/executor/spi.c
--- b/src/backend/executor/spi.c
***************
*** 1769,1774 **** _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
--- 1769,1775 ----
  	SPITupleTable *my_tuptable = NULL;
  	int			res = 0;
  	bool		have_active_snap = ActiveSnapshotSet();
+ 	bool		registered_snap = false;
  	ErrorContextCallback spierrcontext;
  	CachedPlan *cplan = NULL;
  	ListCell   *lc1;
***************
*** 1872,1879 **** _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
  				}
  				else
  				{
! 					PushActiveSnapshot(GetTransactionSnapshot());
  					pushed_active_snap = true;
  				}
  			}
  			else
--- 1873,1882 ----
  				}
  				else
  				{
! 					snapshot = RegisterSnapshot(GetTransactionSnapshot());
! 					PushActiveSnapshot(snapshot);
  					pushed_active_snap = true;
+ 					registered_snap = true;
  				}
  			}
  			else
***************
*** 1966,1975 **** _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
--- 1969,1991 ----
  		 */
  		if (!read_only)
  			CommandCounterIncrement();
+ 
+ 		/*
+ 		 * If we took a new snapshot for this query list, unregister it and
+ 		 * make sure we take a new one for the next list.
+ 		 */
+ 		if (registered_snap)
+ 		{
+ 			UnregisterSnapshot(snapshot);
+ 			snapshot = InvalidSnapshot;
+ 		}
  	}
  
  fail:
  
+ 	if (registered_snap)
+ 		UnregisterSnapshot(snapshot);
+ 
  	/* We no longer need the cached plan refcount, if any */
  	if (cplan)
  		ReleaseCachedPlan(cplan, true);
*** a/src/backend/tcop/postgres.c
--- b/src/backend/tcop/postgres.c
***************
*** 537,547 **** pg_parse_and_rewrite(const char *query_string,	/* string to execute */
  	{
  		Node	   *parsetree = (Node *) lfirst(list_item);
  
! 		querytree_list = list_concat(querytree_list,
! 									 pg_analyze_and_rewrite(parsetree,
! 															query_string,
! 															paramTypes,
! 															numParams));
  	}
  
  	return querytree_list;
--- 537,547 ----
  	{
  		Node	   *parsetree = (Node *) lfirst(list_item);
  
! 		querytree_list = lappend(querytree_list,
! 								 pg_analyze_and_rewrite(parsetree,
! 														query_string,
! 														paramTypes,
! 														numParams));
  	}
  
  	return querytree_list;
*** a/src/backend/tcop/pquery.c
--- b/src/backend/tcop/pquery.c
***************
*** 170,180 **** ProcessQuery(PlannedStmt *plan,
  	elog(DEBUG3, "ProcessQuery");
  
  	/*
- 	 * Must always set a snapshot for plannable queries.
- 	 */
- 	PushActiveSnapshot(GetTransactionSnapshot());
- 
- 	/*
  	 * Create the QueryDesc object
  	 */
  	queryDesc = CreateQueryDesc(plan, sourceText,
--- 170,175 ----
***************
*** 234,241 **** ProcessQuery(PlannedStmt *plan,
  	/* Now take care of any queued AFTER triggers */
  	AfterTriggerEndQuery(queryDesc->estate);
  
- 	PopActiveSnapshot();
- 
  	/*
  	 * Now, we close down all the scans and free allocated resources.
  	 */
--- 229,234 ----
***************
*** 1220,1225 **** PortalRunMulti(Portal portal, bool isTopLevel,
--- 1213,1219 ----
  			   char *completionTag)
  {
  	ListCell   *stmtlist_item;
+ 	Snapshot	snapshot = InvalidSnapshot;
  
  	/*
  	 * If the destination is DestRemoteExecute, change to DestNone.  The
***************
*** 1262,1267 **** PortalRunMulti(Portal portal, bool isTopLevel,
--- 1256,1270 ----
  			if (log_executor_stats)
  				ResetUsage();
  
+ 			/* if no snapshot is set, grab a new one and register it */
+ 			if (snapshot == InvalidSnapshot)
+ 			{
+ 				snapshot = RegisterSnapshot(GetTransactionSnapshot());
+ 				PushActiveSnapshot(snapshot);
+ 			}
+ 			else
+ 				PushUpdatedSnapshot(snapshot);
+ 
  			if (pstmt->canSetTag)
  			{
  				/* statement can set tag string */
***************
*** 1279,1284 **** PortalRunMulti(Portal portal, bool isTopLevel,
--- 1282,1289 ----
  							 altdest, NULL);
  			}
  
+ 			PopActiveSnapshot();
+ 
  			if (log_executor_stats)
  				ShowUsage("EXECUTOR STATISTICS");
  
***************
*** 1291,1301 **** PortalRunMulti(Portal portal, bool isTopLevel,
--- 1296,1320 ----
  			 *
  			 * These are assumed canSetTag if they're the only stmt in the
  			 * portal.
+ 			 *
+ 			 * NotifyStmt is the only utility statement allowed in a list of
+ 			 * rewritten queries, and it doesn't need a snapshot so we don't
+ 			 * need to worry about it.  However, if the list has only one
+ 			 * statement and it's a utility statement, we are not allowed to
+ 			 * take a snapshot.  See the first comment in PortalRunUtility().
  			 */
  			if (list_length(portal->stmts) == 1)
+ 			{
+ 				Assert(snapshot == InvalidSnapshot);
+ 
  				PortalRunUtility(portal, stmt, isTopLevel, dest, completionTag);
+ 			}
  			else
+ 			{
+ 				Assert(IsA(stmt, NotifyStmt));
+ 
  				PortalRunUtility(portal, stmt, isTopLevel, altdest, NULL);
+ 			}
  		}
  
  		/*
***************
*** 1313,1318 **** PortalRunMulti(Portal portal, bool isTopLevel,
--- 1332,1340 ----
  		MemoryContextDeleteChildren(PortalGetHeapMemory(portal));
  	}
  
+ 	if (snapshot != InvalidSnapshot)
+ 		UnregisterSnapshot(snapshot);
+ 
  	/*
  	 * If a command completion tag was supplied, use it.  Otherwise use the
  	 * portal's commandTag as the default completion tag.
#30Bruce Momjian
bruce@momjian.us
In reply to: Tom Lane (#25)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

Tom Lane wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Aug 4, 2010 at 2:45 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I seriously doubt that there are many applications out there that are
actually depending on this aspect of rule execution; if anything, there
are probably more that will see it as a bug.

Changing EXPLAIN ANALYZE seems a bit less likely to break things for
anyone depending on current behavior;

Well, the point I was trying to make is that there may well be fewer
people depending on the current behavior than there are people for whom
the current behavior is wrong, only they don't know it because they've
not seen a failure (or not seen one often enough to diagnose what's
happening).

This is of course merest speculation either way. But I don't feel that
we need to necessarily treat rule behavior as graven in stone.

Where are we on this? It seems it is an issue independent of writable
common table expressions (wCTEs).

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

#31Robert Haas
robertmhaas@gmail.com
In reply to: Bruce Momjian (#30)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

On Thu, Feb 17, 2011 at 1:04 PM, Bruce Momjian <bruce@momjian.us> wrote:

Tom Lane wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Aug 4, 2010 at 2:45 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I seriously doubt that there are many applications out there that are
actually depending on this aspect of rule execution; if anything, there
are probably more that will see it as a bug.

Changing EXPLAIN ANALYZE seems a bit less likely to break things for
anyone depending on current behavior;

Well, the point I was trying to make is that there may well be fewer
people depending on the current behavior than there are people for whom
the current behavior is wrong, only they don't know it because they've
not seen a failure (or not seen one often enough to diagnose what's
happening).

This is of course merest speculation either way.  But I don't feel that
we need to necessarily treat rule behavior as graven in stone.

Where are we on this?  It seems it is an issue independent of writable
common table expressions (wCTEs).

I believe that it's the same issue as this patch:

https://commitfest.postgresql.org/action/patch_view?id=377

The status of that patch is that Tom promised to look at it two months
ago and hasn't. It would be nice if someone else could pick it up.
It's not good for our community to ignore patches that people have
taken the trouble to write and submit.

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

#32Bruce Momjian
bruce@momjian.us
In reply to: Robert Haas (#31)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

Robert Haas wrote:

On Thu, Feb 17, 2011 at 1:04 PM, Bruce Momjian <bruce@momjian.us> wrote:

Tom Lane wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Aug 4, 2010 at 2:45 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I seriously doubt that there are many applications out there that are
actually depending on this aspect of rule execution; if anything, there
are probably more that will see it as a bug.

Changing EXPLAIN ANALYZE seems a bit less likely to break things for
anyone depending on current behavior;

Well, the point I was trying to make is that there may well be fewer
people depending on the current behavior than there are people for whom
the current behavior is wrong, only they don't know it because they've
not seen a failure (or not seen one often enough to diagnose what's
happening).

This is of course merest speculation either way. ?But I don't feel that
we need to necessarily treat rule behavior as graven in stone.

Where are we on this? ?It seems it is an issue independent of writable
common table expressions (wCTEs).

I believe that it's the same issue as this patch:

https://commitfest.postgresql.org/action/patch_view?id=377

The status of that patch is that Tom promised to look at it two months
ago and hasn't. It would be nice if someone else could pick it up.
It's not good for our community to ignore patches that people have
taken the trouble to write and submit.

Oh, at least it is in the current commit-fest and was not lost.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

#33Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#31)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

Robert Haas <robertmhaas@gmail.com> writes:

The status of that patch is that Tom promised to look at it two months
ago and hasn't. It would be nice if someone else could pick it up.

I'm pedaling as fast as I can, honestly.

regards, tom lane

#34David E. Wheeler
david@kineticode.com
In reply to: Tom Lane (#33)
Re: Rewrite, normal execution vs. EXPLAIN ANALYZE

On Feb 17, 2011, at 11:28 AM, Tom Lane wrote:

The status of that patch is that Tom promised to look at it two months
ago and hasn't. It would be nice if someone else could pick it up.

I'm pedaling as fast as I can, honestly

Tom leaves everything on the road.

http://en.wiktionary.org/wiki/leave_everything_on_the_road

David