Re: [BUGS] [HACKERS] object_classes array is broken, again

2016-11-29 Thread Alvaro Herrera
Alvaro Herrera wrote:
> Jaimin Pan wrote:
> > Hi all,
> > 
> > How about this patch. I believe it will never missing someone and be
> > detected while compiling.
> 
> Hmm, yeah this looks like something that's worth considering going
> forward.  I can't think of any reason not to do this.  Perhaps we can
> write getObjectClass using this, too.

I just noticed a lot of the DropFooById() functions consist of just
"open catalog, lookup tuple by OID, delete tuple, close catalog".  Which
I think we could rewrite in a generic manner using the table proposed by
this patch, and save some boilerplate code.  Now there's a portion of
the functions that have some code in addition to that, but we can still
call the generic one and then do the rest of the stuff in the
class-specific function.  Looks like a pretty easy code removal project.

-- 
Á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: [BUGS] [HACKERS] object_classes array is broken, again

2015-07-23 Thread Jaimin Pan
Hi all,

How about this patch. I believe it will never missing someone and be
detected while compiling.

2015-07-21 19:38 GMT+08:00 Alvaro Herrera alvhe...@2ndquadrant.com:

 Tom Lane wrote:
  I wrote:
   +1 to this patch, in fact I think we could remove MAX_OCLASS altogether
   which would be very nice for switch purposes.
 
  Oh, wait, I forgot that the patch itself introduces another reference to
  MAX_OCLASS.  I wonder though if we should get rid of that as an enum
 value
  in favor of #define'ing a LAST_OCLASS macro referencing the last enum
  item, or some other trick like that.  It's certainly inconvenient in
  event_trigger.c to have a phony member of the enum.

 Yeah, that works well enough.  Pushed that way.

  Are there any other arrays that need such tests?

 I looked around with this:

 git grep 'const.*\[.*[^][0-9].*\].*=.*{'

 and found one suspicious-looking use of MAX_ACL_KIND which we could
 perhaps define in a way equivalent to what we've done here.  We also
 have RM_MAX_ID in a couple of arrays but that looks safe because both
 the values and the arrays are auto-generated.

 We also have MAX_TIME_PRECISION, MAX_TIMESTAMP_PRECISION,
 MAX_INTERVAL_PRECISION, MAXDATEFIELDS, KeyWord_INDEX_SIZE, but these
 don't look likely to actually cause any trouble.

 (There are many arrays sized to numerical constants, but there are about
 5000 of those and I'm not in a hurry to verify how they are used.)

 --
 Álvaro Herrerahttp://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Remote DBA, Training  Services



fix.patch
Description: Binary data

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


Re: [BUGS] [HACKERS] object_classes array is broken, again

2015-07-21 Thread Alvaro Herrera
Jaimin Pan wrote:
 Hi all,
 
 How about this patch. I believe it will never missing someone and be
 detected while compiling.

Hmm, yeah this looks like something that's worth considering going
forward.  I can't think of any reason not to do this.  Perhaps we can
write getObjectClass using this, too.

The new file has a wrong comment above the list, copy-pasted from
rmgrlist.h.

I've always wondered about unifying OCLASS with OBJECT, but that's
probably a completely independent topic.

-- 
Á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: [BUGS] [HACKERS] object_classes array is broken, again

2015-07-21 Thread Alvaro Herrera
Tom Lane wrote:
 I wrote:
  +1 to this patch, in fact I think we could remove MAX_OCLASS altogether
  which would be very nice for switch purposes.
 
 Oh, wait, I forgot that the patch itself introduces another reference to
 MAX_OCLASS.  I wonder though if we should get rid of that as an enum value
 in favor of #define'ing a LAST_OCLASS macro referencing the last enum
 item, or some other trick like that.  It's certainly inconvenient in
 event_trigger.c to have a phony member of the enum.

Yeah, that works well enough.  Pushed that way.

 Are there any other arrays that need such tests?

I looked around with this:

git grep 'const.*\[.*[^][0-9].*\].*=.*{'

and found one suspicious-looking use of MAX_ACL_KIND which we could
perhaps define in a way equivalent to what we've done here.  We also
have RM_MAX_ID in a couple of arrays but that looks safe because both
the values and the arrays are auto-generated.

We also have MAX_TIME_PRECISION, MAX_TIMESTAMP_PRECISION,
MAX_INTERVAL_PRECISION, MAXDATEFIELDS, KeyWord_INDEX_SIZE, but these
don't look likely to actually cause any trouble.

(There are many arrays sized to numerical constants, but there are about
5000 of those and I'm not in a hurry to verify how they are used.)

-- 
Á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

2015-07-20 Thread Alvaro Herrera
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: [BUGS] [HACKERS] object_classes array is broken, again

2015-07-20 Thread Tom Lane
Alvaro Herrera alvhe...@2ndquadrant.com writes:
 Any opinions on this idea?  I don't like it all that much, but it's
 plenty effective.

I don't like it much either.

What about adding StaticAsserts that lengthof() the relevant constant
arrays is equal to MAX_OCLASS?  (Or other similar ways of checking
that they have the right number of entries.)

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: [BUGS] [HACKERS] object_classes array is broken, again

2015-07-20 Thread Tom Lane
I wrote:
 +1 to this patch, in fact I think we could remove MAX_OCLASS altogether
 which would be very nice for switch purposes.

Oh, wait, I forgot that the patch itself introduces another reference to
MAX_OCLASS.  I wonder though if we should get rid of that as an enum value
in favor of #define'ing a LAST_OCLASS macro referencing the last enum
item, or some other trick like that.  It's certainly inconvenient in
event_trigger.c to have a phony member of the enum.

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: [BUGS] [HACKERS] object_classes array is broken, again

2015-07-20 Thread Tom Lane
Alvaro Herrera alvhe...@2ndquadrant.com writes:
 Tom Lane wrote:
 What about adding StaticAsserts that lengthof() the relevant constant
 arrays is equal to MAX_OCLASS?  (Or other similar ways of checking
 that they have the right number of entries.)

 Well, the array itself is declared like this:
   static const Oid object_classes[MAX_OCLASS] = {
 so testing lengthof() of it is useless because it's a constant and the
 assertion always holds.  If it were declared like this instead:
   static const Oid object_classes[] = {
 then we could use lengthof().

Ah.  I think the point of using MAX_OCLASS there was to get a warning
if the array was too short, but evidently it doesn't work like that.

 I don't see any drawwbacks to that.

+1 to this patch, in fact I think we could remove MAX_OCLASS altogether
which would be very nice for switch purposes.

Are there any other arrays that need such tests?

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: [BUGS] [HACKERS] object_classes array is broken, again

2015-07-20 Thread Alvaro Herrera
Tom Lane wrote:
 Alvaro Herrera alvhe...@2ndquadrant.com writes:
  Any opinions on this idea?  I don't like it all that much, but it's
  plenty effective.
 
 I don't like it much either.
 
 What about adding StaticAsserts that lengthof() the relevant constant
 arrays is equal to MAX_OCLASS?  (Or other similar ways of checking
 that they have the right number of entries.)

Well, the array itself is declared like this:
static const Oid object_classes[MAX_OCLASS] = {
so testing lengthof() of it is useless because it's a constant and the
assertion always holds.  If it were declared like this instead:
static const Oid object_classes[] = {
then we could use lengthof().

I don't see any drawwbacks to that.

-- 
Á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..de2e2f2 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -126,7 +126,7 @@ typedef struct
  * This constant table maps ObjectClasses to the corresponding catalog OIDs.
  * See also getObjectClass().
  */
-static const Oid object_classes[MAX_OCLASS] = {
+static const Oid object_classes[] = {
 	RelationRelationId,			/* OCLASS_CLASS */
 	ProcedureRelationId,		/* OCLASS_PROC */
 	TypeRelationId,/* OCLASS_TYPE */
@@ -158,7 +158,8 @@ 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_TRANSFORM */
 };
 
 
@@ -2037,6 +2038,12 @@ add_object_address(ObjectClass oclass, Oid objectId, int32 subId,
 {
 	ObjectAddress *item;
 
+	/*
+	 * Make sure object_classes is kept up to date with the ObjectClass enum.
+	 */
+	StaticAssertStmt((lengthof(object_classes) == MAX_OCLASS),
+	 object_classes[] must cover all ObjectClasses);
+
 	/* 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..138788e 100644
--- a/src/include/catalog/dependency.h
+++ b/src/include/catalog/dependency.h
@@ -112,7 +112,7 @@ 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 object_classes[].
  */
 typedef enum 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

2015-07-18 Thread Alvaro Herrera
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

2015-06-26 Thread Jim Nasby

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


[HACKERS] object_classes array is broken, again

2015-06-24 Thread Robert Haas
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])), plus a (static?) assert
someplace checking that OidIsValid(object_classes[MAX_OCLASS - 1])?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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