Re: Review: Fix snapshot taking inconsistencies

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.

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

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

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

Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)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

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

On 2010-10-04 5:31 PM +0300, Tom Lane wrote:
> Marko Tiikkaja<marko(dot)tiikkaja(at)cs(dot)helsinki(dot)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

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

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
>

Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)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

On 2010-10-13 1:21 AM +0300, Tom Lane wrote:
> Marko Tiikkaja<marko(dot)tiikkaja(at)cs(dot)helsinki(dot)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

Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)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

On 2010-10-13 2:10 AM +0300, Tom Lane wrote:
> Marko Tiikkaja<marko(dot)tiikkaja(at)cs(dot)helsinki(dot)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

Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)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

Excerpts from Tom Lane's message of mar oct 12 20:49:28 -0300 2010:
> Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)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

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

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

On Wed, Oct 20, 2010 at 6:15 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)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

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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)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

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

Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)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

Excerpts from Tom Lane's message of mié feb 23 19:39:23 -0300 2011:
> Alvaro Herrera <alvherre(at)commandprompt(dot)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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On 2011-02-24 2:31 AM, Tom Lane wrote:
> Marko Tiikkaja<marko(dot)tiikkaja(at)cs(dot)helsinki(dot)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

Alvaro Herrera <alvherre(at)commandprompt(dot)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

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

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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)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

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

Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)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

On Thu, Feb 24, 2011 at 11:02 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)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

Robert Haas <robertmhaas(at)gmail(dot)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

Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)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

On Mon, Feb 28, 2011 at 1:22 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Feb 28, 2011 at 1:22 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

On Mon, Feb 28, 2011 at 1:45 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Mon, Feb 28, 2011 at 1:22 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

On 2011-02-28 8:22 PM, Tom Lane wrote:
> Marko Tiikkaja<marko(dot)tiikkaja(at)cs(dot)helsinki(dot)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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Feb 28, 2011 at 1:45 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)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

On Mon, Feb 28, 2011 at 2:01 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Mon, Feb 28, 2011 at 1:45 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

On 2011-02-28 9:03 PM, Tom Lane wrote:
> Marko Tiikkaja<marko(dot)tiikkaja(at)cs(dot)helsinki(dot)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

Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)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

On 2011-02-28 9:36 PM, Tom Lane wrote:
> Marko Tiikkaja<marko(dot)tiikkaja(at)cs(dot)helsinki(dot)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

Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)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