Re: [PATCHES] WIP: pl/pgsql cleanup
On Mon, 2005-02-07 at 21:25 -0500, Tom Lane wrote: > I haven't looked lately, but my recollection is that there are just a > few calls into the main backend from pl_comp.c. I'm not sure they are > all to backend/parser though; check /catalog, etc as well. Attached is a patch that implements this. I could only find a few places that needed to switch into the temporary context; thankfully, doing that wasn't _too_ ugly, since it allows us to dispense with retail pfrees in the function as well. The patch also has mostly-finished support for better PL/PgSQL syntax checking (per the discussion on the subject from a few months ago). My original implementation did the syntax checking after parsing was complete; this version does the checking in gram.y itself, so it should hopefully be more resilient against syntax errors that confuse statement boundaries and the like. It would have been nice to check for errors in plpgsql_read_expression() itself (rather than adding checks in most of its call sites), but we sometimes use plpgsql_read_expression() to read malformed SQL (e.g. gram.y:1069) -- it might be possible to fix that. I need to add some more regression tests and clean up the error message we emit on a syntax error, but the rest of the work is done. -Neil plpgsql_cleanup-23.patch.gz Description: GNU Zip compressed data ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] WIP: pl/pgsql cleanup
Neil Conway <[EMAIL PROTECTED]> writes: > WRT calls to backend/parser, I can see LookupTypeName (pl_comp.c:1060), > and parseTypeString (pl_comp.c:1627). Are these the only calls you had > in mind, or am I missing some? I haven't looked lately, but my recollection is that there are just a few calls into the main backend from pl_comp.c. I'm not sure they are all to backend/parser though; check /catalog, etc as well. regards, tom lane ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] WIP: pl/pgsql cleanup
On Mon, 2005-02-07 at 19:22 -0500, Tom Lane wrote: > What might work is to run in the function context as the basic state, > but switch to a short-term context when calling any main-backend code > from pl_comp.c. I'm not sure how many such calls there are, but if it's > not more than a dozen or two then this wouldn't be horridly painful. WRT calls to backend/parser, I can see LookupTypeName (pl_comp.c:1060), and parseTypeString (pl_comp.c:1627). Are these the only calls you had in mind, or am I missing some? If that's all, then it would probably be doable to manually switch contexts. -Neil ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] WIP: pl/pgsql cleanup
Neil Conway <[EMAIL PROTECTED]> writes: > On Mon, 2005-02-07 at 10:41 -0500, Tom Lane wrote: >> My recollection is that I was not nearly as worried about short-term >> pallocs in the plpgsql code itself, as about leakage in various main- >> backend routines that get called incidentally during parsing. > Hmmm. What about switching the CurrentMemoryContext to the function's > long-lived context when we invoke plpgsql_yyparse(), but keeping it as > the short-lived context during the rest of the compilation process? Doesn't plpgsql_yyparse cover pretty much all of it? IIRC, a lot of code in pl_comp.c is called from either the lexing or parsing code. It's not nearly as well compartmentalized as in the main SQL parser :-( What might work is to run in the function context as the basic state, but switch to a short-term context when calling any main-backend code from pl_comp.c. I'm not sure how many such calls there are, but if it's not more than a dozen or two then this wouldn't be horridly painful. (he says, knowing *he* doesn't have to do it...) regards, tom lane ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] WIP: pl/pgsql cleanup
On Mon, 2005-02-07 at 10:41 -0500, Tom Lane wrote: > My recollection is that I was not nearly as worried about short-term > pallocs in the plpgsql code itself, as about leakage in various main- > backend routines that get called incidentally during parsing. > backend/parser/ is quite cavalier about this as a whole, because it > expects to be called in contexts that are not long-lived. Hmmm. What about switching the CurrentMemoryContext to the function's long-lived context when we invoke plpgsql_yyparse(), but keeping it as the short-lived context during the rest of the compilation process? This unfortunately complicates the memory management model still further, but it should significantly reduce the chance of any memory leaks. -Neil ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] again: add collation support function
John Hansen <[EMAIL PROTECTED]> writes: > It still does not answer his question, being if this is the way core > will want to go? I've been waiting to see other comments on it. I think this is certainly not the long-term solution, but if enough people think it is useful as a short-term hack then maybe it should go into contrib. (I suspec Marc's vote will be "put it in pgfoundry" in any case...) regards, tom lane ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] again: add collation support function
On Mon, 2005-02-07 at 12:55 -0500, Bruce Momjian wrote: > Ameen - Etemady wrote: > > Why you didn't reply this messages. > > > > I was searching for this one several hours, thanks for M.Taghizadeh. I > > think, if it place in contribution section of postgresql, it will avoid > > wasting several hours for peoples like me that wants this one. > > Does it have a bad implementaion? > > Sorry I think it is a very bad manner for postgresql master developers to > > leave such this message without any reply. > > I will appreciate if you reply this one. > > It still does not answer his question, being if this is the way core will want to go? > It was only posted seven days ago and will not appear until 8.1 so > people will have to keep looking for many more months no matter how > quickly we add it. > -- John Hansen <[EMAIL PROTECTED]> GeekNET ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] again: add collation support function
Ameen - Etemady wrote: > Why you didn't reply this messages. > > I was searching for this one several hours, thanks for M.Taghizadeh. I > think, if it place in contribution section of postgresql, it will avoid > wasting several hours for peoples like me that wants this one. > Does it have a bad implementaion? > Sorry I think it is a very bad manner for postgresql master developers to > leave such this message without any reply. > I will appreciate if you reply this one. > It was only posted seven days ago and will not appear until 8.1 so people will have to keep looking for many more months no matter how quickly we add it. -- 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 ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] WIP: pl/pgsql cleanup
Neil Conway <[EMAIL PROTECTED]> writes: > - abandonded the falloc() idea. There really aren't that many > short-lived allocations in the PL/PgSQL compiler, and using falloc() > made it difficult to use List. Instead, make the CurrentMemoryContext > the long-lived function context, and explicitly pfree short-term > allocations. Not _all_ short-lived allocations are explicitly released; > if this turns out to be a problem, it can be cleaned up later. My recollection is that I was not nearly as worried about short-term pallocs in the plpgsql code itself, as about leakage in various main- backend routines that get called incidentally during parsing. backend/parser/ is quite cavalier about this as a whole, because it expects to be called in contexts that are not long-lived. Have you done any checking to ensure that the per-function context doesn't get unreasonably bloated this way? regards, tom lane ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] again: add collation support function
Why you didn't reply this messages. I was searching for this one several hours, thanks for M.Taghizadeh. I think, if it place in contribution section of postgresql, it will avoid wasting several hours for peoples like me that wants this one. Does it have a bad implementaion? Sorry I think it is a very bad manner for postgresql master developers to leave such this message without any reply. I will appreciate if you reply this one. Best Regards. > Sorry I have forgotten to attach the implementaion of > nls_sort function. > > I kindly ask people who concern about support multiple > locale per column to verify this implementation and > fix it to be suitable for contribuation. > > Best Regards > M. Taghizade > ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
