Stephen Frost wrote:
> Alvaro,
>
> * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> > + /*
> > + * Make sure that both objname and objargs were passed, or none was.
> > + * Initialize objargs to empty list, which is the most common case.
> > + */
> > + Assert(PointerIsValid(objname) == PointerIsValid(objargs));
> > + if (objargs)
> > + *objargs = NIL;
> > +
>
> I feel like I must be missing something, but you only explicitly reset
> objargs, not objnames, and then below you sometimes add to objname and
> other times throw away anything which might be there:
>
> > --- 2948,2974 ----
> > attr =
> > get_relid_attribute_name(object->objectId,
> >
> > object->objectSubId);
> > appendStringInfo(&buffer, ".%s",
> > quote_identifier(attr));
> > + if (objname)
> > + *objname = lappend(*objname, attr);
> > }
> > break;
>
> Here's an "add to objname" case, and then:
Right. In the "add to objname" cases, there is already some other
routine that initialized it previously by filling in some stuff; in the
case above, this happens in the getRelationIdentity() immediately
preceding this.
In the other cases we initialize on that spot.
> > --- 3037,3045 ----
> > {
> > appendStringInfo(&buffer, "%s on ",
> >
> > quote_identifier(NameStr(con->conname)));
> > ! getRelationIdentity(&buffer,
> > con->conrelid, objname);
> > ! if (objname)
> > ! *objname = lappend(*objname,
> > pstrdup(NameStr(con->conname)));
> > }
> > else
> > {
>
> And another "add to existing" case.
Note how this one has a getRelationIdentity, just like the first one.
> Guess I have a bit of a hard time with an API that's "we might add to
> this list, or we might replace whatever is there". I think it would be
> best to just initialize both (or assert that they are) and have any
> callers who need to merge the list(s) coming back into an existing list
> handle that themselves.
The thing is, the list is already initialized in all cases to a valid
list in this routine; there is no case that appends to whatever junk
might have been there before this routine started.
> I'm also not a huge fan of the big object_type_map, but I also don't
> have a better solution.
Agreed. We have the ObjectProperty array also in the same file; it
kinda looks like there is duplication here, but actually there isn't.
This whole issue is just fallout from the fact that we have three
different ways to identify object classes: the ObjectClass enum, the
ObjectType enum, and the relation OIDs of each catalog (actually a
fourth one, see below). I don't see any other nice way around this; I
guess we could try to auto-generate these tables somehow from a master
text file, or something. Not material for this patch, I think.
Note my DDL deparse patch adds a comment:
+/* XXX merge this with ObjectTypeMap? */
static event_trigger_support_data event_trigger_support[] = {
and a late revision to that patch added a new function in
event_triggers.c (not yet posted I think) due to GRANT having its own
enum of object types, AclObjectKind.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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
