Thread: [HACKERS] Duplicate node tag assignments
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. 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. Thoughts?
regards, tom lane
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
Andres Freund <andres@anarazel.de> 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
On Wed, Dec 28, 2016 at 10:03 PM, Tom Lane <tgl@sss.pgh.pa.us> 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
Amit Kapila wrote: > On Wed, Dec 28, 2016 at 10:03 PM, Tom Lane <tgl@sss.pgh.pa.us> 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 Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services