vBulletin Search Engine Optimization
| |||||||
| Register | FAQ | Members List | Calendar | Search | Today's Posts | Mark Forums Read |
| ||||
| On Tue, 2008-04-22 at 15:49 -0400, Alvaro Herrera wrote: > - Three CopySnapshot call sites remain outside snapmgr.c: DoCopy() on > copy.c, ExplainOnePlan() on explain.c and _SPI_execute_plan() on spi.c. > They are there because they grab the current ActiveSnapshot, modify it, > and then use the resulting snapshot. There is no corresponding > FreeSnapshot, because it's not needed. Not needed? How can we be certain that the modified snapshot does not outlive its original source? -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches |
| |||
| Simon Riggs wrote: > On Tue, 2008-04-22 at 15:49 -0400, Alvaro Herrera wrote: > > > - Three CopySnapshot call sites remain outside snapmgr.c: DoCopy() on > > copy.c, ExplainOnePlan() on explain.c and _SPI_execute_plan() on spi.c. > > They are there because they grab the current ActiveSnapshot, modify it, > > and then use the resulting snapshot. There is no corresponding > > FreeSnapshot, because it's not needed. > > Not needed? How can we be certain that the modified snapshot does not > outlive its original source? It's not CopySnapshot that's not needed, but FreeSnapshot. The point here is that the snapshot will be freed automatically as soon as it is PopActiveSnapshot'd out of existance. CopySnapshot creates a new, separate copy of the passed snapshot, and each of them will be freed (separately) as soon as their refcounts reach zero. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches |
| |||
| On Tue, 2008-04-22 at 17:50 -0400, Alvaro Herrera wrote: > Simon Riggs wrote: > > On Tue, 2008-04-22 at 15:49 -0400, Alvaro Herrera wrote: > > > > > - Three CopySnapshot call sites remain outside snapmgr.c: DoCopy() on > > > copy.c, ExplainOnePlan() on explain.c and _SPI_execute_plan() on spi.c. > > > They are there because they grab the current ActiveSnapshot, modify it, > > > and then use the resulting snapshot. There is no corresponding > > > FreeSnapshot, because it's not needed. > > > > Not needed? How can we be certain that the modified snapshot does not > > outlive its original source? > > It's not CopySnapshot that's not needed, but FreeSnapshot. The point > here is that the snapshot will be freed automatically as soon as it is > PopActiveSnapshot'd out of existance. CopySnapshot creates a new, > separate copy of the passed snapshot, and each of them will be freed > (separately) as soon as their refcounts reach zero. OK, so it can;t be copied to a longer lived memory context? -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches |
| |||
| Simon Riggs wrote: > OK, so it can;t be copied to a longer lived memory context? CopySnapshot always copies snapshots to SnapshotContext, which is a context that lives until transaction end. There's no mechanism for copying a snapshot into another context, because I don't see the need. If you need that ability, please explain. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches |
| |||
| Alvaro Herrera <alvherre@commandprompt.com> writes: > Simon Riggs wrote: >> OK, so it can;t be copied to a longer lived memory context? > CopySnapshot always copies snapshots to SnapshotContext, which is a > context that lives until transaction end. There's no mechanism for > copying a snapshot into another context, because I don't see the need. The only reason we have memory contexts at all is to avoid the need to track individual palloc'd objects. Since we're instituting exactly such tracking for snapshots, there's no value in placing them in general-purpose memory contexts. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches |
| |||
| Tom Lane wrote: > Alvaro Herrera <alvherre@commandprompt.com> writes: > > > CopySnapshot always copies snapshots to SnapshotContext, which is a > > context that lives until transaction end. There's no mechanism for > > copying a snapshot into another context, because I don't see the need. > > The only reason we have memory contexts at all is to avoid the need to > track individual palloc'd objects. Since we're instituting exactly such > tracking for snapshots, there's no value in placing them in > general-purpose memory contexts. The problem is that we reuse snapshots, and not all uses have the same longevity. If a context goes away from under a snapshot and there are other references to it, the result is a dangling pointer somewhere. That's why we have reference counts on snaps: we know we can free one when its refcounts are zero. At the same time, the snapshots all go away at transaction end with TopTransactionContext. The other possible approach to this problem is creating a separate copy each time a snapshot is reused, but this just causes extra palloc'ing for no gain at all. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches |
| |||
| On Tue, 2008-04-22 at 18:13 -0400, Alvaro Herrera wrote: > Simon Riggs wrote: > > > OK, so it can;t be copied to a longer lived memory context? > > CopySnapshot always copies snapshots to SnapshotContext, which is a > context that lives until transaction end. There's no mechanism for > copying a snapshot into another context, because I don't see the need. > > If you need that ability, please explain. No, I wish to prevent that, not enable it. Perhaps put the TransactionId on each snapshot and then an Assert can check it before its used. -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches |
| |||
| Simon Riggs wrote: > On Tue, 2008-04-22 at 18:13 -0400, Alvaro Herrera wrote: > > Simon Riggs wrote: > > > > > OK, so it can;t be copied to a longer lived memory context? > > > > If you need that ability, please explain. > > No, I wish to prevent that, not enable it. I see. Sure, we don't have that problem. In fact, we didn't have it before either, so I'm not sure I see your point :-) > Perhaps put the TransactionId on each snapshot and then an Assert can > check it before its used. There's no need for that -- all snapshots go away at transaction end. An attempt to use one would cause a prompt crash (at least on an assert-enabled build.) -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches |
| |||
| Tom Lane wrote: > The only reason we have memory contexts at all is to avoid the need to > track individual palloc'd objects. Since we're instituting exactly such > tracking for snapshots, there's no value in placing them in > general-purpose memory contexts. FWIW I noticed yesterday after going to bed that SnapshotContext serves no useful purpose -- we can just remove it and store snaps in TopTransactionContext. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches |
| ||||
| On Wed, 2008-04-23 at 08:21 -0400, Alvaro Herrera wrote: > Simon Riggs wrote: > > On Tue, 2008-04-22 at 18:13 -0400, Alvaro Herrera wrote: > > > Simon Riggs wrote: > > > > > > > OK, so it can;t be copied to a longer lived memory context? > > > > > > If you need that ability, please explain. > > > > No, I wish to prevent that, not enable it. > > I see. Sure, we don't have that problem. In fact, we didn't have it > before either, so I'm not sure I see your point :-) You originally said "because its not needed" but didn't explain why. I wanted to make sure there was no loophole. I'm not trying to make any other point, just checking. Forgive me for being dense, but what is there to stop you using a CopySnapshot in TopMemoryContext? If you did, there would be no way to free it, nor would we notice it had been done, AFAICS. Not anything I'm thinking about doing, though. -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches |
| Thread Tools | |
| Display Modes | |
| |