Re: replicating DROP commands across servers

As you probably know, I've studying how to implement a system to allow
replicating DDL commands to remote servers. I have a fairly good grasp
of handling CREATE commands; we have another thread discussing that
involving a JSON output for such commands. That hasn't been carried to
completion yet; I will finish that up for 9.5. My intention is to have
that code handle ALTER commands as well: I have tried some experiments
and it turns out that it's a bit more complex than I would like, but it
seems doable, now that I've figured out exactly how. (Of course, the
ALTER bits can also be used for auditing and business rule enforcing,
which are the other use-cases for this feature.)

In this thread I focus on DROP. We already added support for dropped
objects in 9.3 in the form of the pg_event_trigger_dropped_objects()
function, which returns the set of objects dropped by any command.
That's quite handy for the auditing purpose it was supposed to serve,
but it's insufficient for replication because it's pretty hard to craft
a command for object deletion on the remote node.

So my current plan involves receiving the set of objects in the remote
node, then adding them to an ObjectAddresses array, then call
performMultipleDeletions() on that. As it turns out, this works quite
well -- except when it doesn't.

The problem is this: at the local node we have the object type and
identity. We can transmit this quite fine from this node to the other
node, and then in the other node parse this back into a objname list and
pass that to get_object_address(); but only for objects that have a
simple representation, such as schemas and relations. For procedures,
constraints, triggers and other types, it's a lot of work to parse the
string into the objname/objargs lists (the identities for such objects
as "constraint_name on schema.tablename" or "schema.funcname(arg1, arg2)"
and so on. Of course, it is *possible* to parse this, but it seems the
wrong thing to do when we could just obtain the right input in the first
place.

My proposal therefore is to add some more columns to
pg_event_trigger_dropped_objects(): more precisely, objname and objargs,
which would carry exactly what get_object_address() would require to
re-construct an ObjectAddress for the object being dropped at the remote
end.

Thoughts, objections?

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> My proposal therefore is to add some more columns to
> pg_event_trigger_dropped_objects(): more precisely, objname and objargs,
> which would carry exactly what get_object_address() would require to
> re-construct an ObjectAddress for the object being dropped at the remote
> end.

Those aren't strings or indeed flat objects at all, but structures, so it
seems like this is still rather underspecified. How will you represent
something like a List of TypeName at the SQL level?

regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> > My proposal therefore is to add some more columns to
> > pg_event_trigger_dropped_objects(): more precisely, objname and objargs,
> > which would carry exactly what get_object_address() would require to
> > re-construct an ObjectAddress for the object being dropped at the remote
> > end.
>
> Those aren't strings or indeed flat objects at all, but structures, so it
> seems like this is still rather underspecified. How will you represent
> something like a List of TypeName at the SQL level?

Yeah, that's an ugly case. I'm thinking that I could print those like
regtype output would, and then read them back in using (something
similar to) parseTypeString(). A bit convoluted perhaps, but I think it
should work. For things such as function and cast identities, typmod
shouldn't matter AFAIK, so that loss is not significant.

Another thing this will need is a table such as

static const struct
{
const char *tm_name;
ObjectType tm_type;
}
ObjectTypeMap[] =
{
/* relation types */
{ "table", OBJECT_TABLE },
{ "index", OBJECT_INDEX },
{ "sequence", OBJECT_SEQUENCE },
...

so that we can translate object types back into the ObjectType enum.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Here's a patch implementing the proposed idea. This is used in the
Bidirectional Replication stuff by Simon/Andres; it works well.

One thing of note is that I added output flags for "normal" and
"original", which mostly come from performDeletion flags. This let one
select only such objects when trying to replicate a drop; otherwise,
we'd add RI triggers to the set to drop remotely, which doesn't work
because their names have OIDs embedded, and in the remote system those
are different.

One curious thing is that I had to add a hack that if an object has a
"reverse" flag in the ObjectAddresses array, also set the "normal"
output flag. (Another possibility would have been to add a "reverse"
output flag, but there doesn't seem to be a use for that --- it seems to
expose internals unnecessarily.)

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Hi.

I thought I'd review this patch, since pgaudit uses the
pg_event_trigger_dropped_objects function.

I went through the patch line by line, and I don't really have anything
to say about it. I notice that there are some XXX/FIXME comments in the
code, but it's not clear if those need to (or can be) fixed before the
code is committed.

Everything else looks good. I think this is ready for committer.

-- Abhijit

On 2014-06-13 15:50:50 -0400, Alvaro Herrera wrote:
> Here's a patch implementing the proposed idea. This is used in the
> Bidirectional Replication stuff by Simon/Andres; it works well.

I think there's been some changes to this patch since july, care to
resend a new version?

I think it's appropriate to mark the patch as "Waiting for Author"
instead of "Ready for Committer" till then.

Greetings,

Andres Freund

Andres Freund wrote:
> On 2014-06-13 15:50:50 -0400, Alvaro Herrera wrote:
> > Here's a patch implementing the proposed idea. This is used in the
> > Bidirectional Replication stuff by Simon/Andres; it works well.
>
> I think there's been some changes to this patch since july, care to
> resend a new version?

Sure, here it is.

The only difference with the previous version is that it now also
supports column defaults. This was found to be a problem when you drop
a sequence that some column default depends on -- for example a column
declared SERIAL, or a sequence marked with ALTER SEQUENCE OWNED BY. The
new code is able to drop both the sequence and the default value
(leaving, of course, the rest of the column intact.) This required
adding support for such objects in get_object_address.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

>
> > I think there's been some changes to this patch since july, care to
> > resend a new version?
>
> Sure, here it is.
>
> The only difference with the previous version is that it now also
> supports column defaults. This was found to be a problem when you drop
> a sequence that some column default depends on -- for example a column
> declared SERIAL, or a sequence marked with ALTER SEQUENCE OWNED BY. The
> new code is able to drop both the sequence and the default value
> (leaving, of course, the rest of the column intact.) This required
> adding support for such objects in get_object_address.

I have given this patch the following review:

- Apply to current master (77e65bf). -- success
- check-world. --success
- multiple FIXME statements still exist -- are there plans to fix these
items? Can the duplicated code be extracted to a static function?

-Adam

--
Adam Brightwell - adam(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com

On 09/16/2014 09:09 PM, Brightwell, Adam wrote:
>>
>>> I think there's been some changes to this patch since july, care to
>>> resend a new version?
>>
>> Sure, here it is.
>>
>> The only difference with the previous version is that it now also
>> supports column defaults. This was found to be a problem when you drop
>> a sequence that some column default depends on -- for example a column
>> declared SERIAL, or a sequence marked with ALTER SEQUENCE OWNED BY. The
>> new code is able to drop both the sequence and the default value
>> (leaving, of course, the rest of the column intact.) This required
>> adding support for such objects in get_object_address.
>
>
> I have given this patch the following review:
>
> - Apply to current master (77e65bf). -- success
> - check-world. --success
> - multiple FIXME statements still exist -- are there plans to fix these
> items? Can the duplicated code be extracted to a static function?

Nothing seems to be happening to this, so I'm marking this as returned
with feedback.

- Heikki

Heikki Linnakangas wrote:
> On 09/16/2014 09:09 PM, Brightwell, Adam wrote:

> >I have given this patch the following review:
> >
> >- Apply to current master (77e65bf). -- success
> >- check-world. --success
> >- multiple FIXME statements still exist -- are there plans to fix these
> >items? Can the duplicated code be extracted to a static function?
>
> Nothing seems to be happening to this, so I'm marking this as
> returned with feedback.

Meh.

There are three fixmes in the code. One can be handled by just removing
the line; we don't really care about duplicating 10 lines of boilerplate
code. The other two mean missing support for domain constraints and for
default ACLs. Is there absolutely no feedback to be had on the
mechanism used by the patch?

Since the patch has had good feedback and no further comments arise, I
can just implement support for those two missing object types and push,
and everybody will be happy. Right?

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro,

* Alvaro Herrera (alvherre(at)2ndquadrant(dot)com) wrote:
> There are three fixmes in the code. One can be handled by just removing
> the line; we don't really care about duplicating 10 lines of boilerplate
> code. The other two mean missing support for domain constraints and for
> default ACLs. Is there absolutely no feedback to be had on the
> mechanism used by the patch?
>
> Since the patch has had good feedback and no further comments arise, I
> can just implement support for those two missing object types and push,
> and everybody will be happy. Right?

In general, I'd say yes, but I'll take a look at the patch now and
provide feedback in a couple hours anyway.

Thanks,

Stephen

On 10/03/2014 08:08 PM, Stephen Frost wrote:
> Alvaro,
>
> * Alvaro Herrera (alvherre(at)2ndquadrant(dot)com) wrote:
>> There are three fixmes in the code. One can be handled by just removing
>> the line; we don't really care about duplicating 10 lines of boilerplate
>> code. The other two mean missing support for domain constraints and for
>> default ACLs. Is there absolutely no feedback to be had on the
>> mechanism used by the patch?
>>
>> Since the patch has had good feedback and no further comments arise, I
>> can just implement support for those two missing object types and push,
>> and everybody will be happy. Right?
>
> In general, I'd say yes, but I'll take a look at the patch now and
> provide feedback in a couple hours anyway.

Thanks Stephen!

I had a very brief look at the docs, and these extra outputs from
pg_event_trigger_dropped_objects caught my eye:

> + <row>
> + <entry><literal>address_names</literal></entry>
> + <entry><type>text[]</type></entry>
> + <entry>
> + An array that, together with <literal>address_args</literal>,
> + can be used by the C-language function getObjectAddress() to
> + recreate the object address in a remote server containing a similar object.
> + </entry>
> + </row>
> + <row>
> + <entry><literal>address_args</literal></entry>
> + <entry><type>text[]</type></entry>
> + <entry>
> + See <literal>address_names</literal> above.
> + </entry>
> + </row>

I couldn't find a function called getObjectAddress anywhere. Typo?

Also, is providing a C-language function the best we can do? The rest of
the information returned by pg_event_trigger_dropped_objects is usable
from any language.

- Heikki

Heikki Linnakangas wrote:

> I had a very brief look at the docs, and these extra outputs from
> pg_event_trigger_dropped_objects caught my eye:
>
> >+ <row>
> >+ <entry><literal>address_names</literal></entry>
> >+ <entry><type>text[]</type></entry>
> >+ <entry>
> >+ An array that, together with <literal>address_args</literal>,
> >+ can be used by the C-language function getObjectAddress() to
> >+ recreate the object address in a remote server containing a similar object.
> >+ </entry>
> >+ </row>
> >+ <row>
> >+ <entry><literal>address_args</literal></entry>
> >+ <entry><type>text[]</type></entry>
> >+ <entry>
> >+ See <literal>address_names</literal> above.
> >+ </entry>
> >+ </row>
>
> I couldn't find a function called getObjectAddress anywhere. Typo?

Ah, yeah, it's get_object_address actually.

> Also, is providing a C-language function the best we can do? The
> rest of the information returned by pg_event_trigger_dropped_objects
> is usable from any language.

Well, the return value from get_object_address is an ObjectAddress.
It's simple enough to create an SQL wrapper that takes the
address_names/address_args arrays and return an ObjectAddress; would
this be useful?

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 10/03/2014 09:06 PM, Alvaro Herrera wrote:
> Heikki Linnakangas wrote:
>
>> I had a very brief look at the docs, and these extra outputs from
>> pg_event_trigger_dropped_objects caught my eye:
>>
>>> + <row>
>>> + <entry><literal>address_names</literal></entry>
>>> + <entry><type>text[]</type></entry>
>>> + <entry>
>>> + An array that, together with <literal>address_args</literal>,
>>> + can be used by the C-language function getObjectAddress() to
>>> + recreate the object address in a remote server containing a similar object.
>>> + </entry>
>>> + </row>
>>> + <row>
>>> + <entry><literal>address_args</literal></entry>
>>> + <entry><type>text[]</type></entry>
>>> + <entry>
>>> + See <literal>address_names</literal> above.
>>> + </entry>
>>> + </row>
>>
>> I couldn't find a function called getObjectAddress anywhere. Typo?
>
> Ah, yeah, it's get_object_address actually.
>
>> Also, is providing a C-language function the best we can do? The
>> rest of the information returned by pg_event_trigger_dropped_objects
>> is usable from any language.
>
> Well, the return value from get_object_address is an ObjectAddress.
> It's simple enough to create an SQL wrapper that takes the
> address_names/address_args arrays and return an ObjectAddress; would
> this be useful?

An ObjectAddress consists of a classid, objid, and objsubid.
pg_event_trigger_dropped_objects already returns all of those as
separate fields. What am I missing?

- Heikki

Heikki Linnakangas wrote:
> On 10/03/2014 09:06 PM, Alvaro Herrera wrote:

> >Well, the return value from get_object_address is an ObjectAddress.
> >It's simple enough to create an SQL wrapper that takes the
> >address_names/address_args arrays and return an ObjectAddress; would
> >this be useful?
>
> An ObjectAddress consists of a classid, objid, and objsubid.
> pg_event_trigger_dropped_objects already returns all of those as
> separate fields. What am I missing?

Precisely the point is not returning those values, because they are
useless to identify the equivalent object in a remote database. What we
need is the object names and other stuff used to uniquely identify it
"by user-visible name". We transmit those name arrays to a remote
server, then on the remote server we can run get_object_address and get
the ObjectAddress, which has the classid,objid,objsubid values you cite ...
but for the remote server. The object can then be dropped there.

Initially we thought that we would use the object_identity object for
this (which is why we invented that functionality and added the column
in 9.3), but this turned out not to work very well for unusual object
types; hence this patch.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera wrote:

> Precisely the point is not returning those values, because they are
> useless to identify the equivalent object in a remote database. What we
> need is the object names and other stuff used to uniquely identify it
> "by user-visible name". We transmit those name arrays to a remote
> server, then on the remote server we can run get_object_address and get
> the ObjectAddress, which has the classid,objid,objsubid values you cite ...
> but for the remote server. The object can then be dropped there.
>
> Initially we thought that we would use the object_identity object for
> this (which is why we invented that functionality and added the column
> in 9.3), but this turned out not to work very well for unusual object
> types; hence this patch.

The other thing to keep in mind is that with all those ObjectAddress
thingies you got, you cannot simply construct a DROP <obj> command:

1. The objects in the list might be of different types; say a table and
a view that are dropped by the same command because of CASCADE. (You
could just pass the CASCADE to the other side and hope that it happens
to do the same thing; but if the schemas are slightly different, it
might not.)

2. DROP OWNED or other commands might have dropped several objects,
again of varying types.

So what we do in BDR is stuff all those ObjectAddress in an array of
them, and then call performMultipleDeletions. There is no way to get
this functionality in non-C code; so it's hard to see that it's very
useful to have a non-C way to use the original objnames/objargs arrays.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro,

* Alvaro Herrera (alvherre(at)2ndquadrant(dot)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:

> case OCLASS_PROC:
> appendStringInfoString(&buffer,
> format_procedure_qualified(object->objectId));
> + if (objname)
> + format_procedure_parts(object->objectId, objname, objargs);
> break;
>
> case OCLASS_TYPE:
> ! {
> ! char *typeout;
> !
> ! typeout = format_type_be_qualified(object->objectId);
> ! appendStringInfoString(&buffer, typeout);
> ! if (objname)
> ! *objname = list_make1(typeout);
> ! }
> break;

here's a "throw away whatever was in objname" case.

> ***************
> *** 2745,2750 **** getObjectIdentity(const ObjectAddress *object)
> --- 2991,3000 ----
> format_type_be_qualified(castForm->castsource),
> format_type_be_qualified(castForm->casttarget));
>
> + if (objname)
> + *objname = list_make2(format_type_be_qualified(castForm->castsource),
> + format_type_be_qualified(castForm->casttarget));
> +
> heap_close(castRel, AccessShareLock);
> break;
> }

And another "throw away" case.

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

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.

I'm also not a huge fan of the big object_type_map, but I also don't
have a better solution.

Thanks,

Stephen

Stephen Frost wrote:
> Alvaro,
>
> * Alvaro Herrera (alvherre(at)2ndquadrant(dot)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

* Alvaro Herrera (alvherre(at)2ndquadrant(dot)com) wrote:
> 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.

ahh, ok, that makes a bit more sense, sorry for missing it. Still makes
me wonder why objargs gets special treatment at the top of the function
and objnames doesn't- seems like both should be initialized either
before being passed in (and perhaps an Assert to verify that they are),
or they should both be initialized, but I tend to prefer just Assert'ing
that they are correct on entry- either both are valid pointers to empty
lists, or both NULL.

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

Yeah, I did notice that, and noticed that it's not duplication.

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

Agreed that this patch doesn't need to address it and not sure that a
master text file would actually be an improvement.. I had been thinking
if there was some way to have a single mapping which could be used in
either direction, but I didn't see any sensible way to make that work
given that it's not *quite* the same backwards and forewards.

> Note my DDL deparse patch adds a comment:
>
> +/* XXX merge this with ObjectTypeMap? */
> static event_trigger_support_data event_trigger_support[] = {

Yes, I saw that, and that you added a comment that the new map needs to
be updated when changes are made, which is also good.

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

Yeah. Perhaps one day we'll unify all of these, though I'm not 100%
sure it'd be possible...

Thanks!

Stephen

Stephen Frost wrote:
> * Alvaro Herrera (alvherre(at)2ndquadrant(dot)com) wrote:
> > 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.
>
> ahh, ok, that makes a bit more sense, sorry for missing it. Still makes
> me wonder why objargs gets special treatment at the top of the function
> and objnames doesn't- seems like both should be initialized either
> before being passed in (and perhaps an Assert to verify that they are),
> or they should both be initialized, but I tend to prefer just Assert'ing
> that they are correct on entry- either both are valid pointers to empty
> lists, or both NULL.

I guess I could initialize objnames to NIL also. I initialize objargs
because that one is unused for a lot of object types (so I would have to
set it to NIL in cases where it's not used), whereas objnames is always
used and thus we know it's always initialized later.

Maybe what I need here is just a longer comment explaining this ...

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

* Alvaro Herrera (alvherre(at)2ndquadrant(dot)com) wrote:
> Stephen Frost wrote:
> > ahh, ok, that makes a bit more sense, sorry for missing it. Still makes
> > me wonder why objargs gets special treatment at the top of the function
> > and objnames doesn't- seems like both should be initialized either
> > before being passed in (and perhaps an Assert to verify that they are),
> > or they should both be initialized, but I tend to prefer just Assert'ing
> > that they are correct on entry- either both are valid pointers to empty
> > lists, or both NULL.
>
> I guess I could initialize objnames to NIL also. I initialize objargs
> because that one is unused for a lot of object types (so I would have to
> set it to NIL in cases where it's not used), whereas objnames is always
> used and thus we know it's always initialized later.
>
> Maybe what I need here is just a longer comment explaining this ...

A one-line comment that it's always reset below would be sufficient for me.

Thanks for explaining it :),

Stephen

On Fri, Oct 3, 2014 at 2:33 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> Heikki Linnakangas wrote:
>> On 10/03/2014 09:06 PM, Alvaro Herrera wrote:
>
>> >Well, the return value from get_object_address is an ObjectAddress.
>> >It's simple enough to create an SQL wrapper that takes the
>> >address_names/address_args arrays and return an ObjectAddress; would
>> >this be useful?
>>
>> An ObjectAddress consists of a classid, objid, and objsubid.
>> pg_event_trigger_dropped_objects already returns all of those as
>> separate fields. What am I missing?
>
> Precisely the point is not returning those values, because they are
> useless to identify the equivalent object in a remote database. What we
> need is the object names and other stuff used to uniquely identify it
> "by user-visible name". We transmit those name arrays to a remote
> server, then on the remote server we can run get_object_address and get
> the ObjectAddress, which has the classid,objid,objsubid values you cite ...
> but for the remote server. The object can then be dropped there.
>
> Initially we thought that we would use the object_identity object for
> this (which is why we invented that functionality and added the column
> in 9.3), but this turned out not to work very well for unusual object
> types; hence this patch.

I'm not really very convinced that it's a good idea to expose this
instead of just figuring out a way to parse the object identity.

But I expect to lose that argument.

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

Robert Haas wrote:

> I'm not really very convinced that it's a good idea to expose this
> instead of just figuring out a way to parse the object identity.

That's the first thing I tried. But it's not pretty: you have to
extract schema names by splitting at a period (and what if a schema name
contains a period?), split out on ON for certain object types, figure
out parens and argument types and names for functions and aggregates,
etc. It's just not sane to try to parse such text strings.

> But I expect to lose that argument.

Good :-)

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2014-10-03 14:02:09 -0300, Alvaro Herrera wrote:
> Since the patch has had good feedback and no further comments arise, I
> can just implement support for those two missing object types and push,
> and everybody will be happy. Right?

I'd like to see a new version before that out here... I don't think
there's fundamental issues, but it's complicated enough to warrant
that. And I don't think it has gotten enough detailed review. I hope to
be able to give a bit more of that...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Oct 3, 2014 at 4:58 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> Robert Haas wrote:
>> I'm not really very convinced that it's a good idea to expose this
>> instead of just figuring out a way to parse the object identity.
>
> That's the first thing I tried. But it's not pretty: you have to
> extract schema names by splitting at a period (and what if a schema name
> contains a period?),

Please tell me that the literals are escaped if necessary. If so,
this is pretty easy. quote_literal() is not a hard transformation to
reverse, and splitting on a unquoted period is not hard...

> split out on ON for certain object types,

...nor is splitting on any other fixed text string, such as " ON ".

> figure
> out parens and argument types and names for functions and aggregates,
> etc.

I certainly agree that parsing out parens and argument types and names
for functions and aggregates is the hardest part of this, mostly
because you can't count a comma to mark the end of one argument and
the beginning of the next - you have to account for quoted
identifiers, and you might be inside a numeric typemod or similar.

> It's just not sane to try to parse such text strings.

But this is a pretty ridiculous argument. We have an existing parser
that does it just fine, and a special-purpose parser that does just
that (and not all of the other stuff that the main parser does) would
be a great deal simpler. Maybe there are examples other than the ones
you listed here that demonstrate that this is actually a hard problem,
but the fact that you might need to undo quote_literal() or search for
and split on fixed strings does not.

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

On 10/4/14, 8:12 PM, Robert Haas wrote:
>> >It's just not sane to try to parse such text strings.
> But this is a pretty ridiculous argument. We have an existing parser
> that does it just fine, and a special-purpose parser that does just
> that (and not all of the other stuff that the main parser does) would
> be a great deal simpler. Maybe there are examples other than the ones
> you listed here that demonstrate that this is actually a hard problem,
> but the fact that you might need to undo quote_literal() or search for
> and split on fixed strings does not.

FWIW, I've run into situations more than once in userspace where I need a way to properly separate schema and object name. Generally I can make do using reg* casts and then hitting catalog tables, but it'd be nice if there was an easier way.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On Mon, Oct 6, 2014 at 7:03 PM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
> On 10/4/14, 8:12 PM, Robert Haas wrote:
>>> >It's just not sane to try to parse such text strings.
>>
>> But this is a pretty ridiculous argument. We have an existing parser
>> that does it just fine, and a special-purpose parser that does just
>> that (and not all of the other stuff that the main parser does) would
>> be a great deal simpler. Maybe there are examples other than the ones
>> you listed here that demonstrate that this is actually a hard problem,
>> but the fact that you might need to undo quote_literal() or search for
>> and split on fixed strings does not.
>
>
> FWIW, I've run into situations more than once in userspace where I need a
> way to properly separate schema and object name. Generally I can make do
> using reg* casts and then hitting catalog tables, but it'd be nice if there
> was an easier way.

Sure, although I think that's a bit of a separate problem. It's hard
to iterate through a string a character at a time from the SQL level
so that you can handle stuff like the quote_literal() rules. If we
want people to be able to do that easily we need to provide tools to
handle it. But C is actually quite well-suited to such tasks.

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

On 10/6/14, 11:24 PM, Robert Haas wrote:

Offlist.

>> FWIW, I've run into situations more than once in userspace where I need a
>> way to properly separate schema and object name. Generally I can make do
>> using reg* casts and then hitting catalog tables, but it'd be nice if there
>> was an easier way.
>
> Sure, although I think that's a bit of a separate problem. It's hard
> to iterate through a string a character at a time from the SQL level
> so that you can handle stuff like the quote_literal() rules. If we
> want people to be able to do that easily we need to provide tools to
> handle it. But C is actually quite well-suited to such tasks.

Yeah, I wouldn't want to attempt this in SQL; I was saying that a built-in function to do this would be broadly useful, not just for replicating DROPs.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

Jim Nasby wrote:
> On 10/6/14, 11:24 PM, Robert Haas wrote:
>
> Offlist.
>
> >>FWIW, I've run into situations more than once in userspace where I need a
> >>way to properly separate schema and object name. Generally I can make do
> >>using reg* casts and then hitting catalog tables, but it'd be nice if there
> >>was an easier way.
> >
> >Sure, although I think that's a bit of a separate problem. It's hard
> >to iterate through a string a character at a time from the SQL level
> >so that you can handle stuff like the quote_literal() rules. If we
> >want people to be able to do that easily we need to provide tools to
> >handle it. But C is actually quite well-suited to such tasks.
>
> Yeah, I wouldn't want to attempt this in SQL; I was saying that a
> built-in function to do this would be broadly useful, not just for
> replicating DROPs.

Well, most of what you need is served by pg_identify_object, I think:
just grab the OID from the appropriate catalog, and the OID of the
catalog itself; that function will give you schema and name, which is
what you need. Probably the most difficult part is figuring out which
reg* cast you want .. and of course there are also cases where there is
no such datatype to cast to in the first place.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Robert Haas wrote:
> On Fri, Oct 3, 2014 at 4:58 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> > Robert Haas wrote:
> >> I'm not really very convinced that it's a good idea to expose this
> >> instead of just figuring out a way to parse the object identity.
> >
> > That's the first thing I tried. But it's not pretty: you have to
> > extract schema names by splitting at a period (and what if a schema name
> > contains a period?),
>
> Please tell me that the literals are escaped if necessary. If so,
> this is pretty easy. quote_literal() is not a hard transformation to
> reverse, and splitting on a unquoted period is not hard...

I don't think it is necessary to force parsing strings for something
that can be more conveniently obtained from the get go, just because
we're too lazy to change the existing definition of the function. I'm
not saying it is impossible or extremely hard to parse the strings, but
since we can emit the right format with no extra effort, there doesn't
seem to be any point on doing it that way. It's not like this patch
adds excessive extra complexity to this code, either.

I'd say that the most complex part of this patch is the addition of the
two flag ("normal" and "original") columns, which we would need
regardless of the rest of the patch; these are used to tell whether
there are routes to the given object that have the eponymous flags set
in the dependency graph.

> > It's just not sane to try to parse such text strings.
>
> But this is a pretty ridiculous argument. We have an existing parser
> that does it just fine, and a special-purpose parser that does just
> that (and not all of the other stuff that the main parser does) would
> be a great deal simpler.

We have a main parser because we have no other option --- it's the way
stuff gets into the system in the first place. But in this case, it's
not about accepting communication from the outside world, but emitting
state that is already in the database, in a different format.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2014-10-04 21:12:24 -0400, Robert Haas wrote:
> On Fri, Oct 3, 2014 at 4:58 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> > Robert Haas wrote:
> >> I'm not really very convinced that it's a good idea to expose this
> >> instead of just figuring out a way to parse the object identity.
> >
> > That's the first thing I tried. But it's not pretty: you have to
> > extract schema names by splitting at a period (and what if a schema name
> > contains a period?),
>
> Please tell me that the literals are escaped if necessary. If so,
> this is pretty easy. quote_literal() is not a hard transformation to
> reverse, and splitting on a unquoted period is not hard...

> > split out on ON for certain object types,
>
> ...nor is splitting on any other fixed text string, such as " ON ".

I'm not following here. Maybe just because I'm misunderstanding your
position.

The patch imo consists out of the following parts:
1) Addition of dependency information to the dropped object list
2) Actual get_object_address() handling for default values - the current
behaviour looks pretty borked to me.
3) The reverse of getObjectTypeDescription()
4) getObjectIdentityParts() - a slightly more detailed version of
getObjectIdentity() that requires less string parsing
5) drop even trigger support for a few types.

Are you saying you want to add a function to do 4) via parsing inside
postgres or are you suggesting to do that in every user of this
facility?

If the former, why would it be preferrable to do string parsing if we
have access to the source data? That part of the patch looks trivial to
me?

If the latter, I don't see the advantage either - this is complicated
enough, why should different users repeat the work?

Am I misunderstanding you here?

Having reread the patch just now I basically see two things to
criticize:
a) why isn't this accessible at SQL level? That seems easy to address.
b) Arguably some of this could well be done in separate commits.

> > It's just not sane to try to parse such text strings.
>
> But this is a pretty ridiculous argument. We have an existing parser
> that does it just fine

Which happens to be the part of postgres that's copied most often. So
it's certainly not something appearing to be trivial.

>,and a special-purpose parser that does just
> that (and not all of the other stuff that the main parser does) would
> be a great deal simpler.

That parser also happens to be far from trivial (if we're talking about
parseNameAndArgTypes() - which just solves one of the cases.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund wrote:

> Having reread the patch just now I basically see two things to
> criticize:
> a) why isn't this accessible at SQL level? That seems easy to address.
> b) Arguably some of this could well be done in separate commits.

Fair comments. I will split it up.

FWIW, I spent some time today fighting with this stuff but the
OBJECT_POLICY stuff has been causing me some trouble. I'm not sure that
what's there currently in get_object_address is completely sane -- for
one thing I'm unsure that tables are the only object kind in the system
that are subject to policies. In that case, we have a shortage of
abstraction here, it seems, which will need some additional fixes.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera wrote:
> Andres Freund wrote:
>
> > Having reread the patch just now I basically see two things to
> > criticize:
> > a) why isn't this accessible at SQL level? That seems easy to address.
> > b) Arguably some of this could well be done in separate commits.
>
> Fair comments. I will split it up.

Here's a split version. The last part is still missing some polish --
in particular handling for OBJECT_POLICY, and the SQL interface which
would also allow us to get something in the regression tests.

Note: in this patch series you can find the ObjectTypeMap thing that you
thought was obsolete in the DDL deparse patch ...

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro,

On Wednesday, October 15, 2014, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:

> Alvaro Herrera wrote:
> > Andres Freund wrote:
> >
> > > Having reread the patch just now I basically see two things to
> > > criticize:
> > > a) why isn't this accessible at SQL level? That seems easy to address.
> > > b) Arguably some of this could well be done in separate commits.
> >
> > Fair comments. I will split it up.
>
> Here's a split version. The last part is still missing some polish --
> in particular handling for OBJECT_POLICY, and the SQL interface which
> would also allow us to get something in the regression tests.

The OBJECT_POLICY bit is on me to clean up and I'm planning to do so
shortly. I agree that we likely want policies for other objects also as a
couple people have brought up that idea now and will investigate it.

I'm planning to address the copy.c comments first and should have a patch
later tonight.

Thanks!

Stephen

On Thu, Oct 16, 2014 at 7:01 AM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Alvaro Herrera wrote:
>> Andres Freund wrote:
>>
>> > Having reread the patch just now I basically see two things to
>> > criticize:
>> > a) why isn't this accessible at SQL level? That seems easy to address.
>> > b) Arguably some of this could well be done in separate commits.
>>
>> Fair comments. I will split it up.
>
> Here's a split version. The last part is still missing some polish --
> in particular handling for OBJECT_POLICY, and the SQL interface which
> would also allow us to get something in the regression tests.
>
>
> Note: in this patch series you can find the ObjectTypeMap thing that you
> thought was obsolete in the DDL deparse patch ...

This patch has had no activity for the last two months, is in "Needs
Review" state and has marked as reviewer Dimitri. As there is no
activity from the reviewer, I am moving that to CF 2014-12 and
removing Dimitri as reviewer. If someone wants to have a look at this
patch, feel free to do so. Dimitri, if you are still planning to look
at it, please re-add your name.
--
Michael

Michael Paquier wrote:

> This patch has had no activity for the last two months, is in "Needs
> Review" state and has marked as reviewer Dimitri. As there is no
> activity from the reviewer, I am moving that to CF 2014-12 and
> removing Dimitri as reviewer. If someone wants to have a look at this
> patch, feel free to do so. Dimitri, if you are still planning to look
> at it, please re-add your name.

FWIW I intend to commit the first patch more or less as-is, and then add
a SQL function accessor to get_object_address to the second part and
commit that one also.

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

Alvaro Herrera wrote:
> Alvaro Herrera wrote:
> > Andres Freund wrote:
> >
> > > Having reread the patch just now I basically see two things to
> > > criticize:
> > > a) why isn't this accessible at SQL level? That seems easy to address.
> > > b) Arguably some of this could well be done in separate commits.
> >
> > Fair comments. I will split it up.
>
> Here's a split version. The last part is still missing some polish --
> in particular handling for OBJECT_POLICY, and the SQL interface which
> would also allow us to get something in the regression tests.

Pushed patch 1.

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

Here's a five-part split of the remaining pieces of this patch.

Patch 0001 is the one I posted in
http://www.postgresql.org/message-id/20141220022308.GY1768@alvh.no-ip.org
which adds support for COMMENT ON CONSTRAINT .. ON DOMAIN. This just
splits OBJECT_CONSTRAINT in OBJECT_TABCONSTRAINT and
OBJECT_DOMCONSTRAINT. It includes \dd support and pg_dump support for
comments on domain constraint comments.

I intend to commit this one first thing tomorrow.

Patch 0002 adds OBJECT_DEFAULT support. This is not needed currently,
so there's no bug being fixed; we just need it if we want to use
get_object_address in a way different from currently.

Patch 0003 adds an (unused) table and routine to map the strings
returned by getObjectTypeDescription into enum ObjectType, for use of
0004. It also splits a part of parseTypeString into a new function
typeStringToTypeName(), for use of 0004.

Patch 0004 adds a SQL-callable interface to get_object_address,
imaginatively called pg_get_object_address; this uses the stuff in patch
0003. It includes a simple regression test. The code that prepares
from text arrays into the appropriate List structure is messy because it
needs to mimic parser output.

I intend to push these three patches as a single commit tomorrow.

Patch 0005 adds getObjectIdentityParts(), which returns the object
identity in arrays that can be passed to pg_get_object_address. This
part needs slight revisions so I'm not sure I will be able to push
tomorrow.

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

I have pushed patches 0001 through 0004, with some revisions. Only the
final part is missing.

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

On 24 December 2014 at 07:41, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:

> I have pushed patches 0001 through 0004, with some revisions. Only the
> final part is missing.
>
>
Hi Alvaro,

Would you be able to commit the attached? It just fixes a new compiler
warning that I'm seeing on MSVC.

src\backend\parser\parse_type.c(795): warning C4715: 'typeStringToTypeName'
: not all control paths return a value [D:\Postgres\a\postgres.vcxproj]

Kind Regards

David Rowley

On 2014-12-24 21:54:20 +1300, David Rowley wrote:
> On 24 December 2014 at 07:41, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
> wrote:
>
> > I have pushed patches 0001 through 0004, with some revisions. Only the
> > final part is missing.
> >
> >
> Hi Alvaro,
>
> Would you be able to commit the attached? It just fixes a new compiler
> warning that I'm seeing on MSVC.
>
> src\backend\parser\parse_type.c(795): warning C4715: 'typeStringToTypeName'
> : not all control paths return a value [D:\Postgres\a\postgres.vcxproj]

Pushed.

I really wonder if we can't make msvc reliably recognize this kind of
scenario - especially this case is pretty trivial?

Which of:
#if defined(HAVE__BUILTIN_UNREACHABLE) && !defined(USE_ASSERT_CHECKING)
#define pg_unreachable() __builtin_unreachable()
#elif defined(_MSC_VER) && !defined(USE_ASSERT_CHECKING)
#define pg_unreachable() __assume(0)
#else
#define pg_unreachable() abort()
#endif

does your build end up using? Does changing things around make it
recognize this and similar cases?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 25 December 2014 at 00:34, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:

> On 2014-12-24 21:54:20 +1300, David Rowley wrote:
> > On 24 December 2014 at 07:41, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
> > wrote:
> >
> > > I have pushed patches 0001 through 0004, with some revisions. Only the
> > > final part is missing.
> > >
> > >
> > Hi Alvaro,
> >
> > Would you be able to commit the attached? It just fixes a new compiler
> > warning that I'm seeing on MSVC.
> >
> > src\backend\parser\parse_type.c(795): warning C4715:
> 'typeStringToTypeName'
> > : not all control paths return a value [D:\Postgres\a\postgres.vcxproj]
>
> Pushed.
>
> Thanks

> I really wonder if we can't make msvc reliably recognize this kind of
> scenario - especially this case is pretty trivial?
>
> Which of:
> #if defined(HAVE__BUILTIN_UNREACHABLE) && !defined(USE_ASSERT_CHECKING)
> #define pg_unreachable() __builtin_unreachable()
> #elif defined(_MSC_VER) && !defined(USE_ASSERT_CHECKING)
> #define pg_unreachable() __assume(0)
> #else
> #define pg_unreachable() abort()
> #endif
>
>
I don't think the problem is here. The problem is the the elevel being set
to a variable in the elog macro to prevent the multiple evaluation problem,
then since it does int elevel_ = elevel ... if (elevel_ >= ERROR) that's
not constant, or at least the microsoft compiler is not smart enough to see
that it is.

The attached patch removes the warning, but likely can't be used in case
someone somewhere is doing elog(var++, "my error");

Compiling with the attached shaves almost 1% off the size of postgres.exe:

before; 5,882,368 bytes
after: 5,830,656 bytes

I've been trawling around to try to see if anything
like __builtin_constant_p() exists for MSVC, but so far I've not found
anything useful.

Kind Regards

David Rowley

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> I really wonder if we can't make msvc reliably recognize this kind of
> scenario - especially this case is pretty trivial?

Even if MSVC did understand pg_unreachable(), there would be other
compilers that didn't, so we'd need to worry about suppressing such
warnings anyhow. Personally I'm just as happy that we have instances
of this case in the buildfarm where we can check it easily.

regards, tom lane

David Rowley <dgrowley(at)gmail(dot)com> writes:
> On 25 December 2014 at 00:34, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> I really wonder if we can't make msvc reliably recognize this kind of
>> scenario - especially this case is pretty trivial?

> The attached patch removes the warning, but likely can't be used in case
> someone somewhere is doing elog(var++, "my error");

Yeah, we're *not* doing that. There are definitely places where
ereport/elog are used with nonconstant elevel.

It's curious though that MSVC fails to notice that the variable never
changes. I wonder whether we could get away with changing the elog
macro to do
const int elevel_ = (elevel);
as ereport does, and whether it would help if so.

regards, tom lane

On 25 December 2014 at 04:47, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> David Rowley <dgrowley(at)gmail(dot)com> writes:
> > On 25 December 2014 at 00:34, Andres Freund <andres(at)2ndquadrant(dot)com>
> wrote:
> >> I really wonder if we can't make msvc reliably recognize this kind of
> >> scenario - especially this case is pretty trivial?
>
> > The attached patch removes the warning, but likely can't be used in case
> > someone somewhere is doing elog(var++, "my error");
>
> Yeah, we're *not* doing that. There are definitely places where
> ereport/elog are used with nonconstant elevel.
>
>
Agreed. The patch was intended as a demo of where the problem is. Although
I don't see why non-const elevel matters. Non-stable expressions are what
actually matter.

> It's curious though that MSVC fails to notice that the variable never
> changes. I wonder whether we could get away with changing the elog
> macro to do
> const int elevel_ = (elevel);
> as ereport does, and whether it would help if so.
>
>
Unlikely, as the one that was just fixed above is an ereport.

I'll dig around a little more and see if there's some way to get MSVC to
optimise this somehow. The 1% reduction in the postgres.exe seems worth a
little bit of investigation time.

Regards

David Rowley

Alvaro Herrera wrote:

> Patch 0005 adds getObjectIdentityParts(), which returns the object
> identity in arrays that can be passed to pg_get_object_address. This
> part needs slight revisions so I'm not sure I will be able to push
> tomorrow.

Here's a fresh version of this patch. I chose to add a SQL-accessible
version, pg_identify_object_as_address, to make it easier to test. In
doing so I noticed a couple of bugs, and most interestingly I noticed
that it was essentially impossible to cleanly address an array type;
doing a roundtrip through the new functions would get me the base type
when I used "integer[]" but the array type when I used "_int4". This
looked like a problem, so I traced through it and noticed that we're
using the type name *list* as a list, rather than as a TypeName, to
refer to OBJECT_TYPE and OBJECT_DOMAIN; I hadn't understood the
significance of this until I realized that domains would be represented
with arrayBounds set to a non-empty list for the integer[] syntax, but
the name list would have "pg_catalog integer" only; when the rest of
TypeName was discarded, the fact that we were talking about an array was
completely forgotten. Before the dawn of time we had this:

-static void
-CommentType(List *typename, char *comment)
-{
- TypeName *tname;
- Oid oid;
-
- /* XXX a bit of a crock; should accept TypeName in COMMENT syntax */
- tname = makeTypeNameFromNameList(typename);

where the XXX comment was removed by commit c10575ff005c330d047534562
without a corresponding comment in the new function.

I'm going to see about changing the grammar to get this fixed; this
patch is important because it will enable us to complete^Wcontinue
working on the DDL deparse testing framework.

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

(The changes in the regression test are bogus, BTW; I didn't care enough
to get them fixed before sorting out the rest of the mess.)

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

Here's a patch that tweaks the grammar to use TypeName in COMMENT,
SECURITY LABEL, and DROP for the type and domain cases. The required
changes in the code are pretty minimal, thankfully. Note the slight
changes to the new object_address test also.

I think I'm pretty much done with this now, so I intend to push this
first thing tomorrow and then the rebased getObjectIdentityParts patch
sometime later.

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

On Mon, Dec 29, 2014 at 2:15 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:

> Here's a patch that tweaks the grammar to use TypeName in COMMENT,
> SECURITY LABEL, and DROP for the type and domain cases. The required
> changes in the code are pretty minimal, thankfully. Note the slight
> changes to the new object_address test also.
>
> I think I'm pretty much done with this now, so I intend to push this
> first thing tomorrow and then the rebased getObjectIdentityParts patch
> sometime later.
>

This commit 3f88672a4e4d8e648d24ccc65 seems to have broken pg_upgrade for
pg_trgm.

On 9.2.9 freshly initdb and started with default config:

$ createdb jjanes

in psql:

create extension pg_trgm ;
create table foo (x text);
create index on foo using gin (upper(x) gin_trgm_ops);

Then run 9.5's pg_upgrade and it fails at the stage of restoring the
database schema.

The relevant log files are attached

Cheers,

Jeff

On Fri, Jan 2, 2015 at 9:59 PM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:

> On Mon, Dec 29, 2014 at 2:15 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
> wrote:
>
>> Here's a patch that tweaks the grammar to use TypeName in COMMENT,
>> SECURITY LABEL, and DROP for the type and domain cases. The required
>> changes in the code are pretty minimal, thankfully. Note the slight
>> changes to the new object_address test also.
>>
>> I think I'm pretty much done with this now, so I intend to push this
>> first thing tomorrow and then the rebased getObjectIdentityParts patch
>> sometime later.
>>
>
>
> This commit 3f88672a4e4d8e648d24ccc65 seems to have broken pg_upgrade for
> pg_trgm.
>
> On 9.2.9 freshly initdb and started with default config:
>
> $ createdb jjanes
>
> in psql:
>
> create extension pg_trgm ;
> create table foo (x text);
> create index on foo using gin (upper(x) gin_trgm_ops);
>
> Then run 9.5's pg_upgrade and it fails at the stage of restoring the
> database schema.
>
>
The problem also occurs doing a self-upgrade from 9.5 to 9.5.

The contents of the dump not changed meaningfully between 9.4 and 9.5.
I've isolated the problem to the backend applying the pg_restore of the
dump, regardless of which version created the dump.

After compiling 3c9e4cdbf2ec876dbb7 with CFLAGS="-fno-omit-frame-pointer",
I get this backtrace for the core-dump of postmaster during the pg_restore:

Core was generated by `postgres: jjanes jjanes [local] ALTER EXTENSION
'.
Program terminated with signal 11, Segmentation fault.
#0 0x00000000005135ff in NameListToString (names=0x257fcf8) at
namespace.c:2935
2935 if (IsA(name, String))
(gdb) bt
#0 0x00000000005135ff in NameListToString (names=0x257fcf8) at
namespace.c:2935
#1 0x0000000000512f33 in DeconstructQualifiedName (names=0x257fcf8,
nspname_p=0x7fff419bc960, objname_p=0x7fff419bc958) at namespace.c:2648
#2 0x000000000058a746 in LookupTypeName (pstate=0x0, typeName=0x257fd10,
typmod_p=0x0, missing_ok=0 '\000') at parse_type.c:153
#3 0x00000000005220b4 in get_object_address_type (objtype=OBJECT_TYPE,
objname=0x257fd50, missing_ok=0 '\000') at objectaddress.c:1306
#4 0x0000000000520cf5 in get_object_address (objtype=OBJECT_TYPE,
objname=0x257fd50, objargs=0x0, relp=0x7fff419bcb58, lockmode=4,
missing_ok=0 '\000')
at objectaddress.c:678
#5 0x00000000005c0f36 in ExecAlterExtensionContentsStmt (stmt=0x257fd80)
at extension.c:2906
#6 0x000000000077508c in ProcessUtilitySlow (parsetree=0x257fd80,
queryString=0x254f990 "\n-- For binary upgrade, must preserve pg_type
oid\nSELECT
binary_upgrade.set_next_pg_type_oid('16394'::pg_catalog.oid);\n\n\n-- For
binary upgrade, must preserve pg_type array oid\nSELECT binary_upgrade.se"...,
context=PROCESS_UTILITY_QUERY, params=0x0, dest=0x2581b60,
completionTag=0x7fff419bd100 "") at utility.c:1167
#7 0x000000000077490e in standard_ProcessUtility (parsetree=0x257fd80,
queryString=0x254f990 "\n-- For binary upgrade, must preserve pg_type
oid\nSELECT
binary_upgrade.set_next_pg_type_oid('16394'::pg_catalog.oid);\n\n\n-- For
binary upgrade, must preserve pg_type array oid\nSELECT binary_upgrade.se"...,
context=PROCESS_UTILITY_QUERY, params=0x0, dest=0x2581b60,
completionTag=0x7fff419bd100 "") at utility.c:840
#8 0x0000000000773bcc in ProcessUtility (parsetree=0x257fd80,
queryString=0x254f990 "\n-- For binary upgrade, must preserve pg_type
oid\nSELECT
binary_upgrade.set_next_pg_type_oid('16394'::pg_catalog.oid);\n\n\n-- For
binary upgrade, must preserve pg_type array oid\nSELECT binary_upgrade.se"...,
context=PROCESS_UTILITY_QUERY, params=0x0, dest=0x2581b60,
completionTag=0x7fff419bd100 "") at utility.c:313
#9 0x0000000000772dd6 in PortalRunUtility (portal=0x2505b90,
utilityStmt=0x257fd80, isTopLevel=0 '\000', dest=0x2581b60,
completionTag=0x7fff419bd100 "")
at pquery.c:1187
#10 0x0000000000772f8c in PortalRunMulti (portal=0x2505b90, isTopLevel=0
'\000', dest=0x2581b60, altdest=0x2581b60, completionTag=0x7fff419bd100 "")
at pquery.c:1318
#11 0x0000000000772560 in PortalRun (portal=0x2505b90,
count=9223372036854775807, isTopLevel=0 '\000', dest=0x2581b60,
altdest=0x2581b60,
completionTag=0x7fff419bd100 "") at pquery.c:816
#12 0x000000000076c868 in exec_simple_query (
query_string=0x254f990 "\n-- For binary upgrade, must preserve pg_type
oid\nSELECT
binary_upgrade.set_next_pg_type_oid('16394'::pg_catalog.oid);\n\n\n-- For
binary upgrade, must preserve pg_type array oid\nSELECT binary_upgrade.se"...)
at postgres.c:1045
#13 0x00000000007708a2 in PostgresMain (argc=1, argv=0x24ed5e0,
dbname=0x24ed4b8 "jjanes", username=0x24ed4a0 "jjanes") at postgres.c:4012
#14 0x0000000000701940 in BackendRun (port=0x250c1b0) at postmaster.c:4130
#15 0x0000000000701083 in BackendStartup (port=0x250c1b0) at
postmaster.c:3805
#16 0x00000000006fd8c5 in ServerLoop () at postmaster.c:1573
#17 0x00000000006fd013 in PostmasterMain (argc=18, argv=0x24ec480) at
postmaster.c:1220
#18 0x000000000066476b in main (argc=18, argv=0x24ec480) at main.c:220

Cheers,

Jeff

Jeff Janes wrote:
> On Fri, Jan 2, 2015 at 9:59 PM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:

> > This commit 3f88672a4e4d8e648d24ccc65 seems to have broken pg_upgrade for
> > pg_trgm.

It seems I failed to notice the get_object_address in the ALTER
EXTENSION path. Should be fixed now. I looked for similar missed
callsites and didn't find anything.

Thanks for the report!

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