Thread: Commitfest 2022-03 Patch Triage Part 1a.i
Last November Daniel Gustafsson did a patch triage. It took him three emails to get through the patches in the commitfest back then. Since then we've had the November and the January commitfests so I was interested to see how many of these patches had advanced.... I'm only part way through the first email but so far only two patches have changed status -- and both to "Returned with feedback" :( So I'm going to post updates but I'm going to break it up into smaller batches because otherwise it'll take me a month before I post anything. > 1608: schema variables, LET command > =================================== > After 18 CF's and two very long threads it seems to be nearing completion > judging by Tomas' review. There is an ask in that review for a second pass > over the docs by a native speaker, any takers? Patch has a new name, "session variables, LET command" There's been a *lot* of work on this patch so I'm loath to bump it. The last review was from Julien Rouhaud which had mostly code style comments but it had one particular concern about using xact callback in core and about EOX cleanup. Pavel, do you have a plan to improve this or are you looking for suggestions from someone about how you should solve this problem? > 1741: Index Skip Scan > ===================== > An often requested feature which has proven hard to reach consensus on an > implementation for. The thread(s) have stalled since May, is there any hope of > taking this further? Where do we go from here with this patch? "Often requested indeed! I would love to be able to stop explaining to people that Postgres can't handle this case well. It seems there are multiple patch variants around and suggestions from Heikki and Peter G about fundamental interface choices. It would be nice to have a good summary from someone who is involved about what's actually left unresolved. > 1712: Remove self join on a unique column > ========================================= > This has moved from "don't write bad queries" to "maybe we should do something > about this". It seems like there is concensus that it can be worth paying the > added planner cost for this, especially if gated by a GUC to keep it out of > installations where it doesn't apply. The regression on large join queries > hasn't been benchmarked it seems (or I missed it), but the patch seems to have > matured and be quite ready. Any takers on getting it across the finish line? There hasn't been any review since the v29 patch was posted in July. That said, to my eye it seemed like pretty basic functionality errors were being corrected quite late. All the bugs got patch updates quickly but does this have enough tests to be sure it's working right? The only real objection was about whether the planning time justified the gains since the gains are small. But I think they were resolved by making the optimization optional. Do we have consensus that that resolved the issue or do we still need benchmarks showing the planning time hit is reasonable? > 2161: standby recovery fails when re-replaying due to missing directory which > was removed in previous replay. > ============================================================================= > Tom and Robert seem to be in agreement that parts of this patchset are good, > and that some parts are not. The thread has stalled and the patch no longer > apply, so unless someone feels like picking up the good pieces this seems like > a contender to close for now. Tom's feedback seems to have been acted on last November. And the last update in January was that it was passing CI now. Is this ready to commit now? > 2138: Incremental Materialized View Maintenance > =============================================== > The size of the > patchset and the length of the thread make it hard to gauge just far away it > is, maybe the author or a reviewer can summarize the current state and outline > what is left for it to be committable. There is an updated patch set as of February but I have the same difficulty wrapping my head around the amount of info here. Is this one really likely to be commitable in 15? If not I think we should move this to 16 now and concentrate on patches that will be commitable in this release. > 2218: Implement INSERT SET syntax > ================================= > The author has kept this patch updated, and has seemingly updated according to > the review comments. Tom: do you have any opinions on whether the updated > version addresses your concerns wrt the SELECT rewrite? I don't see any discussion implying that Tom's concerns were met. I'm not exactly clear why Tom's concerns are real problems though -- wouldn't it be a *good* thing if we have a more expressive syntax? But that's definitely what needs to be resolved before it can move ahead. So unless there's objections I'm going to update this to "waiting on author". -- greg
Hi, On Tue, Mar 01, 2022 at 11:16:36AM -0500, Greg Stark wrote: > > > 1608: schema variables, LET command > > =================================== > > After 18 CF's and two very long threads it seems to be nearing completion > > judging by Tomas' review. There is an ask in that review for a second pass > > over the docs by a native speaker, any takers? > > Patch has a new name, "session variables, LET command" > > There's been a *lot* of work on this patch so I'm loath to bump it. > The last review was from Julien Rouhaud which had mostly code style > comments but it had one particular concern about using xact callback > in core and about EOX cleanup. > > Pavel, do you have a plan to improve this or are you looking for > suggestions from someone about how you should solve this problem? There has indeed been a lot of work done on the patch during the last commit fest, and Pavel always fixed all the reported issues promptly, which is why apart from the EOX cleanup thing most of the last review was minor problems. Pavel sent a new version today that address the EOX problem (and everything else) so I'm now the one that needs to do my reviewer job (which I already started). I didn't get through all the changes yet but as far as I can see the patch is in a very good shape. I'm quite optimistic about this patch being ready for committer very soon, so I think it would be good to keep it and seeif we can get it committed in pg 15. Note that some committers already showed interest in the patch.
Can I suggest to copy the patch authors on bulk emails like these ? (Obviously, an extended discussion about a particular patch should happen on its original thread, but that's not what this is about). -- Justin
As Justin suggested I CC the authors from these patches I'm adding them here. Some of the patches have multiple "Authors" listed in the commitfest which may just be people who posted updated patches so I may have added more people than necessary. [If you received two copies of this do not reply to the first one or you'll get bounces. Hopefully I've cleaned the list now but we'll see...]
On Tue, 1 Mar 2022 at 14:59, Greg Stark <stark@mit.edu> wrote: > > As Justin suggested I CC the authors from these patches I'm adding > them here. Some of the patches have multiple "Authors" listed in the > commitfest which may just be people who posted updated patches so I > may have added more people than necessary. > [If you received two copies of this do not reply to the first one or > you'll get bounces. Hopefully I've cleaned the list now but we'll > see...] Nope. One more address to remove from the CC. -- greg
> On Tue, Mar 01, 2022 at 11:16:36AM -0500, Greg Stark wrote: > Last November Daniel Gustafsson did a patch triage. It took him three > emails to get through the patches in the commitfest back then. Since > then we've had the November and the January commitfests so I was > interested to see how many of these patches had advanced.... > > I'm only part way through the first email but so far only two patches > have changed status -- and both to "Returned with feedback" :( > > So I'm going to post updates but I'm going to break it up into smaller > batches because otherwise it'll take me a month before I post > anything. Thanks for being proactive! > > 1741: Index Skip Scan > > ===================== > > An often requested feature which has proven hard to reach consensus on an > > implementation for. The thread(s) have stalled since May, is there any hope of > > taking this further? Where do we go from here with this patch? > > "Often requested indeed! I would love to be able to stop explaining to > people that Postgres can't handle this case well. > > It seems there are multiple patch variants around and suggestions from > Heikki and Peter G about fundamental interface choices. It would be > nice to have a good summary from someone who is involved about what's > actually left unresolved. I'm going to leave a summary for this one here, if you don't mind. I believe the design commentary from Heikki about using index_rescan was more or less answered by Thomas, and having no follow up on that I'm assuming it was convincing enough. Peter G most recent suggestion about MDAM approach was interesting, but very general, not sure what to make of it in absence of any feedback on follow-up questions/proposed experimental changes. On top of that a correlated patch [1] that supposed to get some improvements for this feature on the planner side didn't get much feedback either. The idea is that the feature could be done in much better way, but the alternative proposal is still not there and I think doesn't even have a CF item. The current state of things is that I've managed to prepare much smaller and less invasive standalone version of the patch for review, leaving most questionable parts aside as optional. Overall it seems that the common agreement about the patch is "the design could be better", but no one have yet articulated in which way, or formulated what are the current issues. Having being through 19 CF the common ground for folks, who were involved into it, is that with no further feedback the CF item could be closed. Sad but true :( [1]: https://commitfest.postgresql.org/37/2433/
> On 1 Mar 2022, at 17:16, Greg Stark <stark@mit.edu> wrote: > Last November Daniel Gustafsson did a patch triage. It took him three > emails to get through the patches in the commitfest back then. It should be noted that I only powered through the patches that had been in 3+ commitfests at the time.. > Since then we've had the November and the January commitfests so I was > interested to see how many of these patches had advanced.... ..so there are new patches now that have crossed the (admittedly arbitrarily chosen) breakpoint of 3+ CF's. > So I'm going to post updates but I'm going to break it up into smaller > batches because otherwise it'll take me a month before I post > anything. Thanks for picking it up and continuing with recent developments. Let me know if you want a hand in triaging patchsets. -- Daniel Gustafsson https://vmware.com/
On Wed, 2 Mar 2022 at 07:12, Daniel Gustafsson <daniel@yesql.se> wrote: > Thanks for picking it up and continuing with recent developments. Let me know > if you want a hand in triaging patchsets. While I have the time there may be patches I may need help coming to the right conclusions about what actions to take. I think the main thing I can do to help is to take patches that have no chance of making this release and taking them off our collective plates. -- Hopefully after they've received feedback but as this is the last commitfest of the release that's a secondary concern. But I'm unclear exactly what the consequences in the commitfest app are of specific state changes. As I understand it there are basically two alternatives: 1) Returned with feedback -- does this make it harder for an author to resume work release? Can they simply reactivate the CF entry or do they need to start a new one and then lose history in the app? 2) Moved to next commitfest -- this seems to just drag the pain on. Then it has to get triaged again next commitfest and if it's actually stalled (or progressing fine without needing feedback) that's just extra work for nothing. Do I have this right? What is the right state to put a patch in that means "this patch doesn't need to be triaged again unless the author actually feels progress has been made and needs new feedback or thinks its committable"? -- greg
Greg Stark <stark@mit.edu> writes:
> Do I have this right? What is the right state to put a patch in that
> means "this patch doesn't need to be triaged again unless the author
> actually feels progress has been made and needs new feedback or thinks
> its committable"?
But that's not really the goal, is it? ISTM what you want to do is
identify patches that we're not going to try to get into v15, and
then push them out to the next CF so that we don't spend more time
on them this month. But that determination should not preclude them
from being looked at on the normal basis once the next CF arrives.
So I'd say just push them forward with status "Needs review" or
"Waiting on author", whichever seems more appropriate.
If a patch seems to have stalled to the point where neither of
those statuses is appropriate, then closing it RWF would be the
thing to do; but that's not special to the last-CF situation.
regards, tom lane
On Wed, Mar 02, 2022 at 11:58:28AM -0500, Greg Stark wrote: > > But I'm unclear exactly what the consequences in the commitfest app > are of specific state changes. As I understand it there are basically > two alternatives: > > 1) Returned with feedback -- does this make it harder for an author to > resume work release? Can they simply reactivate the CF entry or do > they need to start a new one and then lose history in the app? As far as I know they would need to create a new entry, and thus lose the history. > 2) Moved to next commitfest -- this seems to just drag the pain on. > Then it has to get triaged again next commitfest and if it's actually > stalled (or progressing fine without needing feedback) that's just > extra work for nothing. > > Do I have this right? What is the right state to put a patch in that > means "this patch doesn't need to be triaged again unless the author > actually feels progress has been made and needs new feedback or thinks > its committable"? I don't think that 2) means having to triage again. If a patch gets moved to the next commitfest now, then clearly it's not ready and should be also switched to Waiting on Author. In the next commitfest, if the author doesn't address the problems raised during review the patch will still be in Waiting for Author, and the only needed triaging would be to close as Return With Feedback patches that looks abandoned. For now the arbitrary "abandoned" definition is usually "patch in Waiting on Author for at least 2 weeks at the end of the commitfest with no sign of activity from the author".
On Tue, 1 Mar 2022 13:39:45 -0500 Greg Stark <stark@mit.edu> wrote: > As Justin suggested I CC the authors from these patches I'm adding > them here. Some of the patches have multiple "Authors" listed in the > commitfest which may just be people who posted updated patches so I > may have added more people than necessary. > > On Tue, 1 Mar 2022 at 11:16, Greg Stark <stark@mit.edu> wrote: > > > > Last November Daniel Gustafsson did a patch triage. It took him three > > emails to get through the patches in the commitfest back then. Since > > then we've had the November and the January commitfests so I was > > interested to see how many of these patches had advanced.... > > > > I'm only part way through the first email but so far only two patches > > have changed status -- and both to "Returned with feedback" :( > > > > So I'm going to post updates but I'm going to break it up into smaller > > batches because otherwise it'll take me a month before I post > > anything. > > > > > > > > > 1608: schema variables, LET command > > > =================================== > > > After 18 CF's and two very long threads it seems to be nearing completion > > > judging by Tomas' review. There is an ask in that review for a second pass > > > over the docs by a native speaker, any takers? > > > > Patch has a new name, "session variables, LET command" > > > > There's been a *lot* of work on this patch so I'm loath to bump it. > > The last review was from Julien Rouhaud which had mostly code style > > comments but it had one particular concern about using xact callback > > in core and about EOX cleanup. > > > > Pavel, do you have a plan to improve this or are you looking for > > suggestions from someone about how you should solve this problem? > > > > > 1741: Index Skip Scan > > > ===================== > > > An often requested feature which has proven hard to reach consensus on an > > > implementation for. The thread(s) have stalled since May, is there any hope of > > > taking this further? Where do we go from here with this patch? > > > > "Often requested indeed! I would love to be able to stop explaining to > > people that Postgres can't handle this case well. > > > > It seems there are multiple patch variants around and suggestions from > > Heikki and Peter G about fundamental interface choices. It would be > > nice to have a good summary from someone who is involved about what's > > actually left unresolved. > > > > > > > 1712: Remove self join on a unique column > > > ========================================= > > > This has moved from "don't write bad queries" to "maybe we should do something > > > about this". It seems like there is concensus that it can be worth paying the > > > added planner cost for this, especially if gated by a GUC to keep it out of > > > installations where it doesn't apply. The regression on large join queries > > > hasn't been benchmarked it seems (or I missed it), but the patch seems to have > > > matured and be quite ready. Any takers on getting it across the finish line? > > > > There hasn't been any review since the v29 patch was posted in July. > > That said, to my eye it seemed like pretty basic functionality errors > > were being corrected quite late. All the bugs got patch updates > > quickly but does this have enough tests to be sure it's working right? > > > > The only real objection was about whether the planning time justified > > the gains since the gains are small. But I think they were resolved by > > making the optimization optional. Do we have consensus that that > > resolved the issue or do we still need benchmarks showing the planning > > time hit is reasonable? > > > > > 2161: standby recovery fails when re-replaying due to missing directory which > > > was removed in previous replay. > > > ============================================================================= > > > Tom and Robert seem to be in agreement that parts of this patchset are good, > > > and that some parts are not. The thread has stalled and the patch no longer > > > apply, so unless someone feels like picking up the good pieces this seems like > > > a contender to close for now. > > > > Tom's feedback seems to have been acted on last November. And the last > > update in January was that it was passing CI now. Is this ready to > > commit now? > > > > > > > 2138: Incremental Materialized View Maintenance > > > =============================================== > > > The size of the > > > patchset and the length of the thread make it hard to gauge just far away it > > > is, maybe the author or a reviewer can summarize the current state and outline > > > what is left for it to be committable. > > > > There is an updated patch set as of February but I have the same > > difficulty wrapping my head around the amount of info here. > > > > Is this one really likely to be commitable in 15? If not I think we > > should move this to 16 now and concentrate on patches that will be > > commitable in this release. I think this patch set needs more reviews to be commitable in 15, so I returned the target version to blank. I'll change it to 16 later. > > > > > 2218: Implement INSERT SET syntax > > > ================================= > > > The author has kept this patch updated, and has seemingly updated according to > > > the review comments. Tom: do you have any opinions on whether the updated > > > version addresses your concerns wrt the SELECT rewrite? > > > > I don't see any discussion implying that Tom's concerns were met. I'm > > not exactly clear why Tom's concerns are real problems though -- > > wouldn't it be a *good* thing if we have a more expressive syntax? But > > that's definitely what needs to be resolved before it can move ahead. > > > > So unless there's objections I'm going to update this to "waiting on author". > > > > -- > > greg > > > > -- > greg -- Yugo NAGATA <nagata@sraoss.co.jp>
> On 3 Mar 2022, at 10:11, Yugo NAGATA <nagata@sraoss.co.jp> wrote: > I think this patch set needs more reviews to be commitable in 15, so I > returned the target version to blank. I'll change it to 16 later. I've added 16 as a target version in the CF app now. -- Daniel Gustafsson https://vmware.com/