Thread: holdable cursors
This patch implements holdable cursors, following the proposal (materialization into a tuple store) discussed on pgsql-hackers earlier. I've updated the documentation and the regression tests. Notes on the implementation: - I needed to change the tuple store API slightly -- it assumes that it won't be used to hold data across transaction boundaries, so the temp files that it uses for on-disk storage are automatically reclaimed at end-of-transaction. I added a flag to tuplestore_begin_heap() to control this behavior. Is changing the tuple store API in this fashion OK? - in order to store executor results in a tuple store, I added a new CommandDest. This works well for the most part, with one exception: the current DestFunction API doesn't provide enough information to allow the Executor to store results into an arbitrary tuple store (where the particular tuple store to use is chosen by the call site of ExecutorRun). To workaround this, I've temporarily hacked up a solution that works, but is not ideal: since the receiveTuple DestFunction is passed the portal name, we can use that to lookup the Portal data structure for the cursor and then use that to get at the tuple store the Portal is using. This unnecessarily ties the Portal code with the tupleReceiver code, but it works... The proper fix for this is probably to change the DestFunction API -- Tom suggested passing the full QueryDesc to the receiveTuple function. In that case, callers of ExecutorRun could "subclass" QueryDesc to add any additional fields that their particular CommandDest needed to get access to. This approach would work, but I'd like to think about it for a little bit longer before deciding which route to go. In the mean time, the code works fine, so I don't think a fix is urgent. - (semi-related) I added a NO SCROLL keyword to DECLARE CURSOR, and adjusted the behavior of SCROLL in accordance with the discussion on -hackers. - (unrelated) Cleaned up some SGML markup in sql.sgml, copy.sgml Cheers, Neil
Attachment
Your patch has been added to the PostgreSQL unapplied patches list at:
http://momjian.postgresql.org/cgi-bin/pgpatches
I will try to apply it within the next 48 hours.
---------------------------------------------------------------------------
Neil Conway wrote:
> This patch implements holdable cursors, following the proposal
> (materialization into a tuple store) discussed on pgsql-hackers earlier.
> I've updated the documentation and the regression tests.
>
> Notes on the implementation:
>
> - I needed to change the tuple store API slightly -- it assumes that it
> won't be used to hold data across transaction boundaries, so the temp
> files that it uses for on-disk storage are automatically reclaimed at
> end-of-transaction. I added a flag to tuplestore_begin_heap() to control
> this behavior. Is changing the tuple store API in this fashion OK?
>
> - in order to store executor results in a tuple store, I added a new
> CommandDest. This works well for the most part, with one exception: the
> current DestFunction API doesn't provide enough information to allow the
> Executor to store results into an arbitrary tuple store (where the
> particular tuple store to use is chosen by the call site of
> ExecutorRun). To workaround this, I've temporarily hacked up a solution
> that works, but is not ideal: since the receiveTuple DestFunction is
> passed the portal name, we can use that to lookup the Portal data
> structure for the cursor and then use that to get at the tuple store the
> Portal is using. This unnecessarily ties the Portal code with the
> tupleReceiver code, but it works...
>
> The proper fix for this is probably to change the DestFunction API --
> Tom suggested passing the full QueryDesc to the receiveTuple function.
> In that case, callers of ExecutorRun could "subclass" QueryDesc to add
> any additional fields that their particular CommandDest needed to get
> access to. This approach would work, but I'd like to think about it for
> a little bit longer before deciding which route to go. In the mean time,
> the code works fine, so I don't think a fix is urgent.
>
> - (semi-related) I added a NO SCROLL keyword to DECLARE CURSOR, and
> adjusted the behavior of SCROLL in accordance with the discussion on
> -hackers.
>
> - (unrelated) Cleaned up some SGML markup in sql.sgml, copy.sgml
>
> Cheers,
>
> Neil
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: you can get off all lists at once with the unregister command
> (send "unregister YourEmailAddressHere" to majordomo@postgresql.org)
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Patch applied. Thanks. TODO item marked as done, TODO.detail/cursor removed. --------------------------------------------------------------------------- Neil Conway wrote: > This patch implements holdable cursors, following the proposal > (materialization into a tuple store) discussed on pgsql-hackers earlier. > I've updated the documentation and the regression tests. > > Notes on the implementation: > > - I needed to change the tuple store API slightly -- it assumes that it > won't be used to hold data across transaction boundaries, so the temp > files that it uses for on-disk storage are automatically reclaimed at > end-of-transaction. I added a flag to tuplestore_begin_heap() to control > this behavior. Is changing the tuple store API in this fashion OK? > > - in order to store executor results in a tuple store, I added a new > CommandDest. This works well for the most part, with one exception: the > current DestFunction API doesn't provide enough information to allow the > Executor to store results into an arbitrary tuple store (where the > particular tuple store to use is chosen by the call site of > ExecutorRun). To workaround this, I've temporarily hacked up a solution > that works, but is not ideal: since the receiveTuple DestFunction is > passed the portal name, we can use that to lookup the Portal data > structure for the cursor and then use that to get at the tuple store the > Portal is using. This unnecessarily ties the Portal code with the > tupleReceiver code, but it works... > > The proper fix for this is probably to change the DestFunction API -- > Tom suggested passing the full QueryDesc to the receiveTuple function. > In that case, callers of ExecutorRun could "subclass" QueryDesc to add > any additional fields that their particular CommandDest needed to get > access to. This approach would work, but I'd like to think about it for > a little bit longer before deciding which route to go. In the mean time, > the code works fine, so I don't think a fix is urgent. > > - (semi-related) I added a NO SCROLL keyword to DECLARE CURSOR, and > adjusted the behavior of SCROLL in accordance with the discussion on > -hackers. > > - (unrelated) Cleaned up some SGML markup in sql.sgml, copy.sgml > > Cheers, > > Neil [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 2: you can get off all lists at once with the unregister command > (send "unregister YourEmailAddressHere" to majordomo@postgresql.org) -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Neil Conway <neilc@samurai.com> writes:
> - I needed to change the tuple store API slightly -- it assumes that it
> won't be used to hold data across transaction boundaries, so the temp
> files that it uses for on-disk storage are automatically reclaimed at
> end-of-transaction. I added a flag to tuplestore_begin_heap() to control
> this behavior. Is changing the tuple store API in this fashion OK?
What do you do to ensure the temp files *do* get cleaned up eventually?
> To workaround this, I've temporarily hacked up a solution
> that works,
I'm not real pleased with the fact that this patch has already been
applied while it still contains a "temporary hack". Please fix this
soon.
regards, tom lane
On Sun, 2003-03-30 at 15:14, Tom Lane wrote: > What do you do to ensure the temp files *do* get cleaned up eventually? The temp files are cleaned up when the user issues a CLOSE on them. If any remain, they are cleaned up when the postmaster starts up. It would probably be good to clean up any remaining temp files when the backend shuts down. Would that be ok? Cheers, Neil
Neil Conway <neilc@samurai.com> writes:
> On Sun, 2003-03-30 at 15:14, Tom Lane wrote:
>> What do you do to ensure the temp files *do* get cleaned up eventually?
> It would probably be good to clean up any remaining temp files when the
> backend shuts down. Would that be ok?
Yes, you need to do that. Leaving it for the next postmaster restart
isn't acceptable (production sites don't restart the postmaster often).
It should be reasonably painless to do this in a shmem_exit hook.
regards, tom lane
From: "Tom Lane" <tgl@sss.pgh.pa.us> > Neil Conway <neilc@samurai.com> writes: > > On Sun, 2003-03-30 at 15:14, Tom Lane wrote: > >> What do you do to ensure the temp files *do* get cleaned up eventually? > > > It would probably be good to clean up any remaining temp files when the > > backend shuts down. Would that be ok? > > Yes, you need to do that. Leaving it for the next postmaster restart > isn't acceptable (production sites don't restart the postmaster often). Would it make any sense for vacuum to look around for (and clean up) unused temp files left either from this or sorts etc....
"Matthew T. O'Connor" <matthew@zeut.net> writes:
> From: "Tom Lane" <tgl@sss.pgh.pa.us>
>> Yes, you need to do that. Leaving it for the next postmaster restart
>> isn't acceptable (production sites don't restart the postmaster often).
> Would it make any sense for vacuum to look around for (and clean up) unused
> temp files left either from this or sorts etc....
I see no need for that. If a backend leaves temp files on normal exit,
the backend is broken and must be fixed. If it leaves temp files upon
crashing, well we can't expect anything from it; but that scenario will
cause a postmaster restart, which will clean out the temp files.
regards, tom lane