Thread: pg_partition_tree crashes for a non-defined relation
Hi all, While testing another patch, I have bumped into the issue of $subject... I should have put some more negative testing from the start on this stuff, here is a culprit query when passing directly an OID: select pg_partition_tree(0); I think that we should make the function return NULL if the relation defined does not exist, as we usually do for system-facing functions. It is also easier for the caller to know that the relation does not exist instead of having a plpgsql try/catch wrapper or such. Thoughts? -- Michael
Attachment
On Fri, Dec 07, 2018 at 10:04:06AM +0900, Michael Paquier wrote: > While testing another patch, I have bumped into the issue of > $subject... I should have put some more negative testing from the start > on this stuff, here is a culprit query when passing directly an OID: > select pg_partition_tree(0); > > I think that we should make the function return NULL if the relation > defined does not exist, as we usually do for system-facing functions. > It is also easier for the caller to know that the relation does not > exist instead of having a plpgsql try/catch wrapper or such. > > Thoughts? Are there any objections about fixing this issue? I would rather fix it sonner than later. -- Michael
Attachment
Hi,
Sorry for not replying sooner.
On Sat, Dec 8, 2018 at 8:06 Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Dec 07, 2018 at 10:04:06AM +0900, Michael Paquier wrote:
> While testing another patch, I have bumped into the issue of
> $subject... I should have put some more negative testing from the start
> on this stuff, here is a culprit query when passing directly an OID:
> select pg_partition_tree(0);
>
> I think that we should make the function return NULL if the relation
> defined does not exist, as we usually do for system-facing functions.
> It is also easier for the caller to know that the relation does not
> exist instead of having a plpgsql try/catch wrapper or such.
>
> Thoughts?
Are there any objections about fixing this issue? I would rather fix it
sonner than later.
Thanks for noticing it and creating the patch. The fix makes sense.
Regards,
Amit
On Sat, Dec 08, 2018 at 12:28:53PM +0900, Amit Langote wrote: > Thanks for noticing it and creating the patch. The fix makes sense. Thanks a lot for looking at it! -- Michael
Attachment
Michael Paquier <michael@paquier.xyz> writes: > On Fri, Dec 07, 2018 at 10:04:06AM +0900, Michael Paquier wrote: >> I think that we should make the function return NULL if the relation >> defined does not exist, as we usually do for system-facing functions. >> It is also easier for the caller to know that the relation does not >> exist instead of having a plpgsql try/catch wrapper or such. > Are there any objections about fixing this issue? I would rather fix it > sonner than later. Return NULL seems a reasonable behavior. How about cases where the relation OID exists but it's the wrong kind of relation? regards, tom lane
On Fri, Dec 07, 2018 at 11:33:32PM -0500, Tom Lane wrote: > How about cases where the relation OID exists but it's the wrong > kind of relation? Such cases already return an error: =# create sequence popo; CREATE SEQUENCE =# select pg_partition_tree('popo'); ERROR: 42809: "popo" is not a table, a foreign table, or an index LOCATION: pg_partition_tree, partitionfuncs.c:54 I think that's a sensible choice, because it makes no sense to look for the inheritors of unsupported relkinds. Perhaps you have a different view on the matter? -- Michael
Attachment
Greetings, * Michael Paquier (michael@paquier.xyz) wrote: > On Fri, Dec 07, 2018 at 11:33:32PM -0500, Tom Lane wrote: > > How about cases where the relation OID exists but it's the wrong > > kind of relation? > > Such cases already return an error: > =# create sequence popo; > CREATE SEQUENCE > =# select pg_partition_tree('popo'); > ERROR: 42809: "popo" is not a table, a foreign table, or an index > LOCATION: pg_partition_tree, partitionfuncs.c:54 > > I think that's a sensible choice, because it makes no sense to look for > the inheritors of unsupported relkinds. Perhaps you have a different > view on the matter? We should really have a more clearly defined policy around this, but my recollection is that we often prefer to return NULL rather than throwing an error for the convenience of people doing things like querying pg_class using similar functions. I wonder if we maybe should have a regression test for every such function which just queries the catalog in a way to force the function to be called for every relation defined in the regression tests, to ensure that it doesn't segfault or throw an error.. Thanks! Stephen
Attachment
On Sat, Dec 08, 2018 at 08:46:08AM -0500, Stephen Frost wrote: > We should really have a more clearly defined policy around this, but my > recollection is that we often prefer to return NULL rather than throwing > an error for the convenience of people doing things like querying > pg_class using similar functions. Yes, that's visibly right. At least that's what I can see from the various pg_get_*def and pg_*_is_visible. Returning NULL would indeed be more consistent. > I wonder if we maybe should have a regression test for every such > function which just queries the catalog in a way to force the function > to be called for every relation defined in the regression tests, to > ensure that it doesn't segfault or throw an error.. Like sqlsmith? It looks hard to me to make something like that part of the main regression test suite, as that's going to be costly and hard to scale with. -- Michael
Attachment
On Sun, Dec 09, 2018 at 08:15:07AM +0900, Michael Paquier wrote: > On Sat, Dec 08, 2018 at 08:46:08AM -0500, Stephen Frost wrote: >> We should really have a more clearly defined policy around this, but my >> recollection is that we often prefer to return NULL rather than throwing >> an error for the convenience of people doing things like querying >> pg_class using similar functions. > > Yes, that's visibly right. At least that's what I can see from the > various pg_get_*def and pg_*_is_visible. Returning NULL would indeed > be more consistent. Thinking more about your argument, scanning fully pg_class is quite sensible as well because there is no need to apply an extra qual on relkind, so let's change the function as you suggest, by returning NULL on invalid relation type. Any opinions about the attached then which does the switch? -- Michael
Attachment
Greetings, * Michael Paquier (michael@paquier.xyz) wrote: > On Sat, Dec 08, 2018 at 08:46:08AM -0500, Stephen Frost wrote: > > I wonder if we maybe should have a regression test for every such > > function which just queries the catalog in a way to force the function > > to be called for every relation defined in the regression tests, to > > ensure that it doesn't segfault or throw an error.. > > Like sqlsmith? It looks hard to me to make something like that part of > the main regression test suite, as that's going to be costly and hard to > scale with. No, I mean something like: with x as (select pg_partition_tree(relname) from pg_class) select 1 from x limit 1; or whatever it takes to make sure that the function is run against every entry in pg_class (or whatever is appropriate) while not returning the results (since we don't actually care about the output, we just want to make sure it doesn't ERROR or crash). sqlsmith, as I recall, doesn't care about ERROR cases, it's just looking for crashes, so it's not quite the same thing. Thanks! Stephen
Attachment
Greetings, * Michael Paquier (michael@paquier.xyz) wrote: > On Sun, Dec 09, 2018 at 08:15:07AM +0900, Michael Paquier wrote: > > On Sat, Dec 08, 2018 at 08:46:08AM -0500, Stephen Frost wrote: > >> We should really have a more clearly defined policy around this, but my > >> recollection is that we often prefer to return NULL rather than throwing > >> an error for the convenience of people doing things like querying > >> pg_class using similar functions. > > > > Yes, that's visibly right. At least that's what I can see from the > > various pg_get_*def and pg_*_is_visible. Returning NULL would indeed > > be more consistent. > > Thinking more about your argument, scanning fully pg_class is quite > sensible as well because there is no need to apply an extra qual on > relkind, so let's change the function as you suggest, by returning NULL > on invalid relation type. Any opinions about the attached then which > does the switch? Looks alright on a quick glance, but shouldn't you also update the comment..? Thanks! Stephen
Attachment
Stephen Frost <sfrost@snowman.net> writes: > * Michael Paquier (michael@paquier.xyz) wrote: >> On Sat, Dec 08, 2018 at 08:46:08AM -0500, Stephen Frost wrote: >>> I wonder if we maybe should have a regression test for every such >>> function which just queries the catalog in a way to force the function >>> to be called for every relation defined in the regression tests, to >>> ensure that it doesn't segfault or throw an error.. > No, I mean something like: > with x as (select pg_partition_tree(relname) from pg_class) > select 1 from x limit 1; > or whatever it takes to make sure that the function is run against every > entry in pg_class (or whatever is appropriate) while not returning the > results (since we don't actually care about the output, we just want to > make sure it doesn't ERROR or crash). I'm going to object to that on cost grounds. It is not reasonable to run moderately-expensive functions like this on more than a thousand pg_class entries in order to test what are just a few distinct cases, especially in code that's highly unlikely to break once written. Or at least, it's not reasonable to do that every time anybody runs the regression tests for the rest of our lives. Moreover, this would only help check a specific new function if the author or reviewer remembered to add a test case that does it. Since the whole problem here is "I forgot to consider this", I don't have much faith in that happening. Maybe we should have some sort of checklist of things to think about when adding new SQL-visible functions, rather than trying to memorialize every such consideration as a regression test no matter how expensive. Admittedly, "I forgot to go over the checklist" is still a problem; but at least it's not penalizing every developer and every buildfarm run till kingdom come. regards, tom lane
Greetings, * Tom Lane (tgl@sss.pgh.pa.us) wrote: > Stephen Frost <sfrost@snowman.net> writes: > > * Michael Paquier (michael@paquier.xyz) wrote: > >> On Sat, Dec 08, 2018 at 08:46:08AM -0500, Stephen Frost wrote: > >>> I wonder if we maybe should have a regression test for every such > >>> function which just queries the catalog in a way to force the function > >>> to be called for every relation defined in the regression tests, to > >>> ensure that it doesn't segfault or throw an error.. > > > No, I mean something like: > > with x as (select pg_partition_tree(relname) from pg_class) > > select 1 from x limit 1; > > or whatever it takes to make sure that the function is run against every > > entry in pg_class (or whatever is appropriate) while not returning the > > results (since we don't actually care about the output, we just want to > > make sure it doesn't ERROR or crash). > > I'm going to object to that on cost grounds. It is not reasonable to > run moderately-expensive functions like this on more than a thousand > pg_class entries in order to test what are just a few distinct cases, > especially in code that's highly unlikely to break once written. Yeah, I had been wondering if it would be too expensive also. I don't entirely buy off on the argument that it's code that's 'highly unlikely to break once written' though- we do add new relkinds from time to time, for example. Perhaps we could have these functions run just once per relkind. > Moreover, this would only help check a specific new function if the > author or reviewer remembered to add a test case that does it. We could possibly write a test which would run every function that accepts the typical data types (oid/regclass/name/etc) instead of depending on the author to remember to add it. > Since the whole problem here is "I forgot to consider this", I don't > have much faith in that happening. Yeah, I agree that we'd want something more automated as, otherwise, it would definitely be likely to be forgotten. > Maybe we should have some sort of checklist of things to think about > when adding new SQL-visible functions, rather than trying to > memorialize every such consideration as a regression test no matter > how expensive. Admittedly, "I forgot to go over the checklist" is > still a problem; but at least it's not penalizing every developer and > every buildfarm run till kingdom come. This seems like something we should do regardless. Moreover, I'd suggest that we start documenting some of these policies in the way that we have a style guide for errors and such- we need a "system function style guide" that could start with something like "prefer to return NULL instead of ERROR on unexpected but otherwise valid inputs, and test that the code doesn't segfault when given a variety of inputs". Thanks! Stephen
Attachment
Stephen Frost <sfrost@snowman.net> writes: > * Tom Lane (tgl@sss.pgh.pa.us) wrote: >> ... especially in code that's highly unlikely to break once written. > I don't entirely buy off on the argument that it's code that's 'highly > unlikely to break once written' though- we do add new relkinds from time > to time, for example. Perhaps we could have these functions run just > once per relkind. Well, the relevant code is likely to be "if relkind is not x, y, or z, then PG_RETURN_NULL". If we add a new relkind and forget to consider the function, the outcome is a NULL result that perhaps should not have been NULL ... but a test like this won't help us notice that. regards, tom lane
On Sun, Dec 09, 2018 at 02:07:29PM -0500, Tom Lane wrote: > Stephen Frost <sfrost@snowman.net> writes: >> I don't entirely buy off on the argument that it's code that's 'highly >> unlikely to break once written' though- we do add new relkinds from time >> to time, for example. Perhaps we could have these functions run just >> once per relkind. > > Well, the relevant code is likely to be "if relkind is not x, y, or z, > then PG_RETURN_NULL". If we add a new relkind and forget to consider the > function, the outcome is a NULL result that perhaps should not have been > NULL ... but a test like this won't help us notice that. Yes, in order to prevent problems with newly-introduced relkinds, I think that the checks within functions should be careful to check only for relkinds that they support, and not list those they do not support. -- Michael
Attachment
On Sun, Dec 09, 2018 at 12:49:18PM -0500, Stephen Frost wrote: > Looks alright on a quick glance, but shouldn't you also update the > comment..? The comment on HEAD or with the patch is that: /* Only allow relation types that can appear in partition trees. */ This still looks adapted to me. Or would you reword it because "allow" rather implies that an ERROR is returned? Would you prefer changing it something like that perhaps: "Return NULL for relation types that do not appear in partition trees." -- Michael
Attachment
Greetings, * Michael Paquier (michael@paquier.xyz) wrote: > On Sun, Dec 09, 2018 at 02:07:29PM -0500, Tom Lane wrote: > > Stephen Frost <sfrost@snowman.net> writes: > >> I don't entirely buy off on the argument that it's code that's 'highly > >> unlikely to break once written' though- we do add new relkinds from time > >> to time, for example. Perhaps we could have these functions run just > >> once per relkind. > > > > Well, the relevant code is likely to be "if relkind is not x, y, or z, > > then PG_RETURN_NULL". If we add a new relkind and forget to consider the > > function, the outcome is a NULL result that perhaps should not have been > > NULL ... but a test like this won't help us notice that. > > Yes, in order to prevent problems with newly-introduced relkinds, I > think that the checks within functions should be careful to check only > for relkinds that they support, and not list those they do not support. Yes, but it's certainly possible for someone to add another relkind to that list without looking through the rest of the function carefully enough. Perhaps not the best example. I still don't like the assumption that the code won't be broken once it's written correctly the first time though, and I continue to feel that it would be good to have regression tests which run these functions with interesting arguments. I also agree that we don't want to make the regression tests take a lot of additional time. Thanks! Stephen
Attachment
Greetings, * Michael Paquier (michael@paquier.xyz) wrote: > On Sun, Dec 09, 2018 at 12:49:18PM -0500, Stephen Frost wrote: > > Looks alright on a quick glance, but shouldn't you also update the > > comment..? > > The comment on HEAD or with the patch is that: > /* Only allow relation types that can appear in partition trees. */ > > This still looks adapted to me. Or would you reword it because "allow" > rather implies that an ERROR is returned? Would you prefer changing it > something like that perhaps: > "Return NULL for relation types that do not appear in partition trees." Yes. Thanks! Stephen
Attachment
On Mon, Dec 10, 2018 at 08:21:43AM -0500, Stephen Frost wrote: > * Michael Paquier (michael@paquier.xyz) wrote: >> This still looks adapted to me. Or would you reword it because "allow" >> rather implies that an ERROR is returned? Would you prefer changing it >> something like that perhaps: >> "Return NULL for relation types that do not appear in partition trees." > > Yes. OK. Sure, let's do as you suggest then. I'll wait a couple of days before committing the patch so as if someone has extra comments they have the time to post. So please feel free to comment! -- Michael
Attachment
On Mon, Dec 10, 2018 at 10:52:37PM +0900, Michael Paquier wrote: > OK. Sure, let's do as you suggest then. I'll wait a couple of days > before committing the patch so as if someone has extra comments they > have the time to post. So please feel free to comment! And done this way. Thanks for the input, Stephen and others! -- Michael
Attachment
On 2018/12/12 9:52, Michael Paquier wrote: > On Mon, Dec 10, 2018 at 10:52:37PM +0900, Michael Paquier wrote: >> OK. Sure, let's do as you suggest then. I'll wait a couple of days >> before committing the patch so as if someone has extra comments they >> have the time to post. So please feel free to comment! > > And done this way. Thanks for the input, Stephen and others! Thank you Michael for taking care of this. I agree with the consensus that instead of throwing an error for unsupported relkinds, pg_partition_tree should rather return single row containing NULL for all columns. Regards, Amit
On Thu, Dec 13, 2018 at 03:34:56PM +0900, Amit Langote wrote: > Thank you Michael for taking care of this. I agree with the consensus > that instead of throwing an error for unsupported relkinds, > pg_partition_tree should rather return single row containing NULL for all > columns. Cool, thanks for confirming! If there are complains on the matter, let's of course tune it again if necessary before the release. There is still time for that. -- Michael
Attachment
On 2018-Dec-09, Tom Lane wrote: > Stephen Frost <sfrost@snowman.net> writes: > > * Tom Lane (tgl@sss.pgh.pa.us) wrote: > >> ... especially in code that's highly unlikely to break once written. > > > I don't entirely buy off on the argument that it's code that's 'highly > > unlikely to break once written' though- we do add new relkinds from time > > to time, for example. Perhaps we could have these functions run just > > once per relkind. > > Well, the relevant code is likely to be "if relkind is not x, y, or z, > then PG_RETURN_NULL". If we add a new relkind and forget to consider the > function, the outcome is a NULL result that perhaps should not have been > NULL ... but a test like this won't help us notice that. I just happened to come across the result of this rationale in pg_partition_tree() (an SRF) while developing a new related function, pg_partition_ancestors(), and find the resulting behavior rather absurd -- it returns one row with all NULL columns, rather than no rows. I think the sensible behavior would be to do SRF_RETURN_DONE() before stashing any rows to the output, so that we get an empty result set instead. alvherre=# select * from pg_partition_tree('information_schema.sequences'); relid | parentrelid | isleaf | level -------+-------------+--------+------- | | | (1 fila) -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Feb 27, 2019 at 03:48:08PM -0300, Alvaro Herrera wrote: > I just happened to come across the result of this rationale in > pg_partition_tree() (an SRF) while developing a new related function, > pg_partition_ancestors(), and find the resulting behavior rather absurd > -- it returns one row with all NULL columns, rather than no rows. I > think the sensible behavior would be to do SRF_RETURN_DONE() before > stashing any rows to the output, so that we get an empty result set > instead. Hmm. Going through the thread again NULL was decided to make the whole experience consistent, now by returning nothing we would get a behavior as consistent as when NULL is used in input, so point taken to tune the behavior for unsupported relkinds and undefined objects. Does the attached look fine to you? -- Michael
Attachment
Hi, On 2019/02/28 10:45, Michael Paquier wrote: > On Wed, Feb 27, 2019 at 03:48:08PM -0300, Alvaro Herrera wrote: >> I just happened to come across the result of this rationale in >> pg_partition_tree() (an SRF) while developing a new related function, >> pg_partition_ancestors(), and find the resulting behavior rather absurd >> -- it returns one row with all NULL columns, rather than no rows. I >> think the sensible behavior would be to do SRF_RETURN_DONE() before >> stashing any rows to the output, so that we get an empty result set >> instead. > > Hmm. Going through the thread again NULL was decided to make the > whole experience consistent, now by returning nothing we would get > a behavior as consistent as when NULL is used in input, so point taken > to tune the behavior for unsupported relkinds and undefined objects. > > Does the attached look fine to you? Reading the discussion, we just don't want to throw an "ERROR: unsupported relkind" error, to avoid, for example, aborting a query that's iteratively calling pg_partition_tree on all relations in a given set. Returning no rows at all seems like a better way of ignoring such relations. with rels as (select relname from pg_class where relnamespace = 'public'::regnamespace) select pg_partition_tree(relname::regclass) from rels; pg_partition_tree ──────────────────────────────────────── (pk1,pk,t,0) (pk,,f,0) (pk1,pk,t,1) (pk1_pkey,pk_pkey,t,0) (pk_pkey,,f,0) (pk1_pkey,pk_pkey,t,1) (another_pk1,another_pk,t,0) (another_pk,,f,0) (another_pk1,another_pk,t,1) (another_pk1_pkey,another_pk_pkey,t,0) (another_pk_pkey,,f,0) (another_pk1_pkey,another_pk_pkey,t,1) (13 rows) Now that Alvaro mentions it, I too think that returning no rows at all is better than 1 row filled with all NULLs, so +1. Thanks, Amit
On 2019-Feb-28, Michael Paquier wrote: > On Wed, Feb 27, 2019 at 03:48:08PM -0300, Alvaro Herrera wrote: > > I just happened to come across the result of this rationale in > > pg_partition_tree() (an SRF) while developing a new related function, > > pg_partition_ancestors(), and find the resulting behavior rather absurd > > -- it returns one row with all NULL columns, rather than no rows. I > > think the sensible behavior would be to do SRF_RETURN_DONE() before > > stashing any rows to the output, so that we get an empty result set > > instead. > > Hmm. Going through the thread again NULL was decided to make the > whole experience consistent, now by returning nothing we would get > a behavior as consistent as when NULL is used in input, so point taken > to tune the behavior for unsupported relkinds and undefined objects. Right, thanks. > Does the attached look fine to you? Yeah, looks good, please push. What about legacy inheritance? I see that pg_partition_tree handles that case perfectly well -- it seems to return the complete hierarchy rooted at the given relation. However, it seems odd that it works at all, don't you think? Consider this: create table t1 (a int); create table t11 () inherits (t1); create table t2 (b int); create table t111() inherits (t1, t2); alvherre=# select * from pg_partition_tree('t1'); relid | parentrelid | isleaf | level -------+-------------+--------+------- t1 | t | t | 0 t11 | t1 | t | 1 t111 | t1 | t | 1 (3 filas) OK so far... but look at t2's tree: alvherre=# select * from pg_partition_tree('t2'); relid | parentrelid | isleaf | level -------+-------------+--------+------- t2 | t | t | 0 t111 | t1 | t | 2 I think this one is just weird -- t1 is not listed as "relid" anywhere, and why does t111 has level=2? I would opt for returning the empty set for legacy inheritance too. More generally, I think we should return empty for anything that's either not RELKIND_PARTITIONED_TABLE or has relispartition set. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Feb 28, 2019 at 04:32:03PM -0300, Alvaro Herrera wrote: > Yeah, looks good, please push. Done for this part. > I would opt for returning the empty set for legacy inheritance too. > > More generally, I think we should return empty for anything that's > either not RELKIND_PARTITIONED_TABLE or has relispartition set. I think that one option is to make the function return only the table itself if it is not a partitioned table, which would be more consistent with what pg_partition_root() does. What I am writing next sounds perhaps a bit fancy, but in my opinion a normal table is itself a partition tree, made of one single member: itself. -- Michael
Attachment
Hi, On 2019/03/01 9:22, Michael Paquier wrote: > On Thu, Feb 28, 2019 at 04:32:03PM -0300, Alvaro Herrera wrote: >> Yeah, looks good, please push. > > Done for this part. > >> I would opt for returning the empty set for legacy inheritance too. >> >> More generally, I think we should return empty for anything that's >> either not RELKIND_PARTITIONED_TABLE or has relispartition set. > > I think that one option is to make the function return only the table > itself if it is not a partitioned table, which would be more > consistent with what pg_partition_root() does. > > What I am writing next sounds perhaps a bit fancy, but in my opinion a > normal table is itself a partition tree, made of one single member: > itself. That's what we discussed, but it seems that we ended up allowing regular standalone tables (possibly in inheritance trees) only because it *appeared* to work. Alvaro already pointed out what appears to be a bug in how we compute the value of level. Instead of trying to fix it, I agree we should just disallow tables that are not a partitioned table/index or a partition (relispartition). Maybe there won't be any use cases, so we should change that while we still can. So, maybe change the check in check_rel_can_be_partition() as follows: relkind = get_rel_relkind(relid); relispartition = get_rel_relispartition(relid); /* Only allow relation types that can appear in partition trees. */ if (!relispartition && relkind != RELKIND_PARTITIONED_TABLE && relkind != RELKIND_PARTITIONED_INDEX) return false; Thanks, Amit
Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes: > On 2019/03/01 9:22, Michael Paquier wrote: >> What I am writing next sounds perhaps a bit fancy, but in my opinion a >> normal table is itself a partition tree, made of one single member: >> itself. > That's what we discussed, but it seems that we ended up allowing regular > standalone tables (possibly in inheritance trees) only because it > *appeared* to work. Alvaro already pointed out what appears to be a bug > in how we compute the value of level. Instead of trying to fix it, I > agree we should just disallow tables that are not a partitioned > table/index or a partition (relispartition). FWIW, I don't agree with Michael's suggestion above. A plain table is significantly different from a partitioned table with no children: you can store rows in the former but not the latter, and you can add partitions to the latter but not the former. So conflating the two doesn't seem likely to lead to any good outcome. But, having said that, we've learned that it's generally bad for catalog-query functions to fail outright just because they're pointed at the wrong kind of catalog object. So I think that what we want here is for pg_partition_tree to return NULL or an empty set or some such for a plain table, while its output for a childless partitioned table should be visibly different from that. I'm not wedded to details beyond that idea. regards, tom lane
On Thu, Feb 28, 2019 at 11:50:16PM -0500, Tom Lane wrote: > FWIW, I don't agree with Michael's suggestion above. A plain table is > significantly different from a partitioned table with no children: > you can store rows in the former but not the latter, and you can add > partitions to the latter but not the former. So conflating the two > doesn't seem likely to lead to any good outcome. Okay, so the opinion moves into this direction. What would be needed is just something like the attached patch then based on Amit's suggestion? The routine names and comments looked fine to me so I have not touched the surroundings, and comments are welcome. > But, having said that, we've learned that it's generally bad for > catalog-query functions to fail outright just because they're pointed > at the wrong kind of catalog object. So I think that what we want here > is for pg_partition_tree to return NULL or an empty set or some such > for a plain table, while its output for a childless partitioned table > should be visibly different from that. I'm not wedded to details > beyond that idea. Yep, that's the intention since cc53123. I won't come back to return an ERROR in any case. Here is what the patch gives for childless partitions FWIW: =# CREATE TABLE ptif_test (a int, b int) PARTITION BY range (a); CREATE TABLE =# SELECT * FROM pg_partition_tree('ptif_test'); relid | parentrelid | isleaf | level -----------+-------------+--------+------- ptif_test | null | f | 0 (1 row) -- Michael
Attachment
Michael Paquier <michael@paquier.xyz> writes: > On Thu, Feb 28, 2019 at 11:50:16PM -0500, Tom Lane wrote: >> But, having said that, we've learned that it's generally bad for >> catalog-query functions to fail outright just because they're pointed >> at the wrong kind of catalog object. So I think that what we want here >> is for pg_partition_tree to return NULL or an empty set or some such >> for a plain table, while its output for a childless partitioned table >> should be visibly different from that. I'm not wedded to details >> beyond that idea. > Yep, that's the intention since cc53123. I won't come back to return > an ERROR in any case. Here is what the patch gives for childless > partitions FWIW: > =# CREATE TABLE ptif_test (a int, b int) PARTITION BY range (a); > CREATE TABLE > =# SELECT * FROM pg_partition_tree('ptif_test'); > relid | parentrelid | isleaf | level > -----------+-------------+--------+------- > ptif_test | null | f | 0 > (1 row) Right, while you'd get zero rows out for a non-partitioned table. WFM. regards, tom lane
On Fri, Mar 01, 2019 at 11:38:20AM -0500, Tom Lane wrote: > Right, while you'd get zero rows out for a non-partitioned table. > WFM. Exactly. I have committed a patch doing exactly that, and I have added test cases with a partitioned table and a partitioned index which have no partitions. -- Michael
Attachment
On 2019/03/02 18:21, Michael Paquier wrote: > On Fri, Mar 01, 2019 at 11:38:20AM -0500, Tom Lane wrote: >> Right, while you'd get zero rows out for a non-partitioned table. >> WFM. > > Exactly. I have committed a patch doing exactly that, and I have > added test cases with a partitioned table and a partitioned index > which have no partitions. Thanks for committing and adding me as an author. Regards, Amit
On Mon, Mar 04, 2019 at 10:44:10AM +0900, Amit Langote wrote: > Thanks for committing and adding me as an author. Sure. A good portion of the changes suggested on the backend was mainly yours, so that looked right to me. -- Michael
Attachment
On 2019-Feb-28, Alvaro Herrera wrote: > What about legacy inheritance? I see that pg_partition_tree handles > that case perfectly well -- it seems to return the complete hierarchy > rooted at the given relation. However, it seems odd that it works at > all, don't you think? Consider this: > [...] > I would opt for returning the empty set for legacy inheritance too. I added tests to immortalize the current behavior for legacy inheritance relations too. Thanks! -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Mar 04, 2019 at 03:56:00PM -0300, Alvaro Herrera wrote: > I added tests to immortalize the current behavior for legacy inheritance > relations too. Thanks! Makes sense, thanks. -- Michael