Re: [HACKERS] Duplicate node tag assignments

[Alvaro Herrera]

Amit Kapila wrote:
> On Wed, Dec 28, 2016 at 10:03 PM, Tom Lane  wrote:

> > Or we could just abandon the manually-assigned breaks in the list
> > altogether, and let NodeTags run from 1 to whatever.  This would
> > slightly complicate debugging, in that the numeric values of node
> > tags would change more than they used to, but on the whole that does
> > not sound like a large problem.
> 
> Yeah.  In most cases, during debugging I use the tag for typecasting
> the node to see the values of the particular node type.

I do likewise.  The actual numeric values don't matter to me one bit.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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] Duplicate node tag assignments

[Amit Kapila]

On Wed, Dec 28, 2016 at 10:03 PM, Tom Lane  wrote:
> By chance I happened to notice that the recent partition patch pushed
> us over the number of available node tags between
>
> T_A_Expr = 900,
>
> and
>
> T_TriggerData = 950,/* in commands/trigger.h */
>
> Specifically we now have some of the replication grammar node type
> codes conflicting with some of the "random other stuff" tags.
> It's not terribly surprising that that hasn't caused visible problems,
> because of the relatively localized use of replication grammar nodes,
> but it's still a Bad Thing.  And it's especially bad that apparently
> no one's compiler has complained about it.
>
> So we need to renumber enum NodeTag a bit, which is no big deal,
> but I think we had better take thought to prevent a recurrence
> (which might have worse consequences next time).
>
> One idea is to add StaticAsserts someplace asserting that there's
> still daylight at each manually-assigned break in the NodeTag list.
> But I'm not quite sure how to make that work.  For example, asserting
> that T_PlanInvalItem < T_PlanState won't help if people add new nodes
> after PlanInvalItem and don't think to update the Assert.
>
> Or we could just abandon the manually-assigned breaks in the list
> altogether, and let NodeTags run from 1 to whatever.  This would
> slightly complicate debugging, in that the numeric values of node
> tags would change more than they used to, but on the whole that does
> not sound like a large problem.
>

Yeah.  In most cases, during debugging I use the tag for typecasting
the node to see the values of the particular node type.

>  When you're working in gdb, say,
> it's easy enough to convert:
>
> (gdb) p (int) T_CreateReplicationSlotCmd
> $8 = 950
> (gdb) p (enum NodeTag) 949
> $9 = T_BaseBackupCmd
>
> So I'm leaning to the second, more drastic, solution.
>

Sounds sensible.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
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] Duplicate node tag assignments

[Tom Lane]

Andres Freund  writes:
> On 2016-12-28 11:33:31 -0500, Tom Lane wrote:
>> By chance I happened to notice that the recent partition patch pushed
>> us over the number of available node tags between
>> So I'm leaning to the second, more drastic, solution.  Thoughts?

> Alternatively we could add a -Wduplicate-enum/-Werror=duplicate-enum for
> clang. That'd protect against that in all enums...

Meh ... I'm not sure that we want to forbid it in *all* enums, just this
one.  Anyway, a lot of us are not using clang, and I could easily see
wasting a great deal of time identifying a bug caused by this sort of
conflict.

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] Duplicate node tag assignments

[Andres Freund]

Hi,

On 2016-12-28 11:33:31 -0500, Tom Lane wrote:
> By chance I happened to notice that the recent partition patch pushed
> us over the number of available node tags between
> 
>   T_A_Expr = 900,

> So I'm leaning to the second, more drastic, solution.  Thoughts?

Alternatively we could add a -Wduplicate-enum/-Werror=duplicate-enum for
clang. That'd protect against that in all enums...

Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers