Re: replicating DROP commands across servers

Stephen Frost wrote:
> Alvaro,
> 
> * Alvaro Herrera (alvherre@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