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
