Re: Avoid lost result of recursion (src/backend/optimizer/util/inherit.c)

On Fri, Dec 23, 2022 at 9:10 PM David Rowley wrote > On Fri, 23 Dec 2022 at 17:04, Richard Guo wrote: > > Thanks for the test! I looked at this and found that with WCO > > constraints we can also hit the buggy code. Based on David's test case, > > I came up with the following in the morning. >

Re: Avoid lost result of recursion (src/backend/optimizer/util/inherit.c)

Em sex., 23 de dez. de 2022 às 09:10, David Rowley escreveu: > > I've now pushed your fix plus that test. > Thank you all for getting involved to resolve this. regards, Ranier Vilela

Re: Avoid lost result of recursion (src/backend/optimizer/util/inherit.c)

On Fri, 23 Dec 2022 at 17:04, Richard Guo wrote: > Thanks for the test! I looked at this and found that with WCO > constraints we can also hit the buggy code. Based on David's test case, > I came up with the following in the morning. I ended up going with a WCO option test in the end. I

Re: Avoid lost result of recursion (src/backend/optimizer/util/inherit.c)

On Fri, Dec 23, 2022 at 1:04 PM Richard Guo wrote: > On Fri, Dec 23, 2022 at 11:22 AM Amit Langote wrote: >> Attached shows a test case I was able to come up with that I can see >> is broken by a61b1f74 though passes after applying Richard's patch. >> What's broken is that deparseUpdateSql()

Re: Avoid lost result of recursion (src/backend/optimizer/util/inherit.c)

On Fri, Dec 23, 2022 at 11:22 AM Amit Langote wrote: > Attached shows a test case I was able to come up with that I can see > is broken by a61b1f74 though passes after applying Richard's patch. > What's broken is that deparseUpdateSql() outputs a remote UPDATE > statement with the wrong SET

Re: Avoid lost result of recursion (src/backend/optimizer/util/inherit.c)

On Fri, 23 Dec 2022 at 16:22, Amit Langote wrote: > Attached shows a test case I was able to come up with that I can see > is broken by a61b1f74 though passes after applying Richard's patch. Thanks for the test case. I'll look at this now. +UPDATE rootp SET b = b || 'd' RETURNING a, b, c, d; +

Re: Avoid lost result of recursion (src/backend/optimizer/util/inherit.c)

On Fri, Dec 23, 2022 at 12:22 PM Amit Langote wrote: > Attached shows a test case I was able to come up with that I can see > is broken by a61b1f74 though passes after applying Richard's patch. BTW, I couldn't help but notice in the output of the test case I wrote that a generated column of a

Re: Avoid lost result of recursion (src/backend/optimizer/util/inherit.c)

Hi, Thanks everyone for noticing this. It is indeed very obviously broken. :( On Fri, Dec 23, 2022 at 11:26 AM David Rowley wrote: > On Fri, 23 Dec 2022 at 15:21, Richard Guo wrote: > > > > On Thu, Dec 22, 2022 at 5:22 PM David Rowley wrote: > >> I still think we should have a test to ensure

Re: Avoid lost result of recursion (src/backend/optimizer/util/inherit.c)

On Fri, 23 Dec 2022 at 15:21, Richard Guo wrote: > > On Thu, Dec 22, 2022 at 5:22 PM David Rowley wrote: >> I still think we should have a test to ensure this is actually >> working. Do you want to write one? > > > I agree that we should have a test. According to the code coverage > report, the

Re: Avoid lost result of recursion (src/backend/optimizer/util/inherit.c)

On Thu, Dec 22, 2022 at 5:22 PM David Rowley wrote: > On Thu, 22 Dec 2022 at 21:18, Richard Guo wrote: > > My best guess is that this function is intended to share the same code > > pattern as in adjust_appendrel_attrs_multilevel. The recursion is > > needed as 'rel' can be more than one

Re: Avoid lost result of recursion (src/backend/optimizer/util/inherit.c)

On Thu, 22 Dec 2022 at 21:18, Richard Guo wrote: > My best guess is that this function is intended to share the same code > pattern as in adjust_appendrel_attrs_multilevel. The recursion is > needed as 'rel' can be more than one inheritance level below the top > parent. I think we can keep the

Re: Avoid lost result of recursion (src/backend/optimizer/util/inherit.c)

On Wed, Dec 21, 2022 at 9:45 AM David Rowley wrote: > Also, I think it might be better to take the opportunity to rewrite > the function to not use recursion. I don't quite see the need for it > here and it looks like that might have helped contribute to the > reported issue. Can't we just

Re: Avoid lost result of recursion (src/backend/optimizer/util/inherit.c)

Thanks David, for looking at this. Em ter., 20 de dez. de 2022 às 22:45, David Rowley escreveu: > On Wed, 21 Dec 2022 at 13:15, Ranier Vilela wrote: > > IMO, I think that commit a61b1f7, has an oversight. > > Currently is losing the result of recursion of function >

Re: Avoid lost result of recursion (src/backend/optimizer/util/inherit.c)

On Wed, 21 Dec 2022 at 13:15, Ranier Vilela wrote: > IMO, I think that commit a61b1f7, has an oversight. > Currently is losing the result of recursion of function > translate_col_privs_multilevel. > > Once the variable result (Bitmapset pointer) is reassigned. > > Without a test case for this

Avoid lost result of recursion (src/backend/optimizer/util/inherit.c)

Hi. IMO, I think that commit a61b1f7 , has an oversight. Currently is losing the result of recursion of function translate_col_privs_multilevel. Once the variable result (Bitmapset pointer) is reassigned.