Re: [BUGS] [HACKERS] object_classes array is broken, again
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
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 : > 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
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
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: [BUGS] [HACKERS] object_classes array is broken, again
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
Alvaro Herrera 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
Tom Lane wrote: > Alvaro Herrera 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: [BUGS] [HACKERS] object_classes array is broken, again
Alvaro Herrera 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