Thread: get_object_address support for additional object types
This is extracted from the DDL deparse series. These patches add
get_object_address support for the following object types:
- user mappings
- default ACLs
- operators and functions of operator families
In each case I had to create a new value in ObjectType. These object
types can not be created from the parser, which is why they don't exist
yet. But if we want to be able to process DDL for them, then we need to
cope at this level. This is the kind of fix we need to handle the
failures related to commit a486841eb11517e.
There is one strange thing in the last one, which is that an opfamily
member is represented in two arrays like this (objname, objargs):
{opfamily identity, access method identity, number} , {left type, right type}
This is a bit odd considering that operator families themselves are
addressed like this instead:
{opfamily identity} , {access method identity}
Note that the AM name is originally in objargs and moves to objnames.
The reason I did it this way is that the objargs elements can be
processed completely as an array of TypeName, and therefore there's no
need for an extra strange case in pg_get_object_address. But it does
mean that there is some code that knows to search the strategy or
function number in a specific position in the objname array.
If we had more freedom on general object representation I'm sure we
could do better, but it's what we have. I don't think it's a tremendous
problem, considering that get_object_address gets a fairly ad-hoc
representation for each object type anyway, as each gets constructed by
the grammar.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
Alvaro,
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> This is extracted from the DDL deparse series. These patches add
> get_object_address support for the following object types:
>
> - user mappings
> - default ACLs
> - operators and functions of operator families
I took a (relatively quick) look through these patches.
> Subject: [PATCH 1/3] deparse/core: get_object_address support user mappings
[...]
I thought this looked fine. One minor nit-pick is that the function added
doesn't have a single comment, but it's a pretty short too.
> Subject: [PATCH 2/3] deparse/core: get_object_address support default ACLs
[...]
> + char *stuff;
Nit-pick, but 'stuff' isn't really a great variable name. :) Perhaps
'defacltype_name'? It's longer, sure, but it's not used a lot..
> Subject: [PATCH 3/3] deparse/core: get_object_address support opfamily members
> @@ -661,7 +664,8 @@ get_object_address(ObjectType objtype, List *objname, List *objargs,
> ObjectAddress domaddr;
> char *constrname;
>
> - domaddr = get_object_address_type(OBJECT_DOMAIN, objname, missing_ok);
> + domaddr = get_object_address_type(OBJECT_DOMAIN,
> + list_head(objname), missing_ok);
> constrname = strVal(linitial(objargs));
>
> address.classId = ConstraintRelationId;
I don't really care for how all the get_object_address stuff uses lists
for arguments instead of using straight-forward arguments, but it's how
it's been done and I can kind of see the reasoning behind it. I'm not
following why you're switching this case (get_object_address_type) to
using a ListCell though..?
I thought the rest of it looked alright. I agree it's a bit odd how the
opfamily is handled but I agree with your assessment that there's not
much better we can do with this object representation.
Thanks!
Stephen
Stephen, thanks for the comments. Stephen Frost wrote: > I don't really care for how all the get_object_address stuff uses lists > for arguments instead of using straight-forward arguments, but it's how > it's been done and I can kind of see the reasoning behind it. I'm not > following why you're switching this case (get_object_address_type) to > using a ListCell though..? The reason for changing get_object_address_type is that it's a helper soubroutine to get_object_address and we can do whatever is more convenient. It turns out that it's more convenient, for the amop/amproc cases, to pass a ListCell instead of a List. get_object_address gets its stuff directly from the parser in some cases, or from pg_get_object_address otherwise, which constructs things to mimick exactly what the parser does. We can change what the parser emits and as long as we keep get_object_address in agreement, there is no issue. There might be code churn of course (because some of those object representations travel from the parser through other parts of the backend), but that doesn't really matter much. We have now exposed that interface through the SQL-callable pg_get_object_address function, but I'm pretty sure we could change that easily and it wouldn't be a tremendous problem -- as long as we do it before 9.5 is released. For instance we might want to decide that we want to have three lists instead of two; or two lists and a numeric quantity (which would also help the large object case, currently a bit ugly), or that we want something akin to what the parser does to types using struct TypeName for opclass-related objects. Anyway I'm not hot on changing anything here. It's a bit cumbersome an interface to use, but it's not excessively exposed to the user unless they use event triggers, and even then it is perfectly reliable anyhow. > I thought the rest of it looked alright. I agree it's a bit odd how the > opfamily is handled but I agree with your assessment that there's not > much better we can do with this object representation. Great, thanks. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> Anyway I'm not hot on changing anything here. It's a bit cumbersome an
> interface to use, but it's not excessively exposed to the user unless
> they use event triggers, and even then it is perfectly reliable anyhow.
Works for me.
Thanks!
Stephen
Stephen Frost wrote: > I thought the rest of it looked alright. I agree it's a bit odd how the > opfamily is handled but I agree with your assessment that there's not > much better we can do with this object representation. Actually, on second thought I revisited this and changed the representation for opfamilies and opclasses: instead of putting the AM name in objargs, we can put it as the first element of objname instead. That way, objargs is unused for opfamilies and opclasses, and we're free to use it for the type arguments in amops and amprocs. This makes the lists consistent for the four cases: in objname, amname first, then qualified opclass/opfamily name. For amop/amproc, the member number follows. Objargs is unused in opclass/opfamily, and it's a two-element list of types in amop/amproc. The attached patch changes the grammar to comply with the above, and adds the necessary get_object_address and getObjectIdentityParts support code for amop/amproc objects. The only thing a bit unusual is that in does_not_exist_skipping() we need to do an list_copy_tail() to strip out the amname before reporting the "skipping" message, for DROP OPERATOR CLASS/FAMILY IF NOT EXISTS. I don't think this is a problem. In return, the code to deconstruct amop and amproc addresses is more sensible. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
Alvaro,
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> Stephen Frost wrote:
> > I thought the rest of it looked alright. I agree it's a bit odd how the
> > opfamily is handled but I agree with your assessment that there's not
> > much better we can do with this object representation.
>
> Actually, on second thought I revisited this and changed the
> representation for opfamilies and opclasses: instead of putting the AM
> name in objargs, we can put it as the first element of objname instead.
> That way, objargs is unused for opfamilies and opclasses, and we're free
> to use it for the type arguments in amops and amprocs. This makes the
> lists consistent for the four cases: in objname, amname first, then
> qualified opclass/opfamily name. For amop/amproc, the member number
> follows. Objargs is unused in opclass/opfamily, and it's a two-element
> list of types in amop/amproc.
Agreed, that makes more sense to me also.
> The attached patch changes the grammar to comply with the above, and
> adds the necessary get_object_address and getObjectIdentityParts support
> code for amop/amproc objects.
I took a quick look through and it looked fine to me.
> The only thing a bit unusual is that in does_not_exist_skipping() we
> need to do an list_copy_tail() to strip out the amname before reporting
> the "skipping" message, for DROP OPERATOR CLASS/FAMILY IF NOT EXISTS.
> I don't think this is a problem. In return, the code to deconstruct
> amop and amproc addresses is more sensible.
Works for me.
Thanks!
Stephen
Stephen Frost wrote: > Alvaro, > > * Alvaro Herrera (alvherre@2ndquadrant.com) wrote: > > Stephen Frost wrote: > > > I thought the rest of it looked alright. I agree it's a bit odd how the > > > opfamily is handled but I agree with your assessment that there's not > > > much better we can do with this object representation. > > > > Actually, on second thought I revisited this and changed the > > representation for opfamilies and opclasses: instead of putting the AM > > name in objargs, we can put it as the first element of objname instead. > > That way, objargs is unused for opfamilies and opclasses, and we're free > > to use it for the type arguments in amops and amprocs. This makes the > > lists consistent for the four cases: in objname, amname first, then > > qualified opclass/opfamily name. For amop/amproc, the member number > > follows. Objargs is unused in opclass/opfamily, and it's a two-element > > list of types in amop/amproc. > > Agreed, that makes more sense to me also. Great, thanks for checking -- pushed that way. > > The attached patch changes the grammar to comply with the above, and > > adds the necessary get_object_address and getObjectIdentityParts support > > code for amop/amproc objects. > > I took a quick look through and it looked fine to me. Actually, there was a bug in the changes of the rule for ALTER EXTENSION ADD OPERATOR CLASS. I noticed by chance only, and upon testing it manually I realized I had made a mistake. I then remembered I made the same bug previously, fixed by 5c5ffee80f35, and I'm not wondering why do we not have any test for ALTER EXTENSION ADD other than pg_upgrading some database that contains an extension which uses each command. This seems pretty dangerous to me, generally speaking ... we should definitely be testing all these ALTER EXTENSION commands. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> Actually, there was a bug in the changes of the rule for ALTER EXTENSION
> ADD OPERATOR CLASS. I noticed by chance only, and upon testing it
> manually I realized I had made a mistake. I then remembered I made the
> same bug previously, fixed by 5c5ffee80f35, and I'm not wondering why do
> we not have any test for ALTER EXTENSION ADD other than pg_upgrading
> some database that contains an extension which uses each command. This
> seems pretty dangerous to me, generally speaking ... we should
> definitely be testing all these ALTER EXTENSION commands.
It'd certainly be great to have more testing in general, but we're not
going to be able to include complete code coverage tests in the normal
set of regression tests which are run.. What I've been thinking for a
while (probably influenced by other discussion) is that we should have
the buildfarm running tests for code coverage as those are async from
the development process. That isn't to say we shouldn't add more tests
to the main regression suite when it makes sense to, we certainly
should, but we really need to be looking at code coverage tools and
adding tests to improve our test coverage which can be run by the
buildfarm animals (or even just a subset of them, if we feel that having
all the animals running them would be excessive).
Thanks!
Stephen
Stephen Frost wrote: > It'd certainly be great to have more testing in general, but we're not > going to be able to include complete code coverage tests in the normal > set of regression tests which are run.. What I've been thinking for a > while (probably influenced by other discussion) is that we should have > the buildfarm running tests for code coverage as those are async from > the development process. That isn't to say we shouldn't add more tests > to the main regression suite when it makes sense to, we certainly > should, but we really need to be looking at code coverage tools and > adding tests to improve our test coverage which can be run by the > buildfarm animals (or even just a subset of them, if we feel that having > all the animals running them would be excessive). Well, we already have make targets for gcov and friends; you get some HTML charts and marked-up source lines with coverage counts, etc. I don't think we've made any use of that. It'd be neat to have something similar to our doxygen service, running some test suite and publishing the reports on the web. I remember trying to convince someone to set that up for the community, but that seems to have yield no results. We had someone else trying to submit patches to improve coverage of the regression tests, but (probably due to wrong stars alignment) they started with CREATE DATABASE which made the tests a lot slower, which got the patches turned down -- the submitter disappeared after that IIRC, probably discouraged by the lack of results. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro,
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> Stephen Frost wrote:
> > It'd certainly be great to have more testing in general, but we're not
> > going to be able to include complete code coverage tests in the normal
> > set of regression tests which are run.. What I've been thinking for a
> > while (probably influenced by other discussion) is that we should have
> > the buildfarm running tests for code coverage as those are async from
> > the development process. That isn't to say we shouldn't add more tests
> > to the main regression suite when it makes sense to, we certainly
> > should, but we really need to be looking at code coverage tools and
> > adding tests to improve our test coverage which can be run by the
> > buildfarm animals (or even just a subset of them, if we feel that having
> > all the animals running them would be excessive).
>
> Well, we already have make targets for gcov and friends; you get some
> HTML charts and marked-up source lines with coverage counts, etc. I
> don't think we've made any use of that. It'd be neat to have something
> similar to our doxygen service, running some test suite and publishing
> the reports on the web. I remember trying to convince someone to set
> that up for the community, but that seems to have yield no results.
I don't think we've made use of it either. If the targets/code are
already there to make it happen and it's just a matter of having a
system running which is generating the website then I can probably get
that going. I was under the impression that there was more to it than
that though.
> We had someone else trying to submit patches to improve coverage of the
> regression tests, but (probably due to wrong stars alignment) they
> started with CREATE DATABASE which made the tests a lot slower, which
> got the patches turned down -- the submitter disappeared after that
> IIRC, probably discouraged by the lack of results.
Agreed, and I think that's unfortunate. It's an area which we could
really improve in and would be a good place for someone new to the
community to be able to contribute- but we need to provide the right way
for those tests to be added and that way isn't to include them in the
main suite of tests which are run during development.
Thanks!
Stephen
Stephen Frost wrote: > Alvaro, > > * Alvaro Herrera (alvherre@2ndquadrant.com) wrote: > > Well, we already have make targets for gcov and friends; you get some > > HTML charts and marked-up source lines with coverage counts, etc. I > > don't think we've made any use of that. It'd be neat to have something > > similar to our doxygen service, running some test suite and publishing > > the reports on the web. I remember trying to convince someone to set > > that up for the community, but that seems to have yield no results. > > I don't think we've made use of it either. If the targets/code are > already there to make it happen and it's just a matter of having a > system running which is generating the website then I can probably get > that going. I was under the impression that there was more to it than > that though. "configure --enable-coverage"; install the resulting tree; then run whatever tests you want, then "make coverage". That's enough to get the HTML reports. > > We had someone else trying to submit patches to improve coverage of the > > regression tests, but (probably due to wrong stars alignment) they > > started with CREATE DATABASE which made the tests a lot slower, which > > got the patches turned down -- the submitter disappeared after that > > IIRC, probably discouraged by the lack of results. > > Agreed, and I think that's unfortunate. It's an area which we could > really improve in and would be a good place for someone new to the > community to be able to contribute- but we need to provide the right way > for those tests to be added and that way isn't to include them in the > main suite of tests which are run during development. Well, I don't disagree that we could do with some tests that are run additionally to the ones we currently have, but some basic stuff that takes almost no time to run would be adequate to have in the basic regression tests. The one I'm thinking is something like generate a VALUES of all the supported object types, then running "pg_get_object_address" on them to make sure we support all object types (or at least that we're aware which object types are not supported.) For instance, Petr Jelinek's patch to add the TABLESAMPLE clause adds a new object type which needs support in get_object_address, but I'm not sure it's added to the stuff in the object_address test. It's a very easy omission to make. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2015-03-16 12:50:13 -0300, Alvaro Herrera wrote: > Well, we already have make targets for gcov and friends; you get some > HTML charts and marked-up source lines with coverage counts, etc. I > don't think we've made any use of that. It'd be neat to have something > similar to our doxygen service, running some test suite and publishing > the reports on the web. I remember trying to convince someone to set > that up for the community, but that seems to have yield no results. Actually I think Peter E. has: http://pgci.eisentraut.org/jenkins/job/postgresql_master_coverage/Coverage/ > We had someone else trying to submit patches to improve coverage of the > regression tests, but (probably due to wrong stars alignment) they > started with CREATE DATABASE which made the tests a lot slower, which > got the patches turned down -- the submitter disappeared after that > IIRC, probably discouraged by the lack of results. I seem to recall that he'd also submitted a bunch of other things, and that some of it was applied? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Alvaro,
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> Stephen Frost wrote:
> > I don't think we've made use of it either. If the targets/code are
> > already there to make it happen and it's just a matter of having a
> > system running which is generating the website then I can probably get
> > that going. I was under the impression that there was more to it than
> > that though.
>
> "configure --enable-coverage"; install the resulting tree; then run
> whatever tests you want, then "make coverage". That's enough to get the
> HTML reports.
Ok, cool, I'll take a look at that.
> > > We had someone else trying to submit patches to improve coverage of the
> > > regression tests, but (probably due to wrong stars alignment) they
> > > started with CREATE DATABASE which made the tests a lot slower, which
> > > got the patches turned down -- the submitter disappeared after that
> > > IIRC, probably discouraged by the lack of results.
> >
> > Agreed, and I think that's unfortunate. It's an area which we could
> > really improve in and would be a good place for someone new to the
> > community to be able to contribute- but we need to provide the right way
> > for those tests to be added and that way isn't to include them in the
> > main suite of tests which are run during development.
>
> Well, I don't disagree that we could do with some tests that are run
> additionally to the ones we currently have, but some basic stuff that
> takes almost no time to run would be adequate to have in the basic
> regression tests. The one I'm thinking is something like generate a
> VALUES of all the supported object types, then running
> "pg_get_object_address" on them to make sure we support all object types
> (or at least that we're aware which object types are not supported.)
Agreed, that sounds perfectly reasonable to me. I didn't mean to imply
that we shouldn't add tests where they make sense, including to the core
regression suites (particularly coverage tests like that one you're
suggesting here), just pointing out that we need a way to address code
coverage based tested also which is done outside of the main regression
suite.
> For instance, Petr Jelinek's patch to add the TABLESAMPLE clause adds a
> new object type which needs support in get_object_address, but I'm not
> sure it's added to the stuff in the object_address test. It's a very
> easy omission to make.
Agreed.
Thanks!
Stephen
* Andres Freund (andres@2ndquadrant.com) wrote: > On 2015-03-16 12:50:13 -0300, Alvaro Herrera wrote: > > Well, we already have make targets for gcov and friends; you get some > > HTML charts and marked-up source lines with coverage counts, etc. I > > don't think we've made any use of that. It'd be neat to have something > > similar to our doxygen service, running some test suite and publishing > > the reports on the web. I remember trying to convince someone to set > > that up for the community, but that seems to have yield no results. > > Actually I think Peter E. has: > http://pgci.eisentraut.org/jenkins/job/postgresql_master_coverage/Coverage/ Neat! Thanks, Stephen
On 16 March 2015 at 15:11, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: >> > Actually, on second thought I revisited this and changed the >> > representation for opfamilies and opclasses: instead of putting the AM >> > name in objargs, we can put it as the first element of objname instead. >> > That way, objargs is unused for opfamilies and opclasses, and we're free >> > to use it for the type arguments in amops and amprocs. This makes the >> > lists consistent for the four cases: in objname, amname first, then >> > qualified opclass/opfamily name. For amop/amproc, the member number >> > follows. Objargs is unused in opclass/opfamily, and it's a two-element >> > list of types in amop/amproc. >> >> Agreed, that makes more sense to me also. > > Great, thanks for checking -- pushed that way. > I'm getting a couple of compiler warnings that I think are coming from this commit: objectaddress.c: In function ‘get_object_address’: objectaddress.c:1428:12: warning: array subscript is above array bounds objectaddress.c:1430:11: warning: array subscript is above array bounds I think because the compiler doesn't know the list size, so i could be 0,1,2. Regards, Dean
I pushed a fix for this. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services