Horton

Thread

  1. Review: Fix snapshot taking inconsistencies

    Steve Singer <ssinger_pg@sympatico.ca> — 2010-10-03T02:08:40Z

    This is my review on the "Fix snapshot taking inconsistencies patch".

The patch applies against master (a13f12b3a18da0a61571cb134fdecea03a10d6f)


However initdb fails with:

FATAL:  return type mismatch in function declared to return record
DETAIL:  Function's final statement must be SELECT or INSERT/UPDATE/DELETE 
RETURNING.
CONTEXT:  SQL function "ts_debug"

If I run initdb without the patch applied then apply the patch I can test 
the patch.  However the regression tests won't run with the patch applied 
because they call initdb.


Submission:
============
The patch does not include any documentation updates.  I don't feel they
are required.  I can't find anywhere in the documentation where it says
to expect the current behavior.

The patch does not include any regression tests. I don't think we have other 
tests that (can?) test concurrency patterns like this.


Usability review:
=================
The original thread on hackers debated between changing the behavior of 
explain vs changing how rules get executed (so they get a new snapshot). The 
consensus seemed to be to leave explain analyze alone and change the current 
behavior for rules.  This is the route this patch took.

Are there any dangers:  Per Marko's comments on the most recent patch:
"This patch still silently breaks  pg_parse_and_rewrite()..." this still 
seems unresolved.  Marko proposed replacing this with something new for SQL 
functions.  Unfortunately I don't see this as having been followed up on.

I also don't have enough understanding of the code to see exactly how/why it 
was broken or what would be involved in fixing it. I don't know if this is 
why initdb is failing.

With the patch applied SQL functions still can see changes made by other 
transactions after the function starts (ie a function select 
$$ pg_sleep(5);insert into bar select * FROM baz;$$  inserts the row into 
bar both with and without the patch applied.  This is how I would expect a 
function (in SQL or any pl) to work.


Does this patch effect anything outside of rules execution?  Not that I've 
been able to find but I'm probably not qualified to answer that and the 
regression tests don't work.


Performance Review
------------------
I don't see this patch impacting performance

Coding Review
------------------
Coding guidelines: no issues that I can find
Portability: no issues
Windows: not tested but I don't see anything that looks suspicious
Comments: no obvious places that require more comments. I don't feel I have 
a good enough handle on the code to judge the accuracy of the comments.
Does it do what it says: yes
compiler warnings: no
can I make it crash: initdb issue.

  2. Re: Review: Fix snapshot taking inconsistencies

    Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2010-10-03T22:27:06Z

    On 2010-10-03 5:08 AM +0300, Steve Singer wrote:
> The patch applies against master (a13f12b3a18da0a61571cb134fdecea03a10d6f)
>
>
> However initdb fails with:
>
> FATAL:  return type mismatch in function declared to return record
> DETAIL:  Function's final statement must be SELECT or INSERT/UPDATE/DELETE
> RETURNING.
> CONTEXT:  SQL function "ts_debug"
>
> If I run initdb without the patch applied then apply the patch I can test
> the patch.  However the regression tests won't run with the patch applied
> because they call initdb.

Hmm..  I can't reproduce this.  What platform are you on?

> The patch does not include any regression tests. I don't think we have other
> tests that (can?) test concurrency patterns like this.

Right, we can't, at least not yet.

> Are there any dangers:  Per Marko's comments on the most recent patch:
> "This patch still silently breaks  pg_parse_and_rewrite()..." this still
> seems unresolved.  Marko proposed replacing this with something new for SQL
> functions.  Unfortunately I don't see this as having been followed up on.
>
> I also don't have enough understanding of the code to see exactly how/why it
> was broken or what would be involved in fixing it.

Currently pg_parse_and_rewrite() returns all Query nodes in one huge 
list.  That's not acceptable for this patch since that list is already 
missing the information we need: when should we take a new snapshot?  So 
the patch breaks the API of pg_parse_and_rewrite() to return a list of 
lists instead, but I'm not convinced that's a bright idea since third 
party code might use it, so I suggested adding a new function.  Then 
again, third party code can't use pg_parse_and_rewrite() any way if/when 
the wCTE patch goes in.


Regards,
Marko Tiikkaja

  3. Re: Review: Fix snapshot taking inconsistencies

    Steve Singer <ssinger_pg@sympatico.ca> — 2010-10-04T03:19:56Z

    On Mon, 4 Oct 2010, Marko Tiikkaja wrote:

> On 2010-10-03 5:08 AM +0300, Steve Singer wrote:

>
> Hmm..  I can't reproduce this.  What platform are you on?

Sorry, I it seems the changes to one file (pg_proc.c) didn't get applied to 
my source repository.  Now that I've applied them initdb works and the 
regression tests pass.

I also noticed that functions.c is now generating a warning that should be 
easy to clean up.

functions.c: In function 'sql_exec_error_callback':
functions.c:989: warning: 'es' may be used uninitialized in this function
functions.c: In function 'fmgr_sql':
functions.c:712: warning: 'es' is used uninitialized in this function

> Currently pg_parse_and_rewrite() returns all Query nodes in one huge list. 
> That's not acceptable for this patch since that list is already missing the 
> information we need: when should we take a new snapshot?  So the patch breaks 
> the API of pg_parse_and_rewrite() to return a list of lists instead, but I'm 
> not convinced that's a bright idea since third party code might use it, so I 
> suggested adding a new function.  Then again, third party code can't use 
> pg_parse_and_rewrite() any way if/when the wCTE patch goes in.
>

Is there any third party code in particular that your thinking of?  I don't 
see anything that says pg_parse_and_rewrite is part of a stable server 
side API (in contrast to SPI or something an third party index access method 
or custom data-type would call).

>
> Regards,
> Marko Tiikkaja
>

Steve Singer

  4. Re: Review: Fix snapshot taking inconsistencies

    Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2010-10-04T09:28:05Z

    On 2010-10-04 6:19 AM, Steve Singer wrote:
> I also noticed that functions.c is now generating a warning that should be
> easy to clean up.
>
> functions.c: In function 'sql_exec_error_callback':
> functions.c:989: warning: 'es' may be used uninitialized in this function
> functions.c: In function 'fmgr_sql':
> functions.c:712: warning: 'es' is used uninitialized in this function

Those didn't look like actual bugs to me, but fixed in the attached 
patch.  Thanks.

>> Currently pg_parse_and_rewrite() returns all Query nodes in one huge list.
>> That's not acceptable for this patch since that list is already missing the
>> information we need: when should we take a new snapshot?  So the patch breaks
>> the API of pg_parse_and_rewrite() to return a list of lists instead, but I'm
>> not convinced that's a bright idea since third party code might use it, so I
>> suggested adding a new function.  Then again, third party code can't use
>> pg_parse_and_rewrite() any way if/when the wCTE patch goes in.
>>
>
> Is there any third party code in particular that your thinking of?  I don't
> see anything that says pg_parse_and_rewrite is part of a stable server
> side API (in contrast to SPI or something an third party index access method
> or custom data-type would call).

Nope.  I think I grepped contrib/ at some point and none of those were 
using pg_parse_and_rewrite() so this is all just speculation.  And yes, 
it's not really part of any stable API but breaking third party modules 
needlessly is not something I want to do.  However, in this case there 
is no way to avoid breaking them.

My primary concern is that any module using pg_parse_and_rewrite() will 
more or less silently break once this patch goes in no matter what we 
do.  If we leave pg_parse_and_rewrite as is, they will do the wrong 
thing (grab a new snapshot for every rewrite product).  The problem 
might not be noticeable (no reports of EXPLAIN ANALYZE behaving 
differently for several years), but once the wCTE patch gets in, it will 
be.  If we modify pg_parse_and_rewrite like the patch does, modules 
start behaving weirdly.  So what I'm suggesting is:

   - Deprecate pg_parse_and_rewrite().  I have no idea how the project
     has done this in the past, but grepping the source code for
     "deprecated" suggests that we just remove the function.

   - Introduce a new function, specifically designed for SQL functions.
     Both callers of pg_parse_and_rewrite (init_sql_fcache and
     fmgr_sql_validator) call check_sql_fn_retval after
     pg_parse_and_rewrite so I think we could merge those into the new
     function.

Does anyone have any concerns about this?  Better ideas?


Regards,
Marko Tiikkaja

  5. Re: Review: Fix snapshot taking inconsistencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-10-04T14:31:26Z

    Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> writes:
> On 2010-10-04 6:19 AM, Steve Singer wrote:
>> Is there any third party code in particular that your thinking of?  I don't
>> see anything that says pg_parse_and_rewrite is part of a stable server
>> side API (in contrast to SPI or something an third party index access method
>> or custom data-type would call).

> Nope.  I think I grepped contrib/ at some point and none of those were 
> using pg_parse_and_rewrite() so this is all just speculation.  And yes, 
> it's not really part of any stable API but breaking third party modules 
> needlessly is not something I want to do.  However, in this case there 
> is no way to avoid breaking them.

The important thing in such cases is to not break third-party code
*silently*.  You want to make sure that they'll get a compilation
error if they haven't adjusted their code.  Change the parameter
list or invent a new name for the function.

In the particular case at hand here, I rather wonder why SQL functions
are depending on postgres.c at all.  It might be better to just
duplicate a bit of code to make them independent.  pg_parse_and_rewrite
would then be dead code and could be deleted.

			regards, tom lane

  6. Re: Review: Fix snapshot taking inconsistencies

    Steve Singer <ssinger_pg@sympatico.ca> — 2010-10-07T02:21:51Z

    On Mon, 4 Oct 2010, Marko Tiikkaja wrote:

> the patch does, modules start behaving weirdly.  So what I'm suggesting is:
>
>  - Deprecate pg_parse_and_rewrite().  I have no idea how the project
>    has done this in the past, but grepping the source code for
>    "deprecated" suggests that we just remove the function.
>
>  - Introduce a new function, specifically designed for SQL functions.
>    Both callers of pg_parse_and_rewrite (init_sql_fcache and
>    fmgr_sql_validator) call check_sql_fn_retval after
>    pg_parse_and_rewrite so I think we could merge those into the new
>    function.
>
> Does anyone have any concerns about this?  Better ideas?

The only comment I've seen on this was from Tom and his only concern was 
that old code wouldn't be able to compile against a new version of the 
function.  What you propose (delete pg_parse_and_rewrite) and replacing it 
with a function of the new name sounds fine.

Since no one else has proposed a better idea and the commit fest is ticking 
away I think you should go ahead and do that.



>
>
> Regards,
> Marko Tiikkaja

Steve

  7. Re: Review: Fix snapshot taking inconsistencies

    Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2010-10-08T23:07:25Z

    On 2010-10-04 5:31 PM +0300, Tom Lane wrote:
> Marko Tiikkaja<marko.tiikkaja@cs.helsinki.fi>  writes:
>> Nope.  I think I grepped contrib/ at some point and none of those were
>> using pg_parse_and_rewrite() so this is all just speculation.  And yes,
>> it's not really part of any stable API but breaking third party modules
>> needlessly is not something I want to do.  However, in this case there
>> is no way to avoid breaking them.
>
> In the particular case at hand here, I rather wonder why SQL functions
> are depending on postgres.c at all.  It might be better to just
> duplicate a bit of code to make them independent.  pg_parse_and_rewrite
> would then be dead code and could be deleted.

I'm confused.  Even if we get rid of pg_parse_and_rewrite, SQL functions 
need pg_parse_query and pg_analyze_and_rewrite from postgres.c.  You're 
not suggesting duplicating the code in those two, are you?


Regards,
Marko Tiikkaja

  8. Re: Review: Fix snapshot taking inconsistencies

    Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2010-10-10T11:48:17Z

    On 2010-10-07 5:21 AM +0300, Steve Singer wrote:
> Since no one else has proposed a better idea and the commit fest is ticking
> away I think you should go ahead and do that.

Here's a new version of the patch, deprecating pg_parse_and_rewrite.

I duplicated the parse/rewrite logic in the two places where 
pg_parse_and_rewrite is currently used, per comment from Tom.


Regards,
Marko Tiikkaja

  9. Re: Review: Fix snapshot taking inconsistencies

    Steve Singer <ssinger_pg@sympatico.ca> — 2010-10-12T00:50:25Z

    On Sun, 10 Oct 2010, Marko Tiikkaja wrote:

> On 2010-10-07 5:21 AM +0300, Steve Singer wrote:
>> Since no one else has proposed a better idea and the commit fest is ticking
>> away I think you should go ahead and do that.
>
> Here's a new version of the patch, deprecating pg_parse_and_rewrite.
>
> I duplicated the parse/rewrite logic in the two places where 
> pg_parse_and_rewrite is currently used, per comment from Tom.

That looks fine.

I've marking the patch as ready for a committer.


>
>
> Regards,
> Marko Tiikkaja
>

  10. Re: Review: Fix snapshot taking inconsistencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-10-12T22:21:32Z

    Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> writes:
> Here's a new version of the patch, deprecating pg_parse_and_rewrite.

I started looking at this patch, and I'm wondering why you inserted all
the Register/UnregisterSnapshot calls that weren't there before (eg, why
did spi.c have to change at all?).  ISTM either these are not necessary,
or there is a pre-existing snapshot management bug that's independent
of the question of just when to take new snapshots.  I couldn't find
anything in the thread claiming the latter though.

			regards, tom lane

  11. Re: Review: Fix snapshot taking inconsistencies

    Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2010-10-12T22:36:30Z

    On 2010-10-13 1:21 AM +0300, Tom Lane wrote:
> Marko Tiikkaja<marko.tiikkaja@cs.helsinki.fi>  writes:
>> Here's a new version of the patch, deprecating pg_parse_and_rewrite.
>
> I started looking at this patch, and I'm wondering why you inserted all
> the Register/UnregisterSnapshot calls that weren't there before (eg, why
> did spi.c have to change at all?).  ISTM either these are not necessary,
> or there is a pre-existing snapshot management bug that's independent
> of the question of just when to take new snapshots.  I couldn't find
> anything in the thread claiming the latter though.

That's actually just my ignorance I forgot to mention.  As I understand 
it, our code currently first pushes one snapshot and then does multiple 
PushActiveSnapshot (or PushUpdatedSnapshot)/PopActiveSnapshot rounds 
before popping the oldest snapshot off the stack (and releasing it).  So 
in the patch, I would've had to push the snapshot twice the first time 
to avoid it being released.

Thinking about it now, that might be a better option.  Or maybe we 
should change the snapshot API to make this more convenient?

The spi.c change also changes the logic; the SPI code currently takes a 
new snapshot for every query if the caller doesn't provide a snapshot.


Regards,
Marko Tiikkaja

  12. Re: Review: Fix snapshot taking inconsistencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-10-12T23:10:08Z

    Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> writes:
> On 2010-10-13 1:21 AM +0300, Tom Lane wrote:
>> I started looking at this patch, and I'm wondering why you inserted all
>> the Register/UnregisterSnapshot calls that weren't there before

> That's actually just my ignorance I forgot to mention.  As I understand 
> it, our code currently first pushes one snapshot and then does multiple 
> PushActiveSnapshot (or PushUpdatedSnapshot)/PopActiveSnapshot rounds 
> before popping the oldest snapshot off the stack (and releasing it).  So 
> in the patch, I would've had to push the snapshot twice the first time 
> to avoid it being released.

It looks to me like you've added quite a lot of management overhead that
wasn't there before.  Wouldn't it be better to just not pop the snapshot
till you're done with it?

> Thinking about it now, that might be a better option.  Or maybe we 
> should change the snapshot API to make this more convenient?

Well, I'm not in love with the current snapshot API by any means,
particularly not PushUpdatedSnapshot which seems to be the only
API-sanctioned way to put a new CID into a snapshot without taking
a whole new snapshot.  It'd be better if the logic was something
along the lines of:

	* at start of a query, PushActiveSnapshot(GetTransactionSnapshot()).

	* between commands of a query, CommandCounterIncrement and
	  then directly modify the curcid of the active snapshot;
	  AFAICS there's no reason to make another copy of it at this
	  point.  Especially not if we can see it has refcount 1.

	* at end of query, PopActiveSnapshot().

where a "query" is whatever we think the unit of noticing commits by
other backends ought to be.

> The spi.c change also changes the logic; the SPI code currently takes a 
> new snapshot for every query if the caller doesn't provide a snapshot.

[ squint... ]  Oh.  I see now, but that is horribly ugly and
underdocumented.  The code was previously treating the snapshot argument
as a constant and relying on that constant value to tell it what to do
each time through the loop.  Now you've got it changing the flag and
then changing it back sometime later.  Ick.

I think what you need to do to make this understandable is to move the
snapshot push/pop logic outside the per-command loop, instead of hacking
things around to keep it exactly where it was before.  We may well need
to adjust the API of snapmgr.c to make that sane.

BTW, this patch seems to be also the time to remove the AtStart_Cache()
call in CommandCounterIncrement, as foreseen in the comment there.

			regards, tom lane

  13. Re: Review: Fix snapshot taking inconsistencies

    Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2010-10-12T23:33:44Z

    On 2010-10-13 2:10 AM +0300, Tom Lane wrote:
> Marko Tiikkaja<marko.tiikkaja@cs.helsinki.fi>  writes:
>> That's actually just my ignorance I forgot to mention.  As I understand
>> it, our code currently first pushes one snapshot and then does multiple
>> PushActiveSnapshot (or PushUpdatedSnapshot)/PopActiveSnapshot rounds
>> before popping the oldest snapshot off the stack (and releasing it).  So
>> in the patch, I would've had to push the snapshot twice the first time
>> to avoid it being released.
>
> It looks to me like you've added quite a lot of management overhead that
> wasn't there before.  Wouldn't it be better to just not pop the snapshot
> till you're done with it?

Yes, you're absolutely right.

> It'd be better if the logic was something
> along the lines of:

That's exactly what I had in mind, so +1.

>> The spi.c change also changes the logic; the SPI code currently takes a
>> new snapshot for every query if the caller doesn't provide a snapshot.
>
> [ squint... ]  Oh.  I see now, but that is horribly ugly and
> underdocumented.  The code was previously treating the snapshot argument
> as a constant and relying on that constant value to tell it what to do
> each time through the loop.  Now you've got it changing the flag and
> then changing it back sometime later.  Ick.
>
> I think what you need to do to make this understandable is to move the
> snapshot push/pop logic outside the per-command loop, instead of hacking
> things around to keep it exactly where it was before.  We may well need
> to adjust the API of snapmgr.c to make that sane.

*blushes*

Yeah, that makes a lot more sense.

> BTW, this patch seems to be also the time to remove the AtStart_Cache()
> call in CommandCounterIncrement, as foreseen in the comment there.

Frankly, I have no idea what to do about this.


Regards,
Marko Tiikkaja

  14. Re: Review: Fix snapshot taking inconsistencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-10-12T23:49:28Z

    Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> writes:
> On 2010-10-13 2:10 AM +0300, Tom Lane wrote:
>> BTW, this patch seems to be also the time to remove the AtStart_Cache()
>> call in CommandCounterIncrement, as foreseen in the comment there.

> Frankly, I have no idea what to do about this.

Just delete the call.  The only reason I didn't remove it in 2007 is
I was afraid to risk changing things in late beta; but that's not the
situation now.

			regards, tom lane

  15. Re: Review: Fix snapshot taking inconsistencies

    Alvaro Herrera <alvherre@commandprompt.com> — 2010-10-17T03:15:07Z

    Excerpts from Tom Lane's message of mar oct 12 20:49:28 -0300 2010:
> Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> writes:
> > On 2010-10-13 2:10 AM +0300, Tom Lane wrote:
> >> BTW, this patch seems to be also the time to remove the AtStart_Cache()
> >> call in CommandCounterIncrement, as foreseen in the comment there.
> 
> > Frankly, I have no idea what to do about this.
> 
> Just delete the call.  The only reason I didn't remove it in 2007 is
> I was afraid to risk changing things in late beta; but that's not the
> situation now.

I just applied just this change and ran the regression tests; it works
fine.  I didn't do anything else though, like the cache-clobber-always
flag, etc.  If no one objects I will push this patch to see what the
buildfarm has to say about it.

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index b02db9e..d2e2e11 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -729,17 +729,6 @@ CommandCounterIncrement(void)
         */
        AtCCI_LocalCache();
    }
-
-   /*
-    * Make any other backends' catalog changes visible to me.
-    *
-    * XXX this is probably in the wrong place: CommandCounterIncrement should
-    * be purely a local operation, most likely.  However fooling with this
-    * will affect asynchronous cross-backend interactions, which doesn't seem
-    * like a wise thing to do in late beta, so save improving this for
-    * another day - tgl 2007-11-30
-    */
-   AtStart_Cache();
 }
 
 /*


-- 
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

  16. Re: Review: Fix snapshot taking inconsistencies

    Alvaro Herrera <alvherre@commandprompt.com> — 2010-10-20T19:33:12Z

    Excerpts from Tom Lane's message of lun oct 04 10:31:26 -0400 2010:

> In the particular case at hand here, I rather wonder why SQL functions
> are depending on postgres.c at all.  It might be better to just
> duplicate a bit of code to make them independent.  pg_parse_and_rewrite
> would then be dead code and could be deleted.

This idea doesn't work, unless pushed a lot further.  Attached are two
separate patches, extracted from the last patch version posted by Marko
(git commit --interactive helped here).  The first one does what you
suggest above: remove pg_parse_and_rewrite and inline it into the
callers.  The other patch is the remainder.

Read on for the details of the first patch.  As for the second patch,
which is Marko's original intention anyway, I don't see that it needs to
be delayed because of the first one.  So while I haven't reviewed it, I
think it should be considered separately.


The problem with this patch (0001) is that the inlined versions of the
code that used to be pg_parse_and_rewrite are still depending on
functions in postgres.c.  These are pg_parse_query and
pg_analyze_and_rewrite.  pg_parse_query is just a layer on top of
raw_parser.  pg_analyze_and_rewrite is a layer on top of parse_analyze
plus pg_rewrite_query (also on postgres.c).

Now, the only difference between those functions and the ones that
underlie them is that they have the bunch of tracing macros and
log_parser_stats reporting.  So one solution would be to have SQL
functions (pg_proc.c and executor/functions.c) call the raw parser.c and
analyze.c functions directly, without invoking the tracing/logging code.  

The other idea would be to move the whole of those functions out of
postgres.c and into their own modules, i.e. move pg_parse_query into
parser.c and pg_analyze_and_rewrite and pg_rewrite_query into
rewriteHandler.c.  (This actually requires a bit more effort because we
should also move pg_analyze_and_rewrite_params out of postgres.c,
following pg_analyze_and_rewrite).

Note that pg_analyze_and_rewrite and its "params" siblings are being
called from copy.c, spi.c, optimizer/util/clauses.c, and plancache.c.
So it does make certain sense to move them out of postgres.c, if we want
to think of postgres.c as a module only concerned with client
interaction.

The only quarrel I have with this code shuffling is that
pg_rewrite_query is being called from exec_parse_message.  Since it's
now a static function, it would have to stop being static so that it can
be called from both places (postgres.c and rewriteHandler.c)

-- 
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

  17. Re: Review: Fix snapshot taking inconsistencies

    Alvaro Herrera <alvherre@commandprompt.com> — 2010-10-20T21:59:32Z

    Excerpts from Alvaro Herrera's message of mié oct 20 16:33:12 -0300 2010:

> The only quarrel I have with this code shuffling is that
> pg_rewrite_query is being called from exec_parse_message.  Since it's
> now a static function, it would have to stop being static so that it can
> be called from both places (postgres.c and rewriteHandler.c)

Actually, I just noticed that the "remainder" patch uses pg_plan_query,
which is also in postgres.c.  This function along with its sibling
pg_plan_queries is also called from other internal places, like the
PREPARE code, SPI and the plan cache.

It strikes me that if we really want to restructure things to divide
client interaction from other query-processing routines, we should
create another file, say src/backend/tcop/queries.c; this would have
stuff like pg_plan_query, pg_plan_queries, pg_rewrite_query, and the
other things that the patch was evicting from postgres.c (plus, I
imagine, a bunch of other stuff that I may be missing).  In fact, if we
go down this route, there would be no point in removing
pg_parse_and_rewrite; we would just move it to this new module.

-- 
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

  18. Re: Review: Fix snapshot taking inconsistencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-10-20T22:15:43Z

    Alvaro Herrera <alvherre@commandprompt.com> writes:
> It strikes me that if we really want to restructure things to divide
> client interaction from other query-processing routines, we should
> create another file, say src/backend/tcop/queries.c; this would have
> stuff like pg_plan_query, pg_plan_queries, pg_rewrite_query, and the
> other things that the patch was evicting from postgres.c (plus, I
> imagine, a bunch of other stuff that I may be missing).  In fact, if we
> go down this route, there would be no point in removing
> pg_parse_and_rewrite; we would just move it to this new module.

Yeah, possibly that would be a good idea.

To my mind, the first thing that has to be resolved before messing
around in this area is whether or not we want the logging/statistics
behavior of these functions to apply when they are used in contexts
other than interactive queries.  Personally I would vote no, mainly
because I don't think that behavior is very sensible in nested
execution.  If that is the decision, then probably these functions
should stay where they are and as they are, and we just deprecate
outside use of them.  I'm not sure whether there's enough left after
deleting the logging/statistics behavior to justify making exported
versions, as opposed to just having the callers call the next-layer-down
functionality directly.

			regards, tom lane

  19. Re: Review: Fix snapshot taking inconsistencies

    Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2010-10-21T12:32:54Z

    Hi,

Here's an updated patch.

I'm still not too fond of the logic in spi.c, but I can't see a better 
way to do this.  If someone sees a better way, I'm not going to object.

I also made some changes to the SQL functions now that we have a 
different API.  The previous code pushed and popped snapshots quite heavily.

I'd also like to see more regression tests for SQL functions, but I'm 
going to submit my suggestions as a separate patch.


Regards,
Marko Tiikkaja

  20. Re: Review: Fix snapshot taking inconsistencies

    Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2011-01-15T20:30:14Z

    On 2010-10-21 3:32 PM +0200, Marko Tiikkaja wrote:
> <patch>

Here's the patch rebased against the master.  No code changes since the 
last patch I sent.


Regards,
Marko Tiikkaja

  21. Re: Review: Fix snapshot taking inconsistencies

    Robert Haas <robertmhaas@gmail.com> — 2011-01-30T22:53:39Z

    On Wed, Oct 20, 2010 at 6:15 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
>> It strikes me that if we really want to restructure things to divide
>> client interaction from other query-processing routines, we should
>> create another file, say src/backend/tcop/queries.c; this would have
>> stuff like pg_plan_query, pg_plan_queries, pg_rewrite_query, and the
>> other things that the patch was evicting from postgres.c (plus, I
>> imagine, a bunch of other stuff that I may be missing).  In fact, if we
>> go down this route, there would be no point in removing
>> pg_parse_and_rewrite; we would just move it to this new module.
>
> Yeah, possibly that would be a good idea.
>
> To my mind, the first thing that has to be resolved before messing
> around in this area is whether or not we want the logging/statistics
> behavior of these functions to apply when they are used in contexts
> other than interactive queries.  Personally I would vote no, mainly
> because I don't think that behavior is very sensible in nested
> execution.  If that is the decision, then probably these functions
> should stay where they are and as they are, and we just deprecate
> outside use of them.  I'm not sure whether there's enough left after
> deleting the logging/statistics behavior to justify making exported
> versions, as opposed to just having the callers call the next-layer-down
> functionality directly.

It looks to me like the log_planner_stats stuff will blow up in nested
execution.  So there's certainly no point in doing that.

The debug_print_plan stuff *might* be useful in nested execution...
although I'm not convinced.  Not too many people are probably going to
use this at all, since the output is not human-readable.

I'm not real sure about the dtrace probes.  If they are useful in
nested execution, we could move them down a bit (e.g. put
TRACE_POSTGRESQL_QUERY_PLAN_START/END into planner()).

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

  22. Re: Review: Fix snapshot taking inconsistencies

    Alvaro Herrera <alvherre@commandprompt.com> — 2011-02-23T20:27:29Z

    Excerpts from Marko Tiikkaja's message of sáb ene 15 17:30:14 -0300 2011:
> On 2010-10-21 3:32 PM +0200, Marko Tiikkaja wrote:
> > <patch>
> 
> Here's the patch rebased against the master.  No code changes since the 
> last patch I sent.

Having a look at this.

-- 
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

  23. Re: Review: Fix snapshot taking inconsistencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-02-23T22:39:23Z

    Alvaro Herrera <alvherre@commandprompt.com> writes:
> Excerpts from Marko Tiikkaja's message of sb ene 15 17:30:14 -0300 2011:
>> On 2010-10-21 3:32 PM +0200, Marko Tiikkaja wrote:
> <patch>
>> 
>> Here's the patch rebased against the master.  No code changes since the 
>> last patch I sent.

> Having a look at this.

My recollection is that this was pretty tightly coupled to the wCTE
patch.  I had been intending to review them together, and have just
now come up for air enough to start doing that.  Do you really want
to review this one separately?

			regards, tom lane

  24. Re: Review: Fix snapshot taking inconsistencies

    Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2011-02-23T22:48:38Z

    On 2011-02-24 12:39 AM, Tom Lane wrote:
> My recollection is that this was pretty tightly coupled to the wCTE
> patch.

It was, but isn't anymore.  Now it's just a bugfix.


Regards,
Marko Tiikkaja

  25. Re: Review: Fix snapshot taking inconsistencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-02-24T00:31:21Z

    Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> writes:
> On 2011-02-24 12:39 AM, Tom Lane wrote:
>> My recollection is that this was pretty tightly coupled to the wCTE
>> patch.

> It was, but isn't anymore.  Now it's just a bugfix.

The connection is the question of where to do CommandCounterIncrement
between successive DML WITH operations in a single command.  Right
offhand, I don't see any CommandCounterIncrement in the wCTE patch,
so I'm sort of wondering whether the case works at all...

			regards, tom lane

  26. Re: Review: Fix snapshot taking inconsistencies

    Alvaro Herrera <alvherre@commandprompt.com> — 2011-02-24T00:35:13Z

    Excerpts from Tom Lane's message of mié feb 23 19:39:23 -0300 2011:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
> > Excerpts from Marko Tiikkaja's message of sáb ene 15 17:30:14 -0300 2011:
> >> On 2010-10-21 3:32 PM +0200, Marko Tiikkaja wrote:
> > <patch>
> >> 
> >> Here's the patch rebased against the master.  No code changes since the 
> >> last patch I sent.
> 
> > Having a look at this.
> 
> My recollection is that this was pretty tightly coupled to the wCTE
> patch.  I had been intending to review them together, and have just
> now come up for air enough to start doing that.  Do you really want
> to review this one separately?

Dunno.  If you're gonna pick it up I guess my time is best spent
elsewhere.  There was some restructuring in code in postgres.c to be
done near this patch, which wasn't attacked at all by Marko AFAICS.
Maybe I should be looking at that instead.

-- 
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

  27. Re: Review: Fix snapshot taking inconsistencies

    Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2011-02-24T00:38:05Z

    On 2011-02-24 2:31 AM, Tom Lane wrote:
> Marko Tiikkaja<marko.tiikkaja@cs.helsinki.fi>  writes:
>> On 2011-02-24 12:39 AM, Tom Lane wrote:
>>> My recollection is that this was pretty tightly coupled to the wCTE
>>> patch.
>
>> It was, but isn't anymore.  Now it's just a bugfix.
>
> The connection is the question of where to do CommandCounterIncrement
> between successive DML WITH operations in a single command.

.. what?  We decided *not* to do any CommandCounterIncrements between 
DML WITHs.


Regards,
Marko Tiikkaja

  28. Re: Review: Fix snapshot taking inconsistencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-02-24T00:39:23Z

    Alvaro Herrera <alvherre@commandprompt.com> writes:
> Excerpts from Tom Lane's message of mi feb 23 19:39:23 -0300 2011:
>> My recollection is that this was pretty tightly coupled to the wCTE
>> patch.  I had been intending to review them together, and have just
>> now come up for air enough to start doing that.  Do you really want
>> to review this one separately?

> Dunno.  If you're gonna pick it up I guess my time is best spent
> elsewhere.  There was some restructuring in code in postgres.c to be
> done near this patch, which wasn't attacked at all by Marko AFAICS.
> Maybe I should be looking at that instead.

Well, Marko claims they're independent, so if you feel it fits into
what you're doing you're welcome to it.  But I was planning to deal
with Marko's two patches as soon as the FDW dust settled, and it
seems to be settled.

			regards, tom lane

  29. Re: Review: Fix snapshot taking inconsistencies

    Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2011-02-24T00:52:42Z

    On 2011-02-24 2:35 AM, Alvaro Herrera wrote:
>  There was some restructuring in code in postgres.c to be
> done near this patch, which wasn't attacked at all by Marko AFAICS.
> Maybe I should be looking at that instead.

I don't feel at all comfortable doing the restructuring you guys have 
been talking about.


Regards,
Marko Tiikkaja

  30. Re: Review: Fix snapshot taking inconsistencies

    Alvaro Herrera <alvherre@commandprompt.com> — 2011-02-24T15:06:17Z

    Excerpts from Alvaro Herrera's message of mié feb 23 21:35:13 -0300 2011:
> Excerpts from Tom Lane's message of mié feb 23 19:39:23 -0300 2011:

> > My recollection is that this was pretty tightly coupled to the wCTE
> > patch.  I had been intending to review them together, and have just
> > now come up for air enough to start doing that.  Do you really want
> > to review this one separately?
> 
> Dunno.  If you're gonna pick it up I guess my time is best spent
> elsewhere.  There was some restructuring in code in postgres.c to be
> done near this patch, which wasn't attacked at all by Marko AFAICS.
> Maybe I should be looking at that instead.

... but then, if I did that, I could create merge conflicts for you, so
please have at it.  I think pure beautification code cleanups can way
till 9.2 starts.

-- 
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

  31. Re: Review: Fix snapshot taking inconsistencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-02-24T15:21:41Z

    Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> writes:
> On 2011-02-24 2:31 AM, Tom Lane wrote:
>> The connection is the question of where to do CommandCounterIncrement
>> between successive DML WITH operations in a single command.

> .. what?  We decided *not* to do any CommandCounterIncrements between 
> DML WITHs.

Oh, did we decide to do it that way?  OK with me, but the submitted docs
are woefully inadequate on the point.  This behavior is going to have to
be explained extremely clearly (and even so, I bet we'll get bug reports
about it :-().

			regards, tom lane

  32. Re: Review: Fix snapshot taking inconsistencies

    Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2011-02-24T15:57:27Z

    On 2011-02-24 5:21 PM, Tom Lane wrote:
> Oh, did we decide to do it that way?  OK with me, but the submitted docs
> are woefully inadequate on the point.  This behavior is going to have to
> be explained extremely clearly (and even so, I bet we'll get bug reports
> about it :-().

I'm ready to put more effort into the documentation if the patch is 
going in, but I really don't want to waste my time just to hear that the 
patch is not going to be in 9.1.  Does this sound acceptable?


Regards,
Marko Tiikkaja

  33. Re: Review: Fix snapshot taking inconsistencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-02-24T16:02:57Z

    Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> writes:
> On 2011-02-24 5:21 PM, Tom Lane wrote:
>> Oh, did we decide to do it that way?  OK with me, but the submitted docs
>> are woefully inadequate on the point.  This behavior is going to have to
>> be explained extremely clearly (and even so, I bet we'll get bug reports
>> about it :-().

> I'm ready to put more effort into the documentation if the patch is 
> going in, but I really don't want to waste my time just to hear that the 
> patch is not going to be in 9.1.  Does this sound acceptable?

I've found some things I don't like about it, but the only part that
seems far short of being committable is the documentation.

			regards, tom lane

  34. Re: Review: Fix snapshot taking inconsistencies

    Robert Haas <robertmhaas@gmail.com> — 2011-02-26T05:09:31Z

    On Thu, Feb 24, 2011 at 11:02 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> writes:
>> On 2011-02-24 5:21 PM, Tom Lane wrote:
>>> Oh, did we decide to do it that way?  OK with me, but the submitted docs
>>> are woefully inadequate on the point.  This behavior is going to have to
>>> be explained extremely clearly (and even so, I bet we'll get bug reports
>>> about it :-().
>
>> I'm ready to put more effort into the documentation if the patch is
>> going in, but I really don't want to waste my time just to hear that the
>> patch is not going to be in 9.1.  Does this sound acceptable?
>
> I've found some things I don't like about it, but the only part that
> seems far short of being committable is the documentation.

Tom/Alvaro, have the two of you hammered out who is going to finish
this one off?  I *believe* Alvaro told me on IM that he was leaving
this one for Tom.

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

  35. Re: Review: Fix snapshot taking inconsistencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-02-26T05:15:58Z

    Robert Haas <robertmhaas@gmail.com> writes:
> Tom/Alvaro, have the two of you hammered out who is going to finish
> this one off?  I *believe* Alvaro told me on IM that he was leaving
> this one for Tom.

Last I heard, the ball was in my court.  I'll try to get it done over
the weekend.

			regards, tom lane

  36. Re: Review: Fix snapshot taking inconsistencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-02-28T18:22:45Z

    Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> writes:
> [ latest version of snapshot-taking patch ]

I started to look at this, and find myself fairly confused as to what
the purpose is anymore.  Reviewing the thread, there has been a lot of
discussion of refactoring the API of pg_parse_and_rewrite and related
functions exported by postgres.c; but the current patch seems to have
abandoned that goal (except for removing pg_parse_and_rewrite itself,
which doesn't seem to me to have a lot of point except as part of a
more general refactoring).  With respect to the issue of changing
snapshot timing, most of the discussion around that seemed to start
from assumptions about the behavior of wCTEs that we've now abandoned.
And there was some discussion about rule behavior too, but it's not
clear to me whether this patch intends to change that or not.  The
lack of any documentation change doesn't help here.

So: exactly what is the intended behavioral change as of now, and what
is the argument supporting that change?

			regards, tom lane

  37. Re: Review: Fix snapshot taking inconsistencies

    Robert Haas <robertmhaas@gmail.com> — 2011-02-28T18:30:14Z

    On Mon, Feb 28, 2011 at 1:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> writes:
>> [ latest version of snapshot-taking patch ]
>
> I started to look at this, and find myself fairly confused as to what
> the purpose is anymore.  Reviewing the thread, there has been a lot of
> discussion of refactoring the API of pg_parse_and_rewrite and related
> functions exported by postgres.c; but the current patch seems to have
> abandoned that goal (except for removing pg_parse_and_rewrite itself,
> which doesn't seem to me to have a lot of point except as part of a
> more general refactoring).  With respect to the issue of changing
> snapshot timing, most of the discussion around that seemed to start
> from assumptions about the behavior of wCTEs that we've now abandoned.
> And there was some discussion about rule behavior too, but it's not
> clear to me whether this patch intends to change that or not.  The
> lack of any documentation change doesn't help here.
>
> So: exactly what is the intended behavioral change as of now, and what
> is the argument supporting that change?

IIUC, this is the result of countless rounds of communal bikeshedding around:

http://archives.postgresql.org/pgsql-hackers/2010-07/msg01256.php

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

  38. Re: Review: Fix snapshot taking inconsistencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-02-28T18:45:04Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Feb 28, 2011 at 1:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> So: exactly what is the intended behavioral change as of now, and what
>> is the argument supporting that change?

> IIUC, this is the result of countless rounds of communal bikeshedding around:

Quite :-(.  But I'm not sure where the merry-go-round stopped.

> http://archives.postgresql.org/pgsql-hackers/2010-07/msg01256.php

Please notice that the very terms of discussion in that message depend
on a view of wCTEs that has got nothing to do with what was applied.
I'm afraid that the goals of this patch might be similarly obsolete.
I definitely don't want to apply the patch in a hurry just because
we're down to the end of the commitfest.

			regards, tom lane

  39. Re: Review: Fix snapshot taking inconsistencies

    Robert Haas <robertmhaas@gmail.com> — 2011-02-28T18:58:17Z

    On Mon, Feb 28, 2011 at 1:45 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Mon, Feb 28, 2011 at 1:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> So: exactly what is the intended behavioral change as of now, and what
>>> is the argument supporting that change?
>
>> IIUC, this is the result of countless rounds of communal bikeshedding around:
>
> Quite :-(.  But I'm not sure where the merry-go-round stopped.
>
>> http://archives.postgresql.org/pgsql-hackers/2010-07/msg01256.php
>
> Please notice that the very terms of discussion in that message depend
> on a view of wCTEs that has got nothing to do with what was applied.
> I'm afraid that the goals of this patch might be similarly obsolete.

No, I don't think so.  IIUC, the problem is that EXPLAIN ANALYZE runs
the rewrite products with different snapshot handling than the regular
execution path.  So in theory you could turn on auto_explain and have
the semantics of your queries change.  That would be Bad.

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

  40. Re: Review: Fix snapshot taking inconsistencies

    Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2011-02-28T18:59:05Z

    On 2011-02-28 8:22 PM, Tom Lane wrote:
> Marko Tiikkaja<marko.tiikkaja@cs.helsinki.fi>  writes:
>> [ latest version of snapshot-taking patch ]
>
> I started to look at this, and find myself fairly confused as to what
> the purpose is anymore.  Reviewing the thread, there has been a lot of
> discussion of refactoring the API of pg_parse_and_rewrite and related
> functions exported by postgres.c; but the current patch seems to have
> abandoned that goal (except for removing pg_parse_and_rewrite itself,
> which doesn't seem to me to have a lot of point except as part of a
> more general refactoring).  With respect to the issue of changing
> snapshot timing, most of the discussion around that seemed to start
> from assumptions about the behavior of wCTEs that we've now abandoned.
> And there was some discussion about rule behavior too, but it's not
> clear to me whether this patch intends to change that or not.  The
> lack of any documentation change doesn't help here.
>
> So: exactly what is the intended behavioral change as of now, and what
> is the argument supporting that change?

The only intended change is what I was wondering in the original post: 
snapshot handling between normal execution and EXPLAIN ANALYZE when a 
query expands to multiple trees because of rewrite rules.  Like I said 
earlier, this is just a bugfix now that wCTEs don't need it anymore.


Rcgards,
Marko Tiikkaja

  41. Re: Review: Fix snapshot taking inconsistencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-02-28T19:01:15Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Feb 28, 2011 at 1:45 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm afraid that the goals of this patch might be similarly obsolete.

> No, I don't think so.  IIUC, the problem is that EXPLAIN ANALYZE runs
> the rewrite products with different snapshot handling than the regular
> execution path.

Possibly, but it's not clear to me that this patch fixes that.
As I said, it's no longer obvious what the patch means to do, and I'd
like a clear statement of that.

> So in theory you could turn on auto_explain and have
> the semantics of your queries change.  That would be Bad.

That's just FUD.  auto_explain doesn't run EXPLAIN ANALYZE.

			regards, tom lane

  42. Re: Review: Fix snapshot taking inconsistencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-02-28T19:03:43Z

    Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> writes:
> On 2011-02-28 8:22 PM, Tom Lane wrote:
>> So: exactly what is the intended behavioral change as of now, and what
>> is the argument supporting that change?

> The only intended change is what I was wondering in the original post: 
> snapshot handling between normal execution and EXPLAIN ANALYZE when a 
> query expands to multiple trees because of rewrite rules.  Like I said 
> earlier, this is just a bugfix now that wCTEs don't need it anymore.

OK, and which behavior is getting changed, to what?  I am not interested
in trying to reverse-engineer a specification from the patch.

			regards, tom lane

  43. Re: Review: Fix snapshot taking inconsistencies

    Robert Haas <robertmhaas@gmail.com> — 2011-02-28T19:14:59Z

    On Mon, Feb 28, 2011 at 2:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Mon, Feb 28, 2011 at 1:45 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I'm afraid that the goals of this patch might be similarly obsolete.
>
>> No, I don't think so.  IIUC, the problem is that EXPLAIN ANALYZE runs
>> the rewrite products with different snapshot handling than the regular
>> execution path.
>
> Possibly, but it's not clear to me that this patch fixes that.
> As I said, it's no longer obvious what the patch means to do, and I'd
> like a clear statement of that.

Fair enough.  I assume Marko will provide that shortly.  I believe the
consensus was to make the regular case behave like EXPLAIN ANALYZE
rather than the other way around...

>> So in theory you could turn on auto_explain and have
>> the semantics of your queries change.  That would be Bad.
>
> That's just FUD.  auto_explain doesn't run EXPLAIN ANALYZE.

Oh, woops.  I stand corrected.  But I guess the query might behave
differently with and without EXPLAIN ANALYZE?

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

  44. Re: Review: Fix snapshot taking inconsistencies

    Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2011-02-28T19:18:55Z

    On 2011-02-28 9:03 PM, Tom Lane wrote:
> Marko Tiikkaja<marko.tiikkaja@cs.helsinki.fi>  writes:
>> On 2011-02-28 8:22 PM, Tom Lane wrote:
>>> So: exactly what is the intended behavioral change as of now, and what
>>> is the argument supporting that change?
>
>> The only intended change is what I was wondering in the original post:
>> snapshot handling between normal execution and EXPLAIN ANALYZE when a
>> query expands to multiple trees because of rewrite rules.  Like I said
>> earlier, this is just a bugfix now that wCTEs don't need it anymore.
>
> OK, and which behavior is getting changed, to what?  I am not interested
> in trying to reverse-engineer a specification from the patch.

My recollection is (and the archives seem to agree) that normal 
execution and SQL functions were changed to only advance the CID instead 
of taking a new snapshot.  EXPLAIN ANALYZE and SPI (not exactly sure 
about this one) did that already so they were just changed to use the 
new API.


Regards,
Marko Tiikkaja

  45. Re: Review: Fix snapshot taking inconsistencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-02-28T19:36:10Z

    Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> writes:
> On 2011-02-28 9:03 PM, Tom Lane wrote:
>> OK, and which behavior is getting changed, to what?  I am not interested
>> in trying to reverse-engineer a specification from the patch.

> My recollection is (and the archives seem to agree) that normal 
> execution and SQL functions were changed to only advance the CID instead 
> of taking a new snapshot.  EXPLAIN ANALYZE and SPI (not exactly sure 
> about this one) did that already so they were just changed to use the 
> new API.

OK, so the intent is that in all cases, we just advance CID and don't
take a new snapshot between queries that were generated (by rule
expansion) from a single original parsetree?  But we still take a new
snap between original parsetrees?  Works for me.

			regards, tom lane

  46. Re: Review: Fix snapshot taking inconsistencies

    Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> — 2011-02-28T19:39:17Z

    On 2011-02-28 9:36 PM, Tom Lane wrote:
> Marko Tiikkaja<marko.tiikkaja@cs.helsinki.fi>  writes:
>> On 2011-02-28 9:03 PM, Tom Lane wrote:
>>> OK, and which behavior is getting changed, to what?  I am not interested
>>> in trying to reverse-engineer a specification from the patch.
>
>> My recollection is (and the archives seem to agree) that normal
>> execution and SQL functions were changed to only advance the CID instead
>> of taking a new snapshot.  EXPLAIN ANALYZE and SPI (not exactly sure
>> about this one) did that already so they were just changed to use the
>> new API.
>
> OK, so the intent is that in all cases, we just advance CID and don't
> take a new snapshot between queries that were generated (by rule
> expansion) from a single original parsetree?  But we still take a new
> snap between original parsetrees?  Works for me.

Exactly.


Regards,
Marko Tiikkaja

  47. Re: Review: Fix snapshot taking inconsistencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-03-01T04:29:46Z

    Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> writes:
> On 2011-02-28 9:36 PM, Tom Lane wrote:
>> OK, so the intent is that in all cases, we just advance CID and don't
>> take a new snapshot between queries that were generated (by rule
>> expansion) from a single original parsetree?  But we still take a new
>> snap between original parsetrees?  Works for me.

> Exactly.

OK, applied with corrections (I didn't think either the spi.c or
functions.c changes were quite right).

			regards, tom lane