Thread: MemoryContext and NodeTags

I'm rewriting parts of PL/Java to be more secure. One of the areas where 
I'd like to improve things concerns ownership of allocated structures. 
Many structures, such as TupleDesc, HeapTuple, ErrorData, etc. can be 
copied into another MemoryContext for safe keeping. PL/Java uses this 
when creating Java wrappers for such objects.

Prior to the rewrite, I maintained mappings from pointers to Java 
wrappers in a hash table. Now, I instead have a special MemoryContext 
that can hold a reference to the Java wrapper in the chunk header. Both 
simpler and more efficient (I also have wet dreams about a future 
MemoryContext that allocates shared memory). But, at present, and 
because of this macro:
 /*  * MemoryContextIsValid  *        True iff memory context is valid.  *  * Add new context types to the set accepted
bythis macro.  */ #define MemoryContextIsValid(context) \     ((context) != NULL && \      (IsA((context),
AllocSetContext)))

I have to cheat and claim that this MemoryContext has the NodeType of 
T_AllocSetContext.

I have a proposal:
The NodeTag T_MemoryContext has the value of 600 and the next occupied 
entry is T_Value which is 650.
- Reserve half of that range for PostgreSQL specific contexts (today you 
only use one), and the other half for custom contexts.
- Change the above macro to consider values between 601 and 649 as valid 
tags. The likelihood of an invalid context hitting that range is second 
to none.
- Accept patches to nodes/nodes.h for new custom tags (properly 
motivated of course).

What do you think? Would a patch that implements this proposal and adds 
a T_PLJavaContext NodeTag be accepted?

Regards,
Thomas Hallgren

Thomas Hallgren <thomas.hallgren@home.se> writes:
> I have a proposal:
> The NodeTag T_MemoryContext has the value of 600 and the next occupied 
> entry is T_Value which is 650.
> - Reserve half of that range for PostgreSQL specific contexts (today you 
> only use one), and the other half for custom contexts.

OK.

> - Accept patches to nodes/nodes.h for new custom tags (properly 
> motivated of course).

No.  Define 'em yourself.
        regards, tom lane

tgl@sss.pgh.pa.us wrote:
> Thomas Hallgren <thomas.hallgren@home.se> writes:
>   
>> I have a proposal:
>> The NodeTag T_MemoryContext has the value of 600 and the next occupied 
>> entry is T_Value which is 650.
>> - Reserve half of that range for PostgreSQL specific contexts (today you 
>> only use one), and the other half for custom contexts.
>>     
>
> OK.
>
>   
>> - Accept patches to nodes/nodes.h for new custom tags (properly 
>> motivated of course).
>>     
>
> No.  Define 'em yourself.
>   
OK, I can do that. But I have a couple of reasons why I think that it 
would be a good idea to get my definitions into node.h:
- If more module authors want to do similar things, they would not risk 
defining overlapping tags.
- The NodeTag is an enum. Code that defines tags that are supposed to 
"fit in" becomes ugly.
- The IsA macro can be used.
- You (PostgreSQL core) want full control over the tags. If all tags are 
in nodes.h, you can move tags to other number ranges without creating a 
hassle for people like me.

Regards,
Thomas Hallgren.

Thomas Hallgren <thomas.hallgren@home.se> writes:
> tgl@sss.pgh.pa.us wrote:
>> No.  Define 'em yourself.
>> 
> OK, I can do that. But I have a couple of reasons why I think that it 
> would be a good idea to get my definitions into node.h:
> - If more module authors want to do similar things, they would not risk 
> defining overlapping tags.

Only for those module authors who manage to get their tags accepted;
and even for them, only for PG versions later than when they start
working.  Not much of an extension mechanism, is it?

> - The NodeTag is an enum. Code that defines tags that are supposed to 
> "fit in" becomes ugly.

I don't see anyone trying to "switch" over MemoryContext tags, so this
is really pretty irrelevant.  AFAICS it should work just fine to do

#define T_FooNode  ((NodeTag) (T_FirstPrivateNode + 1))

> - The IsA macro can be used.

Still can AFAICS --- that macro knows nothing about the enum, just about
the convention that Foo and T_Foo are related names.

> - You (PostgreSQL core) want full control over the tags. If all tags are 
> in nodes.h, you can move tags to other number ranges without creating a 
> hassle for people like me.

As long as you define your tag as T_Something + N, that still holds.
        regards, tom lane

tgl@sss.pgh.pa.us wrote:
>
> Not much of an extension mechanism, is it?
>
>   
Yes it is. If you are a module writer and want to define your own tag, 
the chances are pretty hight that you would look for available tags in 
the current CVS HEAD before you go ahead, thus avoiding any collision. 
First come, first served. You will of course need #ifdef's for backward 
compatibility but at some point in time, they can be removed (as I now 
do with a lot of stuff for 7.x and custom variable classes as I'm 
dropping the 7.x support).

If you don't get your tag accepted then you're on your own of course. 
Then again, if the core team has reservations to accepting your tag 
there's a bigger problem somewhere.
> I don't see anyone trying to "switch" over MemoryContext tags, so this
> is really pretty irrelevant.  AFAICS it should work just fine to do
>
> #define T_FooNode  ((NodeTag) (T_FirstPrivateNode + 1))
>   
I don't see how #define rectifies ugliness. It's horrible in the 
debugger and it screws up code-completion etc. in any IDE. Not being 
able to use a "switch" is a minor problem.

>> - You (PostgreSQL core) want full control over the tags. If all tags are 
>> in nodes.h, you can move tags to other number ranges without creating a 
>> hassle for people like me.
>>     
>
> As long as you define your tag as T_Something + N, that still holds.
>   
No, that's false. Assume a range is full and you need to expand it. 
Someone within that range has to move. Everyone uses the same T_Something...

Regards,
Thomas Hallgren