Обсуждение: Commitfest 2024-01 first week update
Hi, Here's a quick status report after the first week: Status summary: Needs review: 238. Waiting on Author: 44. Ready for Committer: 27. Committed: 36. Moved to next CF: 1. Withdrawn: 2. Returned with Feedback: 3. Rejected: 1. Total: 352. Here is a list of "Needs review" entries for which there has not been much communication on the thread and needs help in proceeding further. Hackers please pick one of these and review/share your suggestions, that will be helpful in taking it forward: Fixes for non-atomic read of read of control file on ext4 + ntfs | Thomas Munro Recovering from detoast-related catcache invalidations | Tom Lane Dump-restore loosing 'attnotnull' bit for DEFERRABLE PRIMARY KEY column(s) | Amul sul pgbench: allow to cancel queries during benchmark | Yugo Nagata vacuumdb/clusterdb/reindexdb: allow specifying objects to process in all databases | Nathan Bossart Add additional extended protocol commands to psql: \parse and \bindx | Anthonin Bonnefoy Function to log backtrace of postgres processes | Vignesh C/Bharath Rupireddy Check consistency of GUC defaults between .sample.conf and pg_settings.boot_val | Nathan Bossart archive modules loose ends | Nathan Bossart make pg_ctl more friendly | Crisp Lee locked reads for atomics | Nathan Bossart common signal handler protection | Nathan Bossart Reuse child_relids in try_partitionwise_join | Ashutosh Bapat Here is a list of "Ready for Committer" entries for which there has not been much communication on the thread and needs help in proceeding further. If any of the committers has some time to spare, please help us on these: Skip hidden files in serverside utilities | Daniel Gustafsson Unlinking Parallel Hash Join inner batch files sooner | Thomas Munro pg_basebackup: mention that spread checkpoints are the default | Michael Banck Evaluate arguments of correlated SubPlans in the referencing ExprState | Andres Freund Cross-database SERIALIZABLE safe snapshots | Thomas Munro Add support function for range containment operators | Kim Johan Andersson Refactor pipe_read_line as a more generic interface for reading arbitrary strings off a pipe | Daniel Gustafsson Use atomic ops for unlogged LSN | John Morris functions to compute size of schemas/AMs (and maybe \dn++ and \dA++) | Justin Pryzby Improve Boolean Predicate JSON Path Docs | David Wheeler Add PQsendPipelineSync() to libpq | Anton Kirilov Trigger violates foreign key constraint | Laurenz Albe If you have submitted a patch and it's in "Waiting for author" state, please get the patch to "Needs review" state soon if you can, as that's where people are most likely to be looking for things to review. I have pinged many threads that are in "Ready for Committer", "Needs review" state and don't apply, compile warning-free, or pass check-world. I have also updated the status of the patches which were not in the correct state. I will also be pinging the patch owners to review a few patches who have submitted one or more patches but have not picked any of the patches for review. Regards, Vignesh
On Mon, 8 Jan 2024 at 07:22, vignesh C <vignesh21@gmail.com> wrote: > Here is a list of "Needs review" entries for which there has not been > much communication on the thread and needs help in proceeding further. Thank you for creating these lists. It's definitely helpful to see what to focus my reviewing effort on. I noticed they are missing one of my commitfest entries though: Add non-blocking version of PQcancel | Jelte Fennema-Nio imho this patch is pretty much finished, but it has only received English spelling feedback in the last 8 months (I addressed the feedback).
On Mon, 8 Jan 2024 at 22:50, Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > > On Mon, 8 Jan 2024 at 07:22, vignesh C <vignesh21@gmail.com> wrote: > > Here is a list of "Needs review" entries for which there has not been > > much communication on the thread and needs help in proceeding further. > > Thank you for creating these lists. It's definitely helpful to see > what to focus my reviewing effort on. I noticed they are missing one > of my commitfest entries though: This is just one list for this week, I will be focussing on a different set of patches for review in the next week. > Add non-blocking version of PQcancel | Jelte Fennema-Nio > > imho this patch is pretty much finished, but it has only received > English spelling feedback in the last 8 months (I addressed the > feedback). Thanks, One of us will have a look at this patch. Regards, Vignesh
Hi, I think we need to be more aggressive about marking things returned with feedback when they don't get updated. If a patch is waiting for reviews for a long time, well, that's one thing. Maybe we eventually close it due to lack of interest in reviewing it, but that should be done cautiously, as it will understandably piss people off. But I regularly find patches in the CommitFest which have been waiting on author for multiple commitfests and are just repeatedly moved forward. That's crazy to me. It makes the CommitFest application fill up with junk that obscures what actually needs to be dealt with. ...Robert
On Wed, 10 Jan 2024 at 03:48, Robert Haas <robertmhaas@gmail.com> wrote: > > Hi, > > I think we need to be more aggressive about marking things returned > with feedback when they don't get updated. If a patch is waiting for > reviews for a long time, well, that's one thing. Maybe we eventually > close it due to lack of interest in reviewing it, but that should be > done cautiously, as it will understandably piss people off. But I > regularly find patches in the CommitFest which have been waiting on > author for multiple commitfests and are just repeatedly moved forward. > That's crazy to me. It makes the CommitFest application fill up with > junk that obscures what actually needs to be dealt with. I also noticed that many patches get carried forward to the next commitfest. I will start highlighting these kinds of patches to the author, request them to act upon the patches and return the patch if nothing is done in this commitfest. I'm also working on the needs review patches, I have already withdrawn one of my patches yesterday at [1] which got no response for a long time. I also got one on my colleagues's patch to be withdrawn by requesting him internally at [2]. I will be posting these kinds of patches in my weekly update. It will be good if one of the senior folks reply whether to take it forward or not, that will help me in taking a decision quickly on what to do with these patches. One kind suggestion for all the patches which have been inactive for a long time is for the author to re-assess the patch once again and act on the entry voluntarily which will help the committers and reviewers in spending time on the real patch which requires focus. [1] - https://www.postgresql.org/message-id/CALDaNm1AQZYgT0tALRrkvpP1Q%2B8%2Be7vkGCUjQ-jim1C0q3e%3DzA%40mail.gmail.com [2] - https://www.postgresql.org/message-id/CAHut+PtBX9S146Zq1CQUQQJ3n-P3ZSV4w9AHfC-LDsX5T5uT9w@mail.gmail.com Regards, Vignesh
> On 9 Jan 2024, at 23:18, Robert Haas <robertmhaas@gmail.com> wrote: > I think we need to be more aggressive about marking things returned > with feedback when they don't get updated. I very much agree. Having marked quite a lot of patches as RwF when being CFM I can attest that it gets very little off-list pushback or angry emails. While it does happen, the overwhelming majority of responses are understanding and positive, so no CFM should be worried about "being the bad guy". -- Daniel Gustafsson
On 2024-Jan-10, Daniel Gustafsson wrote: > > On 9 Jan 2024, at 23:18, Robert Haas <robertmhaas@gmail.com> wrote: > > > I think we need to be more aggressive about marking things returned > > with feedback when they don't get updated. > > I very much agree. Having marked quite a lot of patches as RwF when being CFM > I can attest that it gets very little off-list pushback or angry emails. While > it does happen, the overwhelming majority of responses are understanding and > positive, so no CFM should be worried about "being the bad guy". I like this idea very much -- return patches when the author does not respond AFTER receiving feedback or the patch rotting. However, this time around I saw that a bunch of patches were returned or threatened to be returned JUST BECAUSE nobody had replied to the thread, with a justification like "you need to generate more interest in your patch". This is a TERRIBLE idea, and there's one reason why creating a new commitfest entry in the following commitfest is no good: At the FOSDEM developer meeting, we do a run of CF patch triage, where we check the topmost patches in order of number-of-commitfests. If you return an old patch and a new CF entry is created, this number is reset, and we could quite possibly fail to detect some very old patch because of this. At times, the attention a patch gets during the CF triage is sufficient to get the patch moving forward after long inactivity, so this is not academic. Case in point: [1]. So by all means let's return patches that rot or fail to get updated per feedback. But DO NOT return patches because of inactivity. [1] https://postgr.es/m/202402011550.sfszd46247zi@alvherre.pgsql -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "Las cosas son buenas o malas segun las hace nuestra opinión" (Lisias)
On Sun, 4 Feb 2024 at 14:32, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2024-Jan-10, Daniel Gustafsson wrote: > > > > On 9 Jan 2024, at 23:18, Robert Haas <robertmhaas@gmail.com> wrote: > > > > > I think we need to be more aggressive about marking things returned > > > with feedback when they don't get updated. > > > > I very much agree. Having marked quite a lot of patches as RwF when being CFM > > I can attest that it gets very little off-list pushback or angry emails. While > > it does happen, the overwhelming majority of responses are understanding and > > positive, so no CFM should be worried about "being the bad guy". > > I like this idea very much -- return patches when the author does not > respond AFTER receiving feedback or the patch rotting. > > However, this time around I saw that a bunch of patches were returned or > threatened to be returned JUST BECAUSE nobody had replied to the thread, > with a justification like "you need to generate more interest in your > patch". This is a TERRIBLE idea, and there's one reason why creating a > new commitfest entry in the following commitfest is no good: I have seen that most of the threads are being discussed and being promptly updated. But very few of the entries become stale and just move from one commitfest to another commitfest without anything being done. For these kinds of entries, we were just trying to see if the author or anybody is really interested or not in pursuing it. We should do something about these kinds of entries, there were few suggestions like tagging under a new category or so, can we add a new status to park these entries something like "Waiting for direction". The threads which have no discussion for 6 months or so can be flagged to this new status and these can be discussed in one of the developer meetings or so and conclude on these items. Regards, Vignesh
On Sun, Feb 4, 2024 at 6:51 AM vignesh C <vignesh21@gmail.com> wrote: > We should do something about these kinds of entries, there were few > suggestions like tagging under a new category or so, can we add a new > status to park these entries something like "Waiting for direction". > The threads which have no discussion for 6 months or so can be flagged > to this new status and these can be discussed in one of the developer > meetings or so and conclude on these items. See also [1], with a patch suggestion. IIRC, there was also related discussion on making the "resurrection" process single-step so that you don't lose history, per Alvaro's concern. --Jacob [1] https://postgr.es/m/flat/CAAWbhmjM6hn_3sjVBUyqVKysWVAtsBOkaNt01QF3AMOCunuKMg%40mail.gmail.com
On 2024-Feb-04, vignesh C wrote: > We should do something about these kinds of entries, there were few > suggestions like tagging under a new category or so, can we add a new > status to park these entries something like "Waiting for direction". > The threads which have no discussion for 6 months or so can be flagged > to this new status and these can be discussed in one of the developer > meetings or so and conclude on these items. Maybe a new status is appropriate ... I would suggest "Stalled". Such a patch still applies and has no pending feedback, but nobody seems interested. Making a patch no longer stalled means there's discussion that leads to: 1. further development? Perhaps the author just needed more direction. 2. a decision that it's not a feature we want, or maybe not in this form. Then we close it as rejected. 3. a reviewer/committer finding time to provide additional feedback. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "The problem with the future is that it keeps turning into the present" (Hobbes)
> On 7 Feb 2024, at 13:37, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > Maybe a new status is appropriate ... I would suggest "Stalled". Such a > patch still applies and has no pending feedback, but nobody seems > interested. Since the CF app knows when the last email in the thread was, the state of the patch entry and the number of CF's which is has been present in; maybe we can extend the app to highlight these patches in a way which doesn't add more manual intervention? It might yield a few false positives, but so will setting it manually. -- Daniel Gustafsson
On 2024-Feb-07, Daniel Gustafsson wrote: > Since the CF app knows when the last email in the thread was, the > state of the patch entry and the number of CF's which is has been > present in; maybe we can extend the app to highlight these patches in > a way which doesn't add more manual intervention? It might yield a > few false positives, but so will setting it manually. Hmm, but suppose the author is posting new versions now and again because of apply conflicts; and the CFM is pinging them about that, but not providing any actionable feedback. Such a thread would be active enough, but the patch is still stalled. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "No hay hombre que no aspire a la plenitud, es decir, la suma de experiencias de que un hombre es capaz"
On Wed, Feb 7, 2024 at 6:07 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2024-Feb-04, vignesh C wrote: > > > We should do something about these kinds of entries, there were few > > suggestions like tagging under a new category or so, can we add a new > > status to park these entries something like "Waiting for direction". > > The threads which have no discussion for 6 months or so can be flagged > > to this new status and these can be discussed in one of the developer > > meetings or so and conclude on these items. > > Maybe a new status is appropriate ... I would suggest "Stalled". Such a > patch still applies and has no pending feedback, but nobody seems > interested. Making a patch no longer stalled means there's discussion > that leads to: > > 1. further development? Perhaps the author just needed more direction. > 2. a decision that it's not a feature we want, or maybe not in this > form. Then we close it as rejected. > 3. a reviewer/committer finding time to provide additional feedback. > +1. This suggestion sounds reasonable. I think we probably need to decide the time when the patch's status should be changed to "Stalled". -- With Regards, Amit Kapila.
> On 7 Feb 2024, at 14:15, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2024-Feb-07, Daniel Gustafsson wrote: > >> Since the CF app knows when the last email in the thread was, the >> state of the patch entry and the number of CF's which is has been >> present in; maybe we can extend the app to highlight these patches in >> a way which doesn't add more manual intervention? It might yield a >> few false positives, but so will setting it manually. > > Hmm, but suppose the author is posting new versions now and again > because of apply conflicts; and the CFM is pinging them about that, but > not providing any actionable feedback. Such a thread would be active > enough, but the patch is still stalled. If the patch author is actively rebasing the patch and thus generating traffic on the thread I'm not sure I would call it stalled - there might not be enough (or any) reviews but the activity alone might make someone interested. Either way, I'd personally prefer such false positives if it means reducing the manual workload for the CFM. -- Daniel Gustafsson