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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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 betweenT_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
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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 betweenT_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_BaseBackupCmdSo 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
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers