Обсуждение: 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@2ndquadrant.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@2ndquadrant.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.brightwell@crunchydatasolutions.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@2ndquadrant.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@2ndquadrant.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@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: > 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@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
* Alvaro Herrera (alvherre@2ndquadrant.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@2ndquadrant.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@2ndquadrant.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@2ndquadrant.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@2ndquadrant.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.Nasby@bluetreble.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, notjust 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@2ndquadrant.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@2ndquadrant.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@2ndquadrant.com> wrote:
On Wednesday, October 15, 2014, Alvaro Herrera <alvherre@2ndquadrant.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@2ndquadrant.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@2ndquadrant.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@2ndquadrant.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@2ndquadrant.com> wrote:
On 2014-12-24 21:54:20 +1300, David Rowley wrote:
> On 24 December 2014 at 07:41, Alvaro Herrera <alvherre@2ndquadrant.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@2ndquadrant.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@gmail.com> writes: > On 25 December 2014 at 00:34, Andres Freund <andres@2ndquadrant.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@sss.pgh.pa.us> wrote:
David Rowley <dgrowley@gmail.com> writes:
> On 25 December 2014 at 00:34, Andres Freund <andres@2ndquadrant.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@2ndquadrant.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.janes@gmail.com> wrote:
On Mon, Dec 29, 2014 at 2:15 PM, Alvaro Herrera <alvherre@2ndquadrant.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 jjanesin 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.janes@gmail.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