Re: [HACKERS] Review: Fix snapshot taking inconsistencies
Marko Tiikkaja marko.tiikk...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
On Mon, Feb 28, 2011 at 1:22 PM, Tom Lane t...@sss.pgh.pa.us wrote: Marko Tiikkaja marko.tiikk...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
Robert Haas robertmh...@gmail.com writes: On Mon, Feb 28, 2011 at 1:22 PM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
On Mon, Feb 28, 2011 at 1:45 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Mon, Feb 28, 2011 at 1:22 PM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
On 2011-02-28 8:22 PM, Tom Lane wrote: Marko Tiikkajamarko.tiikk...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
Robert Haas robertmh...@gmail.com writes: On Mon, Feb 28, 2011 at 1:45 PM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
Marko Tiikkaja marko.tiikk...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
On Mon, Feb 28, 2011 at 2:01 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Mon, Feb 28, 2011 at 1:45 PM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
On 2011-02-28 9:03 PM, Tom Lane wrote: Marko Tiikkajamarko.tiikk...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
Marko Tiikkaja marko.tiikk...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
On 2011-02-28 9:36 PM, Tom Lane wrote: Marko Tiikkajamarko.tiikk...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
Marko Tiikkaja marko.tiikk...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
On Thu, Feb 24, 2011 at 11:02 AM, Tom Lane t...@sss.pgh.pa.us wrote: Marko Tiikkaja marko.tiikk...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
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 alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
Marko Tiikkaja marko.tiikk...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
Marko Tiikkaja marko.tiikk...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
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 alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
Alvaro Herrera alvhe...@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? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
Marko Tiikkaja marko.tiikk...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
Excerpts from Tom Lane's message of mié feb 23 19:39:23 -0300 2011: Alvaro Herrera alvhe...@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 alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
On 2011-02-24 2:31 AM, Tom Lane wrote: Marko Tiikkajamarko.tiikk...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
On Wed, Oct 20, 2010 at 6:15 PM, Tom Lane t...@sss.pgh.pa.us wrote: Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
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
*** a/src/backend/catalog/pg_proc.c
--- b/src/backend/catalog/pg_proc.c
***
*** 759,765 fmgr_sql_validator(PG_FUNCTION_ARGS)
--- 759,767
Oid funcoid = PG_GETARG_OID(0);
HeapTuple tuple;
Form_pg_proc proc;
+ List *raw_parsetree_list;
List *querytree_list;
+ ListCell *list_item;
boolisnull;
Datum tmp;
char *prosrc;
***
*** 832,840 fmgr_sql_validator(PG_FUNCTION_ARGS)
*/
if (!haspolyarg)
{
! querytree_list = pg_parse_and_rewrite(prosrc,
!
proc-proargtypes.values,
!
proc-pronargs);
(void) check_sql_fn_retval(funcoid, proc-prorettype,
querytree_list,
NULL, NULL);
--- 834,858
*/
if (!haspolyarg)
{
! /*
!* Parse and rewrite the queries in the function text.
!*
!* Even though check_sql_fn_retval is only interested
in the last
!* query, we analyze all of them here to check for any
errors.
!*/
! raw_parsetree_list = pg_parse_query(prosrc);
!
! querytree_list = NIL;
! foreach(list_item, raw_parsetree_list)
! {
! Node *parsetree = (Node *) lfirst(list_item);
!
! querytree_list =
pg_analyze_and_rewrite(parsetree, prosrc,
!
proc-proargtypes.values, proc-pronargs);
! }
!
! Assert(querytree_list != NIL);
!
(void) check_sql_fn_retval(funcoid, proc-prorettype,
querytree_list,
NULL, NULL);
*** a/src/backend/executor/functions.c
--- b/src/backend/executor/functions.c
***
*** 84,89 typedef struct
--- 84,90
boolreturnsSet; /* true if returning multiple
rows */
boolreturnsTuple; /* true if returning whole tuple result
*/
boolshutdown_reg; /* true if registered shutdown callback
*/
+ boolsnapshot; /* true if pushed an active
snapshot */
boolreadonly_func; /* true to run in read only mode */
boollazyEval; /* true if using lazyEval for
result query */
***
*** 93,107 typedef struct
JunkFilter *junkFilter; /* will be NULL if function returns
VOID */
! /* head of linked list of execution_state records */
! execution_state *func_state;
} SQLFunctionCache;
typedef SQLFunctionCache *SQLFunctionCachePtr;
/* non-export function prototypes */
! static execution_state *init_execution_state(List *queryTree_list,
SQLFunctionCachePtr fcache,
bool lazyEvalOK);
static void init_sql_fcache(FmgrInfo *finfo, bool lazyEvalOK);
--- 94,107
JunkFilter *junkFilter; /* will be NULL if function returns
VOID */
! List *func_state;
} SQLFunctionCache;
typedef SQLFunctionCache *SQLFunctionCachePtr;
/* non-export function prototypes */
! static List *init_execution_state(List *queryTree_list,
SQLFunctionCachePtr fcache,
bool lazyEvalOK);
static void init_sql_fcache(FmgrInfo *finfo, bool lazyEvalOK);
***
*** 123,183 static void sqlfunction_destroy(DestReceiver *self);
/* Set up the list of per-query execution_state records for a SQL function */
! static execution_state *
init_execution_state(List *queryTree_list,
SQLFunctionCachePtr fcache,
bool lazyEvalOK)
{
! execution_state *firstes = NULL;
! execution_state *preves = NULL;
execution_state *lasttages = NULL;
! ListCell *qtl_item;
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
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
*** a/src/backend/catalog/pg_proc.c
--- b/src/backend/catalog/pg_proc.c
***
*** 755,761 fmgr_sql_validator(PG_FUNCTION_ARGS)
--- 755,763
Oid funcoid = PG_GETARG_OID(0);
HeapTuple tuple;
Form_pg_proc proc;
+ List *raw_parsetree_list;
List *querytree_list;
+ ListCell *list_item;
boolisnull;
Datum tmp;
char *prosrc;
***
*** 828,836 fmgr_sql_validator(PG_FUNCTION_ARGS)
*/
if (!haspolyarg)
{
! querytree_list = pg_parse_and_rewrite(prosrc,
!
proc-proargtypes.values,
!
proc-pronargs);
(void) check_sql_fn_retval(funcoid, proc-prorettype,
querytree_list,
NULL, NULL);
--- 830,854
*/
if (!haspolyarg)
{
! /*
!* Parse and rewrite the queries in the function text.
!*
!* Even though check_sql_fn_retval is only interested
in the last
!* query, we analyze all of them here to check for any
errors.
!*/
! raw_parsetree_list = pg_parse_query(prosrc);
!
! querytree_list = NIL;
! foreach(list_item, raw_parsetree_list)
! {
! Node *parsetree = (Node *) lfirst(list_item);
!
! querytree_list =
pg_analyze_and_rewrite(parsetree, prosrc,
!
proc-proargtypes.values, proc-pronargs);
! }
!
! Assert(querytree_list != NIL);
!
(void) check_sql_fn_retval(funcoid, proc-prorettype,
querytree_list,
NULL, NULL);
*** a/src/backend/executor/functions.c
--- b/src/backend/executor/functions.c
***
*** 84,89 typedef struct
--- 84,90
boolreturnsSet; /* true if returning multiple
rows */
boolreturnsTuple; /* true if returning whole tuple result
*/
boolshutdown_reg; /* true if registered shutdown callback
*/
+ boolsnapshot; /* true if pushed an active
snapshot */
boolreadonly_func; /* true to run in read only mode */
boollazyEval; /* true if using lazyEval for
result query */
***
*** 93,107 typedef struct
JunkFilter *junkFilter; /* will be NULL if function returns
VOID */
! /* head of linked list of execution_state records */
! execution_state *func_state;
} SQLFunctionCache;
typedef SQLFunctionCache *SQLFunctionCachePtr;
/* non-export function prototypes */
! static execution_state *init_execution_state(List *queryTree_list,
SQLFunctionCachePtr fcache,
bool lazyEvalOK);
static void init_sql_fcache(FmgrInfo *finfo, bool lazyEvalOK);
--- 94,107
JunkFilter *junkFilter; /* will be NULL if function returns
VOID */
! List *func_state;
} SQLFunctionCache;
typedef SQLFunctionCache *SQLFunctionCachePtr;
/* non-export function prototypes */
! static List *init_execution_state(List *queryTree_list,
SQLFunctionCachePtr fcache,
bool lazyEvalOK);
static void init_sql_fcache(FmgrInfo *finfo, bool lazyEvalOK);
***
*** 123,183 static void sqlfunction_destroy(DestReceiver *self);
/* Set up the list of per-query execution_state records for a SQL function */
! static execution_state *
init_execution_state(List
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
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 alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support 0001-Separate-SQL-function-processing-from-postgres.c.patch Description: Binary data 0002-The-remainder-of-Marko-s-patch.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
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 alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
Excerpts from Tom Lane's message of mar oct 12 20:49:28 -0300 2010: Marko Tiikkaja marko.tiikk...@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 alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
Marko Tiikkaja marko.tiikk...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
On 2010-10-13 1:21 AM +0300, Tom Lane wrote: Marko Tiikkajamarko.tiikk...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
Marko Tiikkaja marko.tiikk...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
On 2010-10-13 2:10 AM +0300, Tom Lane wrote: Marko Tiikkajamarko.tiikk...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
Marko Tiikkaja marko.tiikk...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
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
*** a/src/backend/catalog/pg_proc.c
--- b/src/backend/catalog/pg_proc.c
***
*** 755,761 fmgr_sql_validator(PG_FUNCTION_ARGS)
--- 755,763
Oid funcoid = PG_GETARG_OID(0);
HeapTuple tuple;
Form_pg_proc proc;
+ List *raw_parsetree_list;
List *querytree_list;
+ ListCell *list_item;
boolisnull;
Datum tmp;
char *prosrc;
***
*** 828,836 fmgr_sql_validator(PG_FUNCTION_ARGS)
*/
if (!haspolyarg)
{
! querytree_list = pg_parse_and_rewrite(prosrc,
!
proc-proargtypes.values,
!
proc-pronargs);
(void) check_sql_fn_retval(funcoid, proc-prorettype,
querytree_list,
NULL, NULL);
--- 830,854
*/
if (!haspolyarg)
{
! /*
!* Parse and rewrite the queries in the function text.
!*
!* Even though check_sql_fn_retval is only interested
in the last
!* query, we analyze all of them here to check for any
errors.
!*/
! raw_parsetree_list = pg_parse_query(prosrc);
!
! querytree_list = NIL;
! foreach(list_item, raw_parsetree_list)
! {
! Node *parsetree = (Node *) lfirst(list_item);
!
! querytree_list =
pg_analyze_and_rewrite(parsetree, prosrc,
!
proc-proargtypes.values, proc-pronargs);
! }
!
! Assert(querytree_list != NIL);
!
(void) check_sql_fn_retval(funcoid, proc-prorettype,
querytree_list,
NULL, NULL);
*** a/src/backend/executor/functions.c
--- b/src/backend/executor/functions.c
***
*** 90,107 typedef struct
ParamListInfo paramLI; /* Param list representing current args
*/
Tuplestorestate *tstore;/* where we accumulate result tuples */
JunkFilter *junkFilter; /* will be NULL if function returns
VOID */
! /* head of linked list of execution_state records */
! execution_state *func_state;
} SQLFunctionCache;
typedef SQLFunctionCache *SQLFunctionCachePtr;
/* non-export function prototypes */
! static execution_state *init_execution_state(List *queryTree_list,
SQLFunctionCachePtr fcache,
bool lazyEvalOK);
static void init_sql_fcache(FmgrInfo *finfo, bool lazyEvalOK);
--- 90,107
ParamListInfo paramLI; /* Param list representing current args
*/
Tuplestorestate *tstore;/* where we accumulate result tuples */
+ Snapshotsnapshot;
JunkFilter *junkFilter; /* will be NULL if function returns
VOID */
! List *func_state;
} SQLFunctionCache;
typedef SQLFunctionCache *SQLFunctionCachePtr;
/* non-export function prototypes */
! static List *init_execution_state(List *queryTree_list,
SQLFunctionCachePtr fcache,
bool lazyEvalOK);
static void init_sql_fcache(FmgrInfo *finfo, bool lazyEvalOK);
***
*** 123,183 static void sqlfunction_destroy(DestReceiver *self);
/* Set up the list of per-query execution_state records for a SQL function */
! static execution_state *
init_execution_state(List *queryTree_list,
SQLFunctionCachePtr fcache,
bool lazyEvalOK)
{
! execution_state *firstes = NULL;
! execution_state *preves = NULL;
execution_state *lasttages = NULL;
! ListCell
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
On 2010-10-04 5:31 PM +0300, Tom Lane wrote: Marko Tiikkajamarko.tiikk...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
Marko Tiikkaja marko.tiikk...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
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 *** a/src/backend/catalog/pg_proc.c --- b/src/backend/catalog/pg_proc.c *** *** 832,838 fmgr_sql_validator(PG_FUNCTION_ARGS) proc-proargtypes.values, proc-pronargs); (void) check_sql_fn_retval(funcoid, proc-prorettype, ! querytree_list, NULL, NULL); } else --- 832,838 proc-proargtypes.values, proc-pronargs); (void) check_sql_fn_retval(funcoid, proc-prorettype, ! llast(querytree_list), NULL, NULL); } else *** a/src/backend/executor/functions.c --- b/src/backend/executor/functions.c *** *** 90,107 typedef struct ParamListInfo paramLI; /* Param list representing current args */ Tuplestorestate *tstore;/* where we accumulate result tuples */ JunkFilter *junkFilter; /* will be NULL if function returns VOID */ ! /* head of linked list of execution_state records */ ! execution_state *func_state; } SQLFunctionCache; typedef SQLFunctionCache *SQLFunctionCachePtr; /* non-export function prototypes */ ! static execution_state *init_execution_state(List *queryTree_list, SQLFunctionCachePtr fcache, bool lazyEvalOK); static void init_sql_fcache(FmgrInfo *finfo, bool lazyEvalOK); --- 90,107 ParamListInfo paramLI; /* Param list representing current args */ Tuplestorestate *tstore;/* where we accumulate result tuples */ + Snapshotsnapshot;
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Fix snapshot taking inconsistencies
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
