Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

2013-01-15 Thread Robert Haas
On Sun, Jan 13, 2013 at 4:16 PM, Tom Lane t...@sss.pgh.pa.us wrote: Andres Freund and...@2ndquadrant.com writes: Basically, the aspects of this that I think are likely to be reproducible wins across different platforms are (a) teaching the compiler that elog(ERROR) doesn't return, and (b)

Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

2013-01-15 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes: There are an awful lot of places in our source tree where the error level is fixed. We could invent a new construct, say ereport_error or so, that is just like ereport except that it takes no error-level parameter because it's hard-coded to ERROR.

Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

2013-01-13 Thread Andres Freund
On 2013-01-12 18:15:17 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2013-01-12 13:16:56 -0500, Tom Lane wrote: However, using a do-block with a local variable is definitely something worth considering. I'm getting less enamored of the __builtin_constant_p idea

Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

2013-01-13 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes: On 2013-01-12 15:39:16 -0500, Tom Lane wrote: This is actually a disadvantage of the proposal to replace the abort() calls with __builtin_unreachable(), too. The gcc boys interpret the semantics of that as if control reaches here, the behavior is

Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

2013-01-13 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes: On 2013-01-13 14:17:52 -0500, Tom Lane wrote: I find these numbers pretty hard to credit. There are quite some elog(DEBUG*)s in the backend, and those are taken, so I don't find it unreasonable that doing less work in those cases is measurable.

Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

2013-01-13 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes: Basically, the aspects of this that I think are likely to be reproducible wins across different platforms are (a) teaching the compiler that elog(ERROR) doesn't return, and (b) reducing code size as much as possible. The single-function change

Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

2013-01-12 Thread Tom Lane
Heikki Linnakangas hlinnakan...@vmware.com writes: On 11.01.2013 23:49, Tom Lane wrote: Hm ... well, that's a point. I may be putting too much weight on the fact that any such bug for elevel would be a new one that never existed before. What do others think? It sure would be nice to avoid

Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

2013-01-12 Thread Andres Freund
On 2013-01-12 13:16:56 -0500, Tom Lane wrote: Heikki Linnakangas hlinnakan...@vmware.com writes: On 11.01.2013 23:49, Tom Lane wrote: Hm ... well, that's a point. I may be putting too much weight on the fact that any such bug for elevel would be a new one that never existed before. What

Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

2013-01-12 Thread Heikki Linnakangas
On 12.01.2013 20:42, Andres Freund wrote: On 2013-01-12 13:16:56 -0500, Tom Lane wrote: Heikki Linnakangashlinnakan...@vmware.com writes: Here's one more construct to consider: #define ereport_domain(elevel, domain, rest) \ do { \ const int elevel_ = elevel; \

Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

2013-01-12 Thread Tom Lane
Heikki Linnakangas hlinnakan...@vmware.com writes: On 12.01.2013 20:42, Andres Freund wrote: I don't care for that too much in detail -- if errstart were to return false (it shouldn't, but if it did) this would be utterly broken, With the current ereport(), we'll call abort() if errstart

Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

2013-01-12 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes: On 2013-01-12 13:16:56 -0500, Tom Lane wrote: However, using a do-block with a local variable is definitely something worth considering. I'm getting less enamored of the __builtin_constant_p idea after finding out that the gcc boys seem to have

Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

2013-01-11 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes: On 2013-01-11 15:05:54 -0500, Tom Lane wrote: And another thing: what if the elevel argument isn't safe for multiple evaluation? No such hazard ever existed before these patches, so I'm not very comfortable with adding one. (Even if all our own

Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

2013-01-11 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes: On 2013-01-11 15:52:19 -0500, Tom Lane wrote: I agree the scenario doesn't seem all that probable, but what scares me here is that if we use __builtin_constant_p(elevel) (elevel) = ERROR in some builds, and just (elevel) = ERROR in others, then if

Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

2013-01-11 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes: On 2013-01-11 16:16:58 -0500, Tom Lane wrote: Uh ... because it's *not* unreachable if elevel ERROR. Otherwise we'd just mark errfinish as __attribute((noreturn)) and be done. Of course, that's a gcc-ism too. Well, I mean with the double

Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

2013-01-11 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes: On 2013-01-11 16:28:13 -0500, Tom Lane wrote: I'm not very satisfied with that answer. Right now, Peter's patch has added a class of bugs that never existed before 9.3, and yours would add more. It might well be that those classes are empty ...

[HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

2013-01-09 Thread Andres Freund
Hi, As promised here's a patch to provide palloc emulation for frontend-ish environments. The patch: - makes palloc() into a real function so CurrentMemoryContext doesn't need to be provided - provides common pg_(malloc,malloc0, realloc, strdup, free) wrappers and removes various versions of

Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

2013-01-09 Thread Heikki Linnakangas
On 09.01.2013 13:27, Andres Freund wrote: - makes palloc() into a real function so CurrentMemoryContext doesn't need to be provided I don't understand the need for this change. Can't you just: #define palloc(s) pg_malloc(s) in the frontend context? - Heikki -- Sent via pgsql-hackers

Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

2013-01-09 Thread Andres Freund
On 2013-01-09 13:46:53 +0200, Heikki Linnakangas wrote: On 09.01.2013 13:27, Andres Freund wrote: - makes palloc() into a real function so CurrentMemoryContext doesn't need to be provided I don't understand the need for this change. Can't you just: #define palloc(s) pg_malloc(s) in the

Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

2013-01-09 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes: On 2013-01-09 13:46:53 +0200, Heikki Linnakangas wrote: I don't understand the need for this change. Can't you just: #define palloc(s) pg_malloc(s) in the frontend context? Yes, that would be possible, but imo its the inferior solution: I'm with

Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

2013-01-09 Thread Andres Freund
On 2013-01-09 11:37:46 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2013-01-09 13:46:53 +0200, Heikki Linnakangas wrote: I don't understand the need for this change. Can't you just: #define palloc(s) pg_malloc(s) in the frontend context? Yes, that would be

Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

2013-01-09 Thread Tom Lane
Andres Freund and...@anarazel.de writes: On 2013-01-09 11:37:46 -0500, Tom Lane wrote: I agree that what dirmod is doing is pretty ugly, but it's localized enough that I don't care particularly. (Really, the only reason it's a hack and not The Solution is that at the moment there's only one

Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

2013-01-09 Thread Tom Lane
Andres Freund and...@anarazel.de writes: On 2013-01-09 11:57:35 -0500, Tom Lane wrote: My objection is that you're forcing *all* backend code to go through the trampoline. That probably has a negative impact on performance, and if you think it'll get committed without a thorough proof that

Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

2013-01-09 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes: On 2013-01-09 15:43:19 -0500, Tom Lane wrote: I had thought of proposing that we code palloc() like this: void * palloc(Size size) { MemoryContext context = CurrentMemoryContext; AssertArg(MemoryContextIsValid(context)); if

Re: [HACKERS] [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

2013-01-09 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes: I wonder whether it makes sense to inline the contents pstrdup() additionally? My gut feeling is not, but... I don't recall ever having seen MemoryContextStrdup amount to much, so probably not worth the extra code space.