Re: [HACKERS] [PATCH 2/5] Make relpathbackend return a statically result instead of palloc()'ing it
Andres Freund and...@2ndquadrant.com writes: maxpg From: Andres Freund and...@anarazel.de relpathbackend() (via some of its wrappers) is used in *_desc routines which we want to be useable without a backend environment arround. I'm 100% unimpressed with making relpathbackend return a pointer to a static buffer. Who's to say whether that won't create bugs due to overlapping usages? Change signature to return a 'const char *' to make misuse easier to detect. That seems to create way more churn than is necessary, and it's wrong anyway if the result is palloc'd. 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] [PATCH 2/5] Make relpathbackend return a statically result instead of palloc()'ing it
On 2013-01-08 14:28:14 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: maxpg From: Andres Freund and...@anarazel.de relpathbackend() (via some of its wrappers) is used in *_desc routines which we want to be useable without a backend environment arround. I'm 100% unimpressed with making relpathbackend return a pointer to a static buffer. Who's to say whether that won't create bugs due to overlapping usages? I say it ;). I've gone through all callers and checked. Not that that guarantees anything, but ... The reason a static buffer is better is that some of the *desc routines use relpathbackend() and pfree() the result. That would require providing a stub pfree() in xlogdump which seems to be exceedingly ugly. Change signature to return a 'const char *' to make misuse easier to detect. That seems to create way more churn than is necessary, and it's wrong anyway if the result is palloc'd. It causes warnings in potential external users that pfree() the result of relpathbackend which seems helpful. Obviously only makes sense if relpathbackend() returns a pointer into a static buffer... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- 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] [PATCH 2/5] Make relpathbackend return a statically result instead of palloc()'ing it
Andres Freund and...@2ndquadrant.com writes: On 2013-01-08 14:28:14 -0500, Tom Lane wrote: I'm 100% unimpressed with making relpathbackend return a pointer to a static buffer. Who's to say whether that won't create bugs due to overlapping usages? I say it ;). I've gone through all callers and checked. Not that that guarantees anything, but ... Even if you've proven it safe today, it seems unnecessarily fragile. Just about any other place we've used a static result buffer, we've regretted it, unless the use cases were *very* narrow. The reason a static buffer is better is that some of the *desc routines use relpathbackend() and pfree() the result. That would require providing a stub pfree() in xlogdump which seems to be exceedingly ugly. Why? If we have palloc support how hard can it be to map pfree to free? And why wouldn't we want to? I can hardly imagine providing only palloc and not pfree support. 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] [PATCH 2/5] Make relpathbackend return a statically result instead of palloc()'ing it
On 2013-01-08 14:53:29 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2013-01-08 14:28:14 -0500, Tom Lane wrote: I'm 100% unimpressed with making relpathbackend return a pointer to a static buffer. Who's to say whether that won't create bugs due to overlapping usages? I say it ;). I've gone through all callers and checked. Not that that guarantees anything, but ... Even if you've proven it safe today, it seems unnecessarily fragile. Just about any other place we've used a static result buffer, we've regretted it, unless the use cases were *very* narrow. Hm, relpathbackend seems pretty narrow to me. Funny, we both argued the other way round than we are now when talking about the sprintf(..., %X/%X, (uint32)(recptr 32), (uint32)recptr) thingy ;) The reason a static buffer is better is that some of the *desc routines use relpathbackend() and pfree() the result. That would require providing a stub pfree() in xlogdump which seems to be exceedingly ugly. Why? If we have palloc support how hard can it be to map pfree to free? And why wouldn't we want to? I can hardly imagine providing only palloc and not pfree support. Uhm, we don't have need palloc support and I don't think relpathbackend() is a good justification for adding it. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- 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] [PATCH 2/5] Make relpathbackend return a statically result instead of palloc()'ing it
Andres Freund and...@2ndquadrant.com writes: Uhm, we don't have need palloc support and I don't think relpathbackend() is a good justification for adding it. I've said from the very beginning of this effort that it would be impossible to share any meaningful amount of code between frontend and backend environments without adding some sort of emulation of palloc/pfree/elog. I think this patch is just making the code uglier and more fragile to put off the inevitable, and that we'd be better served to bite the bullet and do that. 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] [PATCH 2/5] Make relpathbackend return a statically result instead of palloc()'ing it
Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: Uhm, we don't have need palloc support and I don't think relpathbackend() is a good justification for adding it. I've said from the very beginning of this effort that it would be impossible to share any meaningful amount of code between frontend and backend environments without adding some sort of emulation of palloc/pfree/elog. I think this patch is just making the code uglier and more fragile to put off the inevitable, and that we'd be better served to bite the bullet and do that. As far as this patch is concerned, I think it's sufficient to do #define palloc(x) malloc(x) #define pfree(x) free(x) -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- 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] [PATCH 2/5] Make relpathbackend return a statically result instead of palloc()'ing it
On 2013-01-08 15:27:23 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: Uhm, we don't have need palloc support and I don't think relpathbackend() is a good justification for adding it. I've said from the very beginning of this effort that it would be impossible to share any meaningful amount of code between frontend and backend environments without adding some sort of emulation of palloc/pfree/elog. I think this patch is just making the code uglier and more fragile to put off the inevitable, and that we'd be better served to bite the bullet and do that. If you think relpathbackend() alone warrants that, yes, I can provide a wrapper. Everything else is imo already handled in a sensible and not really ugly manner? Imo its not worth the effort *for this alone*. I already had some elog(), ereport(), whatever emulation but Heikki preferred not to have it, so its removed by now. To what extent do you want palloc et al. emulation? Provide actual pools or just make redirect to malloc and provide the required symbols (at the very least CurrentMemoryContext)? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- 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] [PATCH 2/5] Make relpathbackend return a statically result instead of palloc()'ing it
Andres Freund and...@2ndquadrant.com writes: To what extent do you want palloc et al. emulation? Provide actual pools or just make redirect to malloc and provide the required symbols (at the very least CurrentMemoryContext)? I don't see any need for memory pools, at least not for frontend applications of the currently envisioned levels of complexity. I concur with Alvaro's suggestion about just #define'ing them to malloc/free --- or maybe better, pg_malloc/free so that we can have a failure-checking wrapper. Not sure how we ought to handle elog, but maybe we can put off that bit of design until we have a more concrete use-case. 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] [PATCH 2/5] Make relpathbackend return a statically result instead of palloc()'ing it
On 08.01.2013 22:39, Andres Freund wrote: On 2013-01-08 15:27:23 -0500, Tom Lane wrote: Andres Freundand...@2ndquadrant.com writes: Uhm, we don't have need palloc support and I don't think relpathbackend() is a good justification for adding it. I've said from the very beginning of this effort that it would be impossible to share any meaningful amount of code between frontend and backend environments without adding some sort of emulation of palloc/pfree/elog. I think this patch is just making the code uglier and more fragile to put off the inevitable, and that we'd be better served to bite the bullet and do that. If you think relpathbackend() alone warrants that, yes, I can provide a wrapper. Everything else is imo already handled in a sensible and not really ugly manner? Imo its not worth the effort *for this alone*. I already had some elog(), ereport(), whatever emulation but Heikki preferred not to have it, so its removed by now. To what extent do you want palloc et al. emulation? Provide actual pools or just make redirect to malloc and provide the required symbols (at the very least CurrentMemoryContext)? Note that the xlogreader facility doesn't need any more emulation. I'm quite satisfied with that part of the patch now. However, the rmgr desc routines do, and I'm not very happy with those. Not sure what to do about it. As you said, we could add enough infrastructure to make them compile in frontend context, or we could try to make them rely less on backend functions. One consideration is that once we have a separate pg_xlogdump utility, I expect that to be the most visible consumer of the rmgr desc functions. The backend will of course still use those functions in e.g error messages, but those don't happen very often. Not sure how that should be taken into account in this patch, but I thought I'd mention it. An rmgr desc routine probably shouldn't be calling elog() even in the backend, because you don't want to throw errors in the contexts where those routines are called. - Heikki -- 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] [PATCH 2/5] Make relpathbackend return a statically result instead of palloc()'ing it
On 2013-01-08 22:47:43 +0200, Heikki Linnakangas wrote: On 08.01.2013 22:39, Andres Freund wrote: On 2013-01-08 15:27:23 -0500, Tom Lane wrote: Andres Freundand...@2ndquadrant.com writes: Uhm, we don't have need palloc support and I don't think relpathbackend() is a good justification for adding it. I've said from the very beginning of this effort that it would be impossible to share any meaningful amount of code between frontend and backend environments without adding some sort of emulation of palloc/pfree/elog. I think this patch is just making the code uglier and more fragile to put off the inevitable, and that we'd be better served to bite the bullet and do that. If you think relpathbackend() alone warrants that, yes, I can provide a wrapper. Everything else is imo already handled in a sensible and not really ugly manner? Imo its not worth the effort *for this alone*. I already had some elog(), ereport(), whatever emulation but Heikki preferred not to have it, so its removed by now. To what extent do you want palloc et al. emulation? Provide actual pools or just make redirect to malloc and provide the required symbols (at the very least CurrentMemoryContext)? Note that the xlogreader facility doesn't need any more emulation. I'm quite satisfied with that part of the patch now. However, the rmgr desc routines do, and I'm not very happy with those. Not sure what to do about it. As you said, we could add enough infrastructure to make them compile in frontend context, or we could try to make them rely less on backend functions. Which emulation needs are you missing in the desc routines besides relpathbackend() and timestamptz_to_str()? pfree() is just needed because its used to free the result of relpathbackend(). No own pallocs, no ereport ... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- 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] [PATCH 2/5] Make relpathbackend return a statically result instead of palloc()'ing it
On 08.01.2013 23:00, Andres Freund wrote: Note that the xlogreader facility doesn't need any more emulation. I'm quite satisfied with that part of the patch now. However, the rmgr desc routines do, and I'm not very happy with those. Not sure what to do about it. As you said, we could add enough infrastructure to make them compile in frontend context, or we could try to make them rely less on backend functions. Which emulation needs are you missing in the desc routines besides relpathbackend() and timestamptz_to_str()? pfree() is just needed because its used to free the result of relpathbackend(). No own pallocs, no ereport ... Nothing besides those, those are the stuff I was referring to. - Heikki -- 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] [PATCH 2/5] Make relpathbackend return a statically result instead of palloc()'ing it
On 2013-01-08 15:45:07 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: To what extent do you want palloc et al. emulation? Provide actual pools or just make redirect to malloc and provide the required symbols (at the very least CurrentMemoryContext)? I don't see any need for memory pools, at least not for frontend applications of the currently envisioned levels of complexity. I concur with Alvaro's suggestion about just #define'ing them to malloc/free --- or maybe better, pg_malloc/free so that we can have a failure-checking wrapper. Unless we want to introduce those into common headers, its more complex than #define's, you actually need to provide at least palloc/pfree/CurrentMemoryContext symbols. Still seems like a shame to do that for one lonely pfree() (+ something an eventual own implementation of relpathbackend(). Not sure how we ought to handle elog, but maybe we can put off that bit of design until we have a more concrete use-case. Agreed. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- 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] [PATCH 2/5] Make relpathbackend return a statically result instead of palloc()'ing it
On Tue, Jan 8, 2013 at 3:00 PM, Andres Freund and...@2ndquadrant.com wrote: Uhm, we don't have need palloc support and I don't think relpathbackend() is a good justification for adding it. FWIW, I'm with Tom on this one. Any meaningful code sharing is going to need that, so we might as well bite the bullet. And functions that return static buffers are evil incarnate. I've spent way too much of my life dealing with the supreme idiocy that is fmtId(). If someone ever finds a way to make that go away, I will buy them a beverage of their choice at the next conference we're both at. -- 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] [PATCH 2/5] Make relpathbackend return a statically result instead of palloc()'ing it
Robert Haas escribió: And functions that return static buffers are evil incarnate. I've spent way too much of my life dealing with the supreme idiocy that is fmtId(). +1 If someone ever finds a way to make that go away, I will buy them a beverage of their choice at the next conference we're both at. +1 -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- 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] [PATCH 2/5] Make relpathbackend return a statically result instead of palloc()'ing it
On 2013-01-08 17:28:33 -0500, Robert Haas wrote: On Tue, Jan 8, 2013 at 3:00 PM, Andres Freund and...@2ndquadrant.com wrote: Uhm, we don't have need palloc support and I don't think relpathbackend() is a good justification for adding it. FWIW, I'm with Tom on this one. Any meaningful code sharing is going to need that, so we might as well bite the bullet. Yes, if we set the scope bigger than xlogreader I aggree. If its xlogreader itself I don't. But as I happen to think we should share more code... Will prepare a patch. I wonder whether it would be acceptable to make palloc() an actual function instead of #define palloc(sz) MemoryContextAlloc(CurrentMemoryContext, (sz)) so we don't have to expose CurrentMemoryContext? Alternatively we can just move the whole of utils/mmgr/* to port, but that would imply an elog/ereport wrapper... And functions that return static buffers are evil incarnate. I've spent way too much of my life dealing with the supreme idiocy that is fmtId(). If someone ever finds a way to make that go away, I will buy them a beverage of their choice at the next conference we're both at. Imo it depends on the circumstances and number of possible callers, but anyway, it seems to be already decided that my suggestion isn't the way to go. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- 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] [PATCH 2/5] Make relpathbackend return a statically result instead of palloc()'ing it
Robert Haas robertmh...@gmail.com writes: And functions that return static buffers are evil incarnate. I've spent way too much of my life dealing with the supreme idiocy that is fmtId(). If someone ever finds a way to make that go away, I will buy them a beverage of their choice at the next conference we're both at. Yeah, that was exactly the case that was top-of-mind when I was complaining about static return buffers upthread. It's not hard to make the ugliness go away: just let it strdup its return value. The problem is that in the vast majority of usages it wouldn't be convenient to free the result, so we'd have a good deal of memory leakage. What might be interesting is to instrument it to see how much (adding a counter to the function ought to be easy enough) and then find out whether it's an amount we still care about in 2013. Frankly, pg_dump is a memory hog already - a few more identifier-sized strings laying about might not matter anymore. (Wanders away wondering how many relpathbackend callers bother to free its result, and whether that matters either ...) 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] [PATCH 2/5] Make relpathbackend return a statically result instead of palloc()'ing it
On Tue, Jan 8, 2013 at 6:23 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: And functions that return static buffers are evil incarnate. I've spent way too much of my life dealing with the supreme idiocy that is fmtId(). If someone ever finds a way to make that go away, I will buy them a beverage of their choice at the next conference we're both at. Yeah, that was exactly the case that was top-of-mind when I was complaining about static return buffers upthread. It's not hard to make the ugliness go away: just let it strdup its return value. The problem is that in the vast majority of usages it wouldn't be convenient to free the result, so we'd have a good deal of memory leakage. What might be interesting is to instrument it to see how much (adding a counter to the function ought to be easy enough) and then find out whether it's an amount we still care about in 2013. Frankly, pg_dump is a memory hog already - a few more identifier-sized strings laying about might not matter anymore. (Wanders away wondering how many relpathbackend callers bother to free its result, and whether that matters either ...) I was thinking more about a sprintf()-type function that only understands a handful of escapes, but adds the additional and novel escapes %I (quote as identifier) and %L (quote as literal). I think that would allow a great deal of code simplification, and it'd be more efficient, too. -- 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] [PATCH 2/5] Make relpathbackend return a statically result instead of palloc()'ing it
Robert Haas robertmh...@gmail.com writes: I was thinking more about a sprintf()-type function that only understands a handful of escapes, but adds the additional and novel escapes %I (quote as identifier) and %L (quote as literal). I think that would allow a great deal of code simplification, and it'd be more efficient, too. Seems like a great idea. Are you offering to code it? Note that this wouldn't entirely fix the fmtId problem, as not all the uses of fmtId are directly in sprintf calls. Still, it might get rid of most of the places where it'd be painful to avoid a memory leak with a strdup'ing version of fmtId. 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] [PATCH 2/5] Make relpathbackend return a statically result instead of palloc()'ing it
On Tue, Jan 8, 2013 at 7:57 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: I was thinking more about a sprintf()-type function that only understands a handful of escapes, but adds the additional and novel escapes %I (quote as identifier) and %L (quote as literal). I think that would allow a great deal of code simplification, and it'd be more efficient, too. Seems like a great idea. Are you offering to code it? Not imminently. Note that this wouldn't entirely fix the fmtId problem, as not all the uses of fmtId are directly in sprintf calls. Still, it might get rid of most of the places where it'd be painful to avoid a memory leak with a strdup'ing version of fmtId. Yeah, I didn't think about that. Might be worth a look to see how comprehensively it would solve the problem. But I'll have to leave that for another day. -- 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
