Thread: DEALLOCATE ALL
When pooling connections where prepared statements are in use, it is hard to give new client totally clean connection as there may be allocated statements that give errors when new client starts preparing statements again. Currently PgPool solves the situation by parsing all queries and keeping list of prepares statements. This may not be a big problem for it as it parses the queries anyway, but for simple pooler like PgBouncer (see pgfoundry) that does not look inside libpq packets it is huge problem. Attached is a patch that allows keyword ALL to be used with DEALLOCATE and then frees all prepared queryes. I think it is useful for most pooling situations, not only PgBouncer. Also its similar in the spirit to RESET ALL. I did it slightly hacky way - if DeallocateStmt->name is NULL is signifies DEALLOCATE ALL command. All the code that looks into DeallocateStmt seems to survive the situation so it should be problem. If still a new node is needed or additional field in the node I can rework the patch. -- marko
Attachment
Marko Kreen wrote: > When pooling connections where prepared statements are in use, > it is hard to give new client totally clean connection as > there may be allocated statements that give errors when > new client starts preparing statements again. Huh, didn't we have a RESET SESSION command to do just that? What about cursors, for example? > I did it slightly hacky way - if DeallocateStmt->name is > NULL is signifies DEALLOCATE ALL command. All the code > that looks into DeallocateStmt seems to survive the situation > so it should be problem. If still a new node is needed > or additional field in the node I can rework the patch. Wouldn't it be easier to just add a bool to DeallocateStmt? -- Alvaro Herrera http://www.PlanetPostgreSQL.org "Si un desconocido se acerca y te regala un CD de Ubuntu ... Eso es ... Eau de Tux"
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Marko Kreen wrote:
>> When pooling connections where prepared statements are in use,
>> it is hard to give new client totally clean connection as
>> there may be allocated statements that give errors when
>> new client starts preparing statements again.
> Huh, didn't we have a RESET SESSION command to do just that? What about
> cursors, for example?
We don't actually *have* one, but I believe it was agreed that that is
the right API to provide. If a pooler has to remember to clear prepared
statements, GUCs, cursors, and who knows what else, it'll be perpetually
broken because there'll be something it omits.
There might be a use-case for DEALLOCATE ALL, but needs of poolers
aren't it. I'd be inclined to vote against this unless someone can
point to a better use-case.
regards, tom lane
On 3/27/07, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > Marko Kreen wrote: > >> When pooling connections where prepared statements are in use, > >> it is hard to give new client totally clean connection as > >> there may be allocated statements that give errors when > >> new client starts preparing statements again. > > > Huh, didn't we have a RESET SESSION command to do just that? What about > > cursors, for example? > > We don't actually *have* one, but I believe it was agreed that that is > the right API to provide. If a pooler has to remember to clear prepared > statements, GUCs, cursors, and who knows what else, it'll be perpetually > broken because there'll be something it omits. Well. Please apply following patch then: http://archives.postgresql.org/pgsql-patches/2004-12/msg00228.php Even if it is incomplete, the missing parts can be added later. I see no reason to keep it from users. > There might be a use-case for DEALLOCATE ALL, but needs of poolers > aren't it. I'd be inclined to vote against this unless someone can > point to a better use-case. Ok, a non-pooler argument: prepared statements are supposed to be garbage-collected by the user. Thats it. There should be friendly way to get a clean state without the need for user to specifically keep track of whats allocated, or to do messy exception-handling around PREPARE/DEALLOCATE. (PREPARE OR REPLACE and DEALLOCATE IF EXISTS would also lessen the pain.) Then a pooler argument: there is one pooler where RandomJoe executes queries and another for specific app where the subset of SQL it uses is known. I want to RESET only specific things in app case. So it would be good if the RESET-s for specific areas would be available. Also the objections to the Hans' patch give impression that different pooling solutions want different RESET EVERYTHING, so again, it would be good if RESET-s for different areas are available and the all-encomassing RESET EVERYTHING just ties all the specific RESETs together. -- marko
Marko Kreen wrote: > Then a pooler argument: there is one pooler where RandomJoe executes > queries and another for specific app where the subset of SQL it uses is > known. I want to RESET only specific things in app case. So it would be > good if the RESET-s for specific areas would be available. > > Also the objections to the Hans' patch give impression that different > pooling solutions want different RESET EVERYTHING, so again, > it would be good if RESET-s for different areas are available > and the all-encomassing RESET EVERYTHING just ties all the specific > RESETs together. Totally agree. Please make the adjustments to DEALLOCATE ALL, and roll that into the 2004 patch for RESET SESSION and post and updated version. Thanks. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Marko Kreen wrote: > When pooling connections where prepared statements are in use, > it is hard to give new client totally clean connection as > there may be allocated statements that give errors when > new client starts preparing statements again. I agree with the other comments that RESET SESSION is the right API for this, although we can also provide DEALLOCATE ALL, I suppose. As to the implementation, calling hash_remove() in a loop seems a pretty unfortunate way to clear a hash table -- adding a hash_reset() function to the dynahash API would be cleaner and faster. -Neil
Neil Conway escribió: > As to the implementation, calling hash_remove() in a loop seems a pretty > unfortunate way to clear a hash table -- adding a hash_reset() function > to the dynahash API would be cleaner and faster. I wonder why hash_drop cannot be used? -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On 3/30/07, Alvaro Herrera <alvherre@commandprompt.com> wrote: > Neil Conway escribió: > > > As to the implementation, calling hash_remove() in a loop seems a pretty > > unfortunate way to clear a hash table -- adding a hash_reset() function > > to the dynahash API would be cleaner and faster. > > I wonder why hash_drop cannot be used? hash_destroy()? Each element need separate destruction. -- marko
Marko Kreen escribió: > On 3/30/07, Alvaro Herrera <alvherre@commandprompt.com> wrote: > >Neil Conway escribió: > > > >> As to the implementation, calling hash_remove() in a loop seems a pretty > >> unfortunate way to clear a hash table -- adding a hash_reset() function > >> to the dynahash API would be cleaner and faster. > > > >I wonder why hash_drop cannot be used? > > hash_destroy()? Each element need separate destruction. Hmm, so maybe something like hash_destroy_deep, like the List equivalent? If it's a simple pfree() for every element this would be simple enough. If this is the case, an even simpler idea would be to allocate the elements in the same MemoryContext as the hash itself (or in children thereof); then calling hash_destroy() would delete (reset?) the context and thus all elements are freed at once as well. If by destruction you mean something different than pfree, then maybe hash_remove in a loop is the best solution, the other idea being passing a function pointer to hash_destroy_deep to call on each element, which is probably too messy an API. In any case it's not likely that there are going to be thousands of prepared statements, so is this really an issue? -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On 3/30/07, Alvaro Herrera <alvherre@commandprompt.com> wrote: > If by destruction you mean something different than pfree, then maybe > hash_remove in a loop is the best solution, the other idea being passing > a function pointer to hash_destroy_deep to call on each element, which > is probably too messy an API. Yes, callback function is needed, either in HASHCTL or as argument to deep_free(). > In any case it's not likely that there are going to be thousands of > prepared statements, so is this really an issue? I think the issue is here that its very common thing to do, so open-coding it everywhere is waste, there should be some utility function for that. void hash_foreach(HTAB, void (*cb_func)(void *)); -- marko
Marko Kreen escribió: > On 3/30/07, Alvaro Herrera <alvherre@commandprompt.com> wrote: > >In any case it's not likely that there are going to be thousands of > >prepared statements, so is this really an issue? > > I think the issue is here that its very common thing to do, > so open-coding it everywhere is waste, there should be some > utility function for that. > > void hash_foreach(HTAB, void (*cb_func)(void *)); Extra points if you can implement a map() function for hashes ;-) (I think it's called "mutator" in our sources for other kind of stuff) I think it would be void *hash_map(HTAB, void *(*map_func) (void *)) Not sure what the return value would be though :-( (Maybe this is extra complication enough that it's not worth the hassle) -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera wrote: > Marko Kreen escribió: >> On 3/30/07, Alvaro Herrera <alvherre@commandprompt.com> wrote: > >>> In any case it's not likely that there are going to be thousands of >>> prepared statements, so is this really an issue? >> I think the issue is here that its very common thing to do, >> so open-coding it everywhere is waste, there should be some >> utility function for that. >> >> void hash_foreach(HTAB, void (*cb_func)(void *)); > > Extra points if you can implement a map() function for hashes ;-) (I > think it's called "mutator" in our sources for other kind of stuff) > > I think it would be > void *hash_map(HTAB, void *(*map_func) (void *)) > > Not sure what the return value would be though :-( (Maybe this is > extra complication enough that it's not worth the hassle) hash_map and hash_foreach seem like overkill to me, looping with hash_seq_search and doing stuff is simple enough that it doesn't really require any additional shorthands. Besides, to use them you'd always have to have a separate function and often a struct to pass down to the function, so it's not really any shorter or simpler. What would be useful is a hash_seq_remove-function that removes the previous item returned by hash_seq_search without the overhead of recalculating the hash value. While reviewing ITAGAKI's dead space map patch I noticed that he also added a hash_truncate function that removes all entries in the hash table, but unlike hash_destroy leaves the hash table intact. His implementation also calls hash_search(REMOVE) in a loop, which isn't the most efficient way to do it, but clearly there's need for more ways to empty a hash table. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com