Re: [HACKERS] refactoring comment.c

2010-08-28 Thread Martijn van Oosterhout
On Fri, Aug 27, 2010 at 09:35:55PM -0400, Tom Lane wrote: Alvaro Herrera alvhe...@commandprompt.com writes: Didn't we inject some smarts so that the compiler would notice that elog(ERROR) doesn't return? No. If you know a portable (as in works on every compiler) way to do that, we could

Re: [HACKERS] refactoring comment.c

2010-08-27 Thread Robert Haas
On Fri, Aug 27, 2010 at 12:52 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Thu, Aug 19, 2010 at 3:34 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: Any other kibitzing before I commit this? Sure ... [kibitzing] v4.

Re: [HACKERS] refactoring comment.c

2010-08-27 Thread Peter Eisentraut
On fre, 2010-08-27 at 07:49 -0400, Robert Haas wrote: Anyway, committed. I suppose this is responsible for this: gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing -fwrapv -g -Werror -Wno-inline -I../../../src/include

Re: [HACKERS] refactoring comment.c

2010-08-27 Thread Robert Haas
On Fri, Aug 27, 2010 at 4:16 PM, Peter Eisentraut pete...@gmx.net wrote: On fre, 2010-08-27 at 07:49 -0400, Robert Haas wrote: Anyway, committed. I suppose this is responsible for this: gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels

Re: [HACKERS] refactoring comment.c

2010-08-27 Thread Kevin Grittner
Robert Haas robertmh...@gmail.com wrote: I suppose this is unhappy because it things elog(ERROR) might return? It looks more like this code uses it without initialization: case OBJECT_INDEX: case OBJECT_SEQUENCE: case OBJECT_TABLE: case OBJECT_VIEW:

Re: [HACKERS] refactoring comment.c

2010-08-27 Thread Robert Haas
On Fri, Aug 27, 2010 at 5:35 PM, Kevin Grittner kevin.gritt...@wicourts.gov wrote: Robert Haas robertmh...@gmail.com wrote: I suppose this is unhappy because it things elog(ERROR) might return? It looks more like this code uses it without initialization:        case OBJECT_INDEX:        

Re: [HACKERS] refactoring comment.c

2010-08-27 Thread Kevin Grittner
Robert Haas robertmh...@gmail.com wrote: OK, I must be missing something. Go through that a little slower? No, my mistake. Please ignore. I am getting the same warnings, but I think your take on the cause must be right. -Kevin -- Sent via pgsql-hackers mailing list

Re: [HACKERS] refactoring comment.c

2010-08-27 Thread Kevin Grittner
Robert Haas robertmh...@gmail.com wrote: I suppose this is unhappy because it things elog(ERROR) might return? If I set these fields right in front of the elog(ERROR) the warnings go away for me. (Hopefully this observation will make up, to some extent, for my earlier brain fart.) -Kevin

Re: [HACKERS] refactoring comment.c

2010-08-27 Thread Robert Haas
On Fri, Aug 27, 2010 at 5:57 PM, Kevin Grittner kevin.gritt...@wicourts.gov wrote: Robert Haas robertmh...@gmail.com wrote: I suppose this is unhappy because it things elog(ERROR) might return? If I set these fields right in front of the elog(ERROR) the warnings go away for me.  (Hopefully

Re: [HACKERS] refactoring comment.c

2010-08-27 Thread Kevin Grittner
Robert Haas robertmh...@gmail.com wrote: I set them right after the ERROR so that those statements needn't actually be executed. Good thinking. I checked and that also eliminates the warnings for me. (No surprise there, of course.) -Kevin -- Sent via pgsql-hackers mailing list

Re: [HACKERS] refactoring comment.c

2010-08-27 Thread Alvaro Herrera
Excerpts from Robert Haas's message of vie ago 27 18:07:55 -0400 2010: On Fri, Aug 27, 2010 at 5:57 PM, Kevin Grittner kevin.gritt...@wicourts.gov wrote: Robert Haas robertmh...@gmail.com wrote: I suppose this is unhappy because it things elog(ERROR) might return? If I set these fields

Re: [HACKERS] refactoring comment.c

2010-08-27 Thread Tom Lane
Alvaro Herrera alvhe...@commandprompt.com writes: Didn't we inject some smarts so that the compiler would notice that elog(ERROR) doesn't return? No. If you know a portable (as in works on every compiler) way to do that, we could talk. If only some compilers understand it, we'll probably end

Re: [HACKERS] refactoring comment.c

2010-08-27 Thread Robert Haas
On Fri, Aug 27, 2010 at 9:35 PM, Tom Lane t...@sss.pgh.pa.us wrote: Alvaro Herrera alvhe...@commandprompt.com writes: Didn't we inject some smarts so that the compiler would notice that elog(ERROR) doesn't return? No.  If you know a portable (as in works on every compiler) way to do that, we

Re: [HACKERS] refactoring comment.c

2010-08-27 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes: What's a bit odd about this is that I do get warnings for this kind of thing in general, which get turned into errors since I compile with -Werror; and in fact the version of the patch that I committed has no fewer than four places where there is a

Re: [HACKERS] refactoring comment.c

2010-08-27 Thread Robert Haas
On Fri, Aug 27, 2010 at 9:49 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: What's a bit odd about this is that I do get warnings for this kind of thing in general, which get turned into errors since I compile with -Werror; and in fact the version of the patch

Re: [HACKERS] refactoring comment.c

2010-08-26 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes: On Thu, Aug 19, 2010 at 3:34 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: Any other kibitzing before I commit this? Sure ... [kibitzing] v4. For my money, you could just have said Applied with suggested

Re: [HACKERS] refactoring comment.c

2010-08-19 Thread Alvaro Herrera
Excerpts from Robert Haas's message of mié ago 18 21:32:48 -0400 2010: Here's v3. The header comment in objectaddress.c contains a funny mistake: it says it works with ObjectAddresses. However, ObjectAddresses is a different type altogether, so I recommend not using that as plural for

Re: [HACKERS] refactoring comment.c

2010-08-19 Thread Tom Lane
Alvaro Herrera alvhe...@commandprompt.com writes: Excerpts from Robert Haas's message of mié ago 18 21:32:48 -0400 2010: Here's v3. The header comment in objectaddress.c contains a funny mistake: it says it works with ObjectAddresses. However, ObjectAddresses is a different type

Re: [HACKERS] refactoring comment.c

2010-08-19 Thread Robert Haas
On Thu, Aug 19, 2010 at 11:57 AM, Tom Lane t...@sss.pgh.pa.us wrote: Alvaro Herrera alvhe...@commandprompt.com writes: Excerpts from Robert Haas's message of mié ago 18 21:32:48 -0400 2010: Here's v3. The header comment in objectaddress.c contains a funny mistake: it says it works with

Re: [HACKERS] refactoring comment.c

2010-08-19 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes: Any other kibitzing before I commit this? Sure ... + * If the object is a relation or a child object of a relation (e.g. an + * attribute or contraint, *relp will set to point to that relation). This Parenthesis in the wrong place here, grammar and

Re: [HACKERS] refactoring comment.c

2010-08-17 Thread Tom Lane
I wrote: Maybe so, but the parser is expected to put out a representation that will still be valid when the command is executed some time later. Rereading this, I see I didn't make my point very clearly. The reason this code doesn't belong in parser/ is that there's no prospect the parser

Re: [HACKERS] refactoring comment.c

2010-08-17 Thread Robert Haas
On Tue, Aug 17, 2010 at 11:30 AM, Tom Lane t...@sss.pgh.pa.us wrote: I wrote: Maybe so, but the parser is expected to put out a representation that will still be valid when the command is executed some time later. Rereading this, I see I didn't make my point very clearly.  The reason this

Re: [HACKERS] refactoring comment.c

2010-08-17 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes: On Tue, Aug 17, 2010 at 11:30 AM, Tom Lane t...@sss.pgh.pa.us wrote: Rereading this, I see I didn't make my point very clearly.  The reason this code doesn't belong in parser/ is that there's no prospect the parser itself would ever use it.  

Re: [HACKERS] refactoring comment.c

2010-08-17 Thread Robert Haas
On Tue, Aug 17, 2010 at 2:24 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Tue, Aug 17, 2010 at 11:30 AM, Tom Lane t...@sss.pgh.pa.us wrote: Rereading this, I see I didn't make my point very clearly.  The reason this code doesn't belong in parser/ is that

Re: [HACKERS] refactoring comment.c

2010-08-17 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes: On Tue, Aug 17, 2010 at 2:24 PM, Tom Lane t...@sss.pgh.pa.us wrote: I don't insist that the separation has to be crisp.  I'm merely saying that putting a large chunk of useful-only-at-execution-time code into backend/parser is the Wrong Thing. OK,

Re: [HACKERS] refactoring comment.c

2010-08-16 Thread Alvaro Herrera
Excerpts from KaiGai Kohei's message of lun ago 16 00:19:54 -0400 2010: (2010/08/16 11:50), Robert Haas wrote: When we were developing largeobject access controls, Tom Lane commented as follows: * Re: [HACKERS] [PATCH] Largeobject access controls

Re: [HACKERS] refactoring comment.c

2010-08-16 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes: 5. Since I'm hoping Tom will read this, I ran it through filterdiff. :-) OK, I looked ;-). It mostly looks good, but of course I've got some opinions... 2. I haven't done anything about moving the definition of ObjectAddress elsewhere, as Alvaro

Re: [HACKERS] refactoring comment.c

2010-08-16 Thread Robert Haas
On Mon, Aug 16, 2010 at 3:48 PM, Tom Lane t...@sss.pgh.pa.us wrote: 2. I haven't done anything about moving the definition of ObjectAddress elsewhere, as Alvaro suggested, because I'm not sure quite where it ought to go.  I still think it's a good idea, though I'm not dead set on it, either.  

Re: [HACKERS] refactoring comment.c

2010-08-16 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes: On Mon, Aug 16, 2010 at 3:48 PM, Tom Lane t...@sss.pgh.pa.us wrote: I think the problem is you're trying to put this into backend/parser which is not really the right place for it. If this isn't parse analysis, then you and I have very different ideas

Re: [HACKERS] refactoring comment.c

2010-08-15 Thread KaiGai Kohei
(2010/08/16 11:50), Robert Haas wrote: On Fri, Aug 6, 2010 at 11:15 PM, KaiGai Koheikai...@kaigai.gr.jp wrote: [brief review] OK, here's an updated patch: 1. I fixed the typo Alvaro spotted. 2. I haven't done anything about moving the definition of ObjectAddress elsewhere, as Alvaro

Re: [HACKERS] refactoring comment.c

2010-08-06 Thread Alvaro Herrera
Excerpts from Robert Haas's message of vie ago 06 11:02:58 -0400 2010: Any comments? (ha ha ha...) Interesting idea. The patch looks fine on a quick once-over. Two small things: this comment +/* + * Databases, tablespaces, and roles are cluster-wide objects, so any + * comments

Re: [HACKERS] refactoring comment.c

2010-08-06 Thread Robert Haas
On Fri, Aug 6, 2010 at 11:36 AM, Alvaro Herrera alvhe...@commandprompt.com wrote: Excerpts from Robert Haas's message of vie ago 06 11:02:58 -0400 2010: Any comments?  (ha ha ha...) Interesting idea.  The patch looks fine on a quick once-over. Thanks for taking a look. Two small things:

Re: [HACKERS] refactoring comment.c

2010-08-06 Thread Simon Riggs
On Fri, 2010-08-06 at 11:02 -0400, Robert Haas wrote: At PGCon, we discussed the possibility that a minimal SE-PostgreSQL implementation would need little more than a hook in ExecCheckRTPerms() [which we've since added] and a security label facility [for which KaiGai has submitted a patch]. I

Re: [HACKERS] refactoring comment.c

2010-08-06 Thread Robert Haas
On Fri, Aug 6, 2010 at 12:26 PM, Simon Riggs si...@2ndquadrant.com wrote: I understand the concept and it seems like it might work. Not too keen on pretending a noun is a verb. That leads to erroring. verb SECURITY LABEL? verb = CREATE, ADD, ... Can't objects have more than one label? How

Re: [HACKERS] refactoring comment.c

2010-08-06 Thread KaiGai Kohei
(2010/08/07 0:02), Robert Haas wrote: At PGCon, we discussed the possibility that a minimal SE-PostgreSQL implementation would need little more than a hook in ExecCheckRTPerms() [which we've since added] and a security label facility [for which KaiGai has submitted a patch]. I actually sat down