Re: [HACKERS] object_classes array is broken, again
Any opinions on this idea? I don't like it all that much, but it's
plenty effective.
Alvaro Herrera wrote:
> The problem is that there aren't enough callers of add_object_address:
> there are many indexes of that array that aren't ever accessed and so
> it's not obvious when the array is broken. If we were to put
> OCLASS_CLASS at the end instead of at the beginning, that would fix the
> problem by making it immediately obvious when things get broken this
> way, because the value used in the most common case would shift around
> every time we add another value. (Of course, we'd have to instruct
> people to not add new members after the pg_class entry.)
> diff --git a/src/backend/catalog/dependency.c
> b/src/backend/catalog/dependency.c
> index c1212e9..0107c53 100644
> --- a/src/backend/catalog/dependency.c
> +++ b/src/backend/catalog/dependency.c
> @@ -127,7 +127,6 @@ typedef struct
> * See also getObjectClass().
> */
> static const Oid object_classes[MAX_OCLASS] = {
> - RelationRelationId, /* OCLASS_CLASS */
> ProcedureRelationId,/* OCLASS_PROC */
> TypeRelationId, /* OCLASS_TYPE */
> CastRelationId, /* OCLASS_CAST */
> @@ -158,7 +157,9 @@ static const Oid object_classes[MAX_OCLASS] = {
> DefaultAclRelationId, /* OCLASS_DEFACL */
> ExtensionRelationId,/* OCLASS_EXTENSION */
> EventTriggerRelationId, /* OCLASS_EVENT_TRIGGER */
> - PolicyRelationId/* OCLASS_POLICY */
> + PolicyRelationId, /* OCLASS_POLICY */
> + TransformRelationId,/* OCLASS_POLICY */
> + RelationRelationId /* OCLASS_CLASS */
> };
>
>
> diff --git a/src/include/catalog/dependency.h
> b/src/include/catalog/dependency.h
> index 5da18c2..6f4802d 100644
> --- a/src/include/catalog/dependency.h
> +++ b/src/include/catalog/dependency.h
> @@ -112,11 +112,10 @@ typedef struct ObjectAddresses ObjectAddresses;
>
> /*
> * This enum covers all system catalogs whose OIDs can appear in
> - * pg_depend.classId or pg_shdepend.classId.
> + * pg_depend.classId or pg_shdepend.classId. See also object_classes[].
> */
> typedef enum ObjectClass
> {
> - OCLASS_CLASS, /* pg_class */
> OCLASS_PROC,/* pg_proc */
> OCLASS_TYPE,/* pg_type */
> OCLASS_CAST,/* pg_cast */
> @@ -149,6 +148,11 @@ typedef enum ObjectClass
> OCLASS_EVENT_TRIGGER, /* pg_event_trigger */
> OCLASS_POLICY, /* pg_policy */
> OCLASS_TRANSFORM, /* pg_transform */
> + /*
> + * Keep this previous-to-last, see
> + * https://www.postgresql.org/message-id/
> + */
> + OCLASS_CLASS, /* pg_class */
> MAX_OCLASS /* MUST BE LAST */
> } ObjectClass;
>
--
Álvaro Herrerahttp://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] object_classes array is broken, again
Robert Haas wrote:
> The transforms patch seems to have forgotten to add
> TransformRelationId to object_classes[], much like the RLS patch
> forgot to add PolicyRelationId in the same place.
>
> Fixing this is easy, but ISTM that we need to insert some sort of a
> guard to prevent people from continuing to forget this, because it's
> apparently quite easy to do. Perhaps add_object_address should
> Assert(OidIsValid(object_classes[oclass])),
The problem is that there aren't enough callers of add_object_address:
there are many indexes of that array that aren't ever accessed and so
it's not obvious when the array is broken. If we were to put
OCLASS_CLASS at the end instead of at the beginning, that would fix the
problem by making it immediately obvious when things get broken this
way, because the value used in the most common case would shift around
every time we add another value. (Of course, we'd have to instruct
people to not add new members after the pg_class entry.)
I just tried this, and it's a bit nasty: while it does causes the tests
to fail, it's not at all obvious that there's a connection between the
failure and object_classes[]. I think we can solve that by adding a
comment to ObjectClass. Here's the first hunk in the large regression
failure:
*** /pgsql/source/master/src/test/regress/expected/triggers.out 2015-05-22
20:09:28.936186873 +0200
---
/home/alvherre/Code/pgsql/build/master/src/test/regress/results/triggers.out
2015-07-18 17:26:13.
664764070 +0200
***
*** 1429,1437
(4 rows)
DROP TABLE city_table CASCADE;
- NOTICE: drop cascades to 2 other objects
- DETAIL: drop cascades to view city_view
- drop cascades to view european_city_view
DROP TABLE country_table;
-- Test pg_trigger_depth()
create table depth_a (id int not null primary key);
--- 1429,1434
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index c1212e9..0107c53 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -127,7 +127,6 @@ typedef struct
* See also getObjectClass().
*/
static const Oid object_classes[MAX_OCLASS] = {
- RelationRelationId, /* OCLASS_CLASS */
ProcedureRelationId, /* OCLASS_PROC */
TypeRelationId,/* OCLASS_TYPE */
CastRelationId,/* OCLASS_CAST */
@@ -158,7 +157,9 @@ static const Oid object_classes[MAX_OCLASS] = {
DefaultAclRelationId, /* OCLASS_DEFACL */
ExtensionRelationId, /* OCLASS_EXTENSION */
EventTriggerRelationId, /* OCLASS_EVENT_TRIGGER */
- PolicyRelationId /* OCLASS_POLICY */
+ PolicyRelationId, /* OCLASS_POLICY */
+ TransformRelationId, /* OCLASS_POLICY */
+ RelationRelationId /* OCLASS_CLASS */
};
diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.h
index 5da18c2..6f4802d 100644
--- a/src/include/catalog/dependency.h
+++ b/src/include/catalog/dependency.h
@@ -112,11 +112,10 @@ typedef struct ObjectAddresses ObjectAddresses;
/*
* This enum covers all system catalogs whose OIDs can appear in
- * pg_depend.classId or pg_shdepend.classId.
+ * pg_depend.classId or pg_shdepend.classId. See also object_classes[].
*/
typedef enum ObjectClass
{
- OCLASS_CLASS,/* pg_class */
OCLASS_PROC,/* pg_proc */
OCLASS_TYPE,/* pg_type */
OCLASS_CAST,/* pg_cast */
@@ -149,6 +148,11 @@ typedef enum ObjectClass
OCLASS_EVENT_TRIGGER, /* pg_event_trigger */
OCLASS_POLICY,/* pg_policy */
OCLASS_TRANSFORM, /* pg_transform */
+ /*
+ * Keep this previous-to-last, see
+ * https://www.postgresql.org/message-id/
+ */
+ OCLASS_CLASS,/* pg_class */
MAX_OCLASS /* MUST BE LAST */
} ObjectClass;
--
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] object_classes array is broken, again
On 6/24/15 2:11 PM, Robert Haas wrote:
Fixing this is easy, but ISTM that we need to insert some sort of a
guard to prevent people from continuing to forget this, because it's
apparently quite easy to do. Perhaps add_object_address should
Assert(OidIsValid(object_classes[oclass])), plus a (static?) assert
someplace checking that OidIsValid(object_classes[MAX_OCLASS - 1])?
I tried doing this and I'm getting a "static_assert expression is not an
integral constant expression" error, even when I reduce it to a simple
constant comparison. Maybe I'm just doing something dumb...
If I replace the StaticAssert with
Assert(OidIsValid(object_classes[MAX_OCLASS - 1])) it works find and
initdb will fail if that assert trips.
I've attached the broken StaticAssert version. Also added a warning
comment to the enum.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Data in Trouble? Get it in Treble! http://BlueTreble.com
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index c1212e9..7adc216 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -2037,6 +2037,15 @@ add_object_address(ObjectClass oclass, Oid objectId,
int32 subId,
{
ObjectAddress *item;
+ /*
+* Ensure object_classes is properly sized. Can't use OidIsValid here
+* because it's not marked as static (nor should it be).
+*/
+ StaticAssertStmt(object_classes[MAX_OCLASS - 1] != 0,
+ "last OID in object_classes[] is invalid (did you
forget to add a new entry to it?)");
+
+ Assert(OidIsValid(object_classes[oclass]));
+
/* enlarge array if needed */
if (addrs->numrefs >= addrs->maxrefs)
{
diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.h
index 5da18c2..515417d 100644
--- a/src/include/catalog/dependency.h
+++ b/src/include/catalog/dependency.h
@@ -149,6 +149,8 @@ typedef enum ObjectClass
OCLASS_EVENT_TRIGGER, /* pg_event_trigger */
OCLASS_POLICY, /* pg_policy */
OCLASS_TRANSFORM, /* pg_transform */
+
+ /* New classes need to be added to object_classes[] too. */
MAX_OCLASS /* MUST BE LAST */
} ObjectClass;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
