Thread: Commitfest 2023-03 starting tomorrow!
So I'm not sure if I'll be CFM this month but I'm assuming I will be at this point.... Regardless, Commitfest 2023-03 starts tomorrow! So this is a good time to check your submitted patches and ensure they're actually in building and don't need a rebase. Take a look at http://cfbot.cputube.org/ for example. I'll do an initial pass marking anything red here as Waiting on Author The next pass would be to grab any patches not marked Ready for Committer and if they look like they'll need more than a one round of feedback and a couple weeks to polish they'll probably get bounced to the next commitfest too. It sucks not getting feedback on your patches for so long but there are really just sooo many patches and so few eyeballs... It would be great if people could do initial reviews of these patches before we bounce them because it really is discouraging for developers to send patches and not get feedback. But realistically it's going to happen to a lot of patches. -- greg
On Tue, Feb 28, 2023 at 01:45:27PM -0500, Greg Stark wrote: > So I'm not sure if I'll be CFM this month but I'm assuming I will be > at this point.... Okay, that's OK for me! Thanks for helping out. > The next pass would be to grab any patches not marked Ready for > Committer and if they look like they'll need more than a one round of > feedback and a couple weeks to polish they'll probably get bounced to > the next commitfest too. It sucks not getting feedback on your patches > for so long but there are really just sooo many patches and so few > eyeballs... It would be great if people could do initial reviews of > these patches before we bounce them because it really is discouraging > for developers to send patches and not get feedback. But realistically > it's going to happen to a lot of patches. I don't have many patches registered this time for the sole reason of being able to spend more cycles on reviews and see what could make the cut. So we'll see how it goes, I guess.. The CF would begin in more or less 5 hours as of the moment of this message: https://www.timeanddate.com/time/zones/aoe -- Michael
Attachment
On Wed, Mar 01, 2023 at 03:47:17PM +0900, Michael Paquier wrote: > The CF would begin in more or less 5 hours as of the moment of this > message: > https://www.timeanddate.com/time/zones/aoe Note: I have switched this CF as "In Process" a few hours ago. -- Michael
Attachment
Sorry, I wasn't feeling very well since Friday. I'm still not 100% but I'm going to try to do some triage this afternoon. There are a few patches that need a rebase. And a few patches failing Meson builds or autoconf stages -- I wonder if there's something unrelated broken there? But what I think is really needed is for committers to pick up patches that are ready to commit and grab them. There are currently two patches with macdice marked as committer and one with michael-kun (i.e. you:) But what can we do to get more some patches picked up now instead of at the end of the commitfest? Would it help if I started asking on existing threads if there's a committer willing to take it up? On Wed, 1 Mar 2023 at 20:27, Michael Paquier <michael@paquier.xyz> wrote: > > On Wed, Mar 01, 2023 at 03:47:17PM +0900, Michael Paquier wrote: > > The CF would begin in more or less 5 hours as of the moment of this > > message: > > https://www.timeanddate.com/time/zones/aoe > > Note: I have switched this CF as "In Process" a few hours ago. > -- > Michael -- Gregory Stark As Commitfest Manager
So, sorry I've been a bit under the weather but can concentrate on the commitfest now. I tried to recapitulate the history but the activity log only goes back a certain distance on the web. If I can log in to the database I guess I could construct the history from sql queries if it was important. But where we stand now is: Status summary: Needs review: 152. Waiting on Author: 42. Ready for Committer: 39. Committed: 61. Moved to next CF: 4. Withdrawn: 17. Returned with Feedback: 4. Total: 319. Of the Needs Review patches there are 81 that have received no email messages since the CF began. A lot of those have reviews attached but presumably those reviewers are mostly from earlier CFs and may have already contributed all they can. I don't know, should we move some or all of these to the next CF already? I'm reluctant to bounce them en masse because there are definitely some patches that will get reviewed and some that should really be marked "Ready for Committer". These patches that are "Needs Review" and have received no comments at all since before March 1st are these. If your patch is amongst this list I would suggest any of: 1) Move it yourself to the next CF (or withdraw it) 2) Post to the list with any pending questions asking for specific feedback -- it's much more likely to get feedback than just a generic "here's a patch plz review"... 3) Mark it Ready for Committer and possibly post explaining the resolution to any earlier questions to make it easier for a committer to understand the state If there's still no emails on these at some point I suppose it will make sense to move them out of the CF. * ALTER TABLE SET ACCESS METHOD on partitioned tables * New hooks in the connection path * Add log messages when replication slots become active and inactive * Avoid use deprecated Windows Memory API * Remove dead macro exec_subplan_get_plan * Consider parallel for LATERAL subqueries having LIMIT/OFFSET * pg_rewind WAL deletion pitfall * Simplify find_my_exec by using realpath(3) * Move backup-related code to xlogbackup.c/.h * Avoid hiding shared filesets in pg_ls_tmpdir (pg_ls_* functions for showing metadata ...) * Fix bogus error emitted by pg_recvlogical when interrupted * warn if GUC set to an invalid shared library * Check consistency of GUC defaults between .sample.conf and pg_settings.boot_val * Code checks for App Devs, using new options for transaction behavior * Lockless queue of waiters based on atomic operations for LWLock * Fix assertion failure with next_phase_at in snapbuild.c * Add SPLIT PARTITION/MERGE PARTITIONS commands * Add sortsupport for range types and btree_gist * asynchronous execution support for Custom Scan * Periodic burst growth of the checkpoint_req counter on replica. * CREATE INDEX CONCURRENTLY on partitioned table * Fix ParamPathInfo for union-all AppendPath * Add OR REPLACE option for CREATE OPERATOR * ALTER TABLE and CLUSTER fail to use a BulkInsertState for toast tables * Partial aggregates push down * Non-replayable WAL records through overflows and >MaxAllocSize lengths * Enable jitlink as an alternative jit linker of legacy Rtdyld and add riscv jitting support * Test for function error in logrep worker * basebackup: support zstd long distance matching * pgbench - adding pl/pgsql versions of tests * Function to log backtrace of postgres processes * More scalable multixacts buffers and locking * Remove nonmeaningful prefixes in PgStat_* fields * COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns * postgres_fdw: commit remote (sub)transactions in parallel during pre-commit * Add semi-join pushdown to postgres_fdw * Skip replicating the tables specified in except table option * Split index and table statistics into different types of stats * Exclusion constraints on partitioned tables * Post-special Page Storage TDE support * Direct I/O (developer-only feature) * Improve doc for autovacuum on partitioned tables * Patch to implement missing join selectivity estimation for range types * Clarify the behavior of the system when approaching XID wraparound * Set arbitrary GUC options during initdb * An attempt to avoid locally-committed-but-not-replicated-to-standby-transactions in synchronous replication * Check lateral references within PHVs for memoize cache keys * Add n_tup_newpage_upd to pg_stat table views * monitoring usage count distribution * Reduce wakeup on idle for bgwriter & walwriter for >5s * Report the query string that caused a memory error under Valgrind * New [relation] options engine * Data is copied twice when specifying both child and parent table in publication * possibility to take name, signature and oid of currently executed function in GET DIAGNOSTICS statement * Named Operators * nbtree performance improvements through specialization on key shape * Fix assertion failure in SnapBuildInitialSnapshot() * Speed up releasing of locks * Compression dictionaries * Improve pg_bsd_indent's handling of multiline initialization expressions * Add EXPLAIN option GENERIC_PLAN for parameterized queries * User functions for building SCRAM secrets * Exit walsender before confirming remote flush in logical replication * Refactoring postgres_fdw/connection.c * Add pg_stat_session * Doc: Improve note about copying into postgres_fdw foreign tables in batch * Kerberos/GSSAPI Credential Delegation * archive modules loose ends * Fix dsa_free() to re-bin segment * Reduce timing overhead of EXPLAIN ANALYZE using rdtsc * clean up permission checks after 599b33b94 * Some revises in adding sorting path * ResourceOwner refactoring * Fix the description of GUC "max_locks_per_transaction" and "max_pred_locks_per_transaction" in guc_table.c * some namespace.c refactoring * Add function to_oct * Switching XLog source from archive to streaming when primary available * Dynamic result sets from procedures * BRIN - SK_SEARCHARRAY and scan key preprocessing * MERGE ... WHEN NOT MATCHED BY SOURCE * Reuse Workers and Replication Slots during Logical Replication
On Wed, 15 Mar 2023 at 14:29, Gregory Stark (as CFM) <stark.cfm@gmail.com> wrote: > > These patches that are "Needs Review" and have received no comments at > all since before March 1st are these. If your patch is amongst this > list I would suggest any of: > > 1) Move it yourself to the next CF (or withdraw it) > 2) Post to the list with any pending questions asking for specific > feedback -- it's much more likely to get feedback than just a generic > "here's a patch plz review"... > 3) Mark it Ready for Committer and possibly post explaining the > resolution to any earlier questions to make it easier for a committer > to understand the state > > If there's still no emails on these at some point I suppose it will > make sense to move them out of the CF. I'm going to go ahead and do this today. Any of these patches that are "Waiting on Author" and haven't received any emails or status changes since March 1 I'm going to move out of the commitfest(*). If you really think your patch in this list is important to get committed then please respond to the thread explaining any plans or feedback needed. It would be nice to actually do Returned With Feedback where appropriate but there are too many to go through them thoroughly. I'll only be able to do a quick review of each thread checking for important bug fixes or obviously rejected patches. (*) I reserve the right to skip and leave some patches where appropriate. In particular I'll skip patches that are actually from committers on the theory that they could just commit them when they feel like it anyways. Some patches may be intentionally waiting until the end of the release cycle to avoid conflicts too. > * ALTER TABLE SET ACCESS METHOD on partitioned tables > * New hooks in the connection path > * Add log messages when replication slots become active and inactive > * Avoid use deprecated Windows Memory API > * Remove dead macro exec_subplan_get_plan > * Consider parallel for LATERAL subqueries having LIMIT/OFFSET > * pg_rewind WAL deletion pitfall > * Simplify find_my_exec by using realpath(3) > * Move backup-related code to xlogbackup.c/.h > * Avoid hiding shared filesets in pg_ls_tmpdir (pg_ls_* functions for > showing metadata ...) > * Fix bogus error emitted by pg_recvlogical when interrupted > * warn if GUC set to an invalid shared library > * Check consistency of GUC defaults between .sample.conf and > pg_settings.boot_val > * Code checks for App Devs, using new options for transaction behavior > * Lockless queue of waiters based on atomic operations for LWLock > * Fix assertion failure with next_phase_at in snapbuild.c > * Add SPLIT PARTITION/MERGE PARTITIONS commands > * Add sortsupport for range types and btree_gist > * asynchronous execution support for Custom Scan > * Periodic burst growth of the checkpoint_req counter on replica. > * CREATE INDEX CONCURRENTLY on partitioned table > * Fix ParamPathInfo for union-all AppendPath > * Add OR REPLACE option for CREATE OPERATOR > * ALTER TABLE and CLUSTER fail to use a BulkInsertState for toast tables > * Partial aggregates push down > * Non-replayable WAL records through overflows and >MaxAllocSize lengths > * Enable jitlink as an alternative jit linker of legacy Rtdyld and > add riscv jitting support > * Test for function error in logrep worker > * basebackup: support zstd long distance matching > * pgbench - adding pl/pgsql versions of tests > * Function to log backtrace of postgres processes > * More scalable multixacts buffers and locking > * Remove nonmeaningful prefixes in PgStat_* fields > * COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns > * postgres_fdw: commit remote (sub)transactions in parallel during pre-commit > * Add semi-join pushdown to postgres_fdw > * Skip replicating the tables specified in except table option > * Split index and table statistics into different types of stats > * Exclusion constraints on partitioned tables > * Post-special Page Storage TDE support > * Direct I/O (developer-only feature) > * Improve doc for autovacuum on partitioned tables > * Patch to implement missing join selectivity estimation for range types > * Clarify the behavior of the system when approaching XID wraparound > * Set arbitrary GUC options during initdb > * An attempt to avoid > locally-committed-but-not-replicated-to-standby-transactions in > synchronous replication > * Check lateral references within PHVs for memoize cache keys > * Add n_tup_newpage_upd to pg_stat table views > * monitoring usage count distribution > * Reduce wakeup on idle for bgwriter & walwriter for >5s > * Report the query string that caused a memory error under Valgrind > * New [relation] options engine > * Data is copied twice when specifying both child and parent table in > publication > * possibility to take name, signature and oid of currently executed > function in GET DIAGNOSTICS statement > * Named Operators > * nbtree performance improvements through specialization on key shape > * Fix assertion failure in SnapBuildInitialSnapshot() > * Speed up releasing of locks > * Compression dictionaries > * Improve pg_bsd_indent's handling of multiline initialization expressions > * Add EXPLAIN option GENERIC_PLAN for parameterized queries > * User functions for building SCRAM secrets > * Exit walsender before confirming remote flush in logical replication > * Refactoring postgres_fdw/connection.c > * Add pg_stat_session > * Doc: Improve note about copying into postgres_fdw foreign tables in batch > * Kerberos/GSSAPI Credential Delegation > * archive modules loose ends > * Fix dsa_free() to re-bin segment > * Reduce timing overhead of EXPLAIN ANALYZE using rdtsc > * clean up permission checks after 599b33b94 > * Some revises in adding sorting path > * ResourceOwner refactoring > * Fix the description of GUC "max_locks_per_transaction" and > "max_pred_locks_per_transaction" in guc_table.c > * some namespace.c refactoring > * Add function to_oct > * Switching XLog source from archive to streaming when primary available > * Dynamic result sets from procedures > * BRIN - SK_SEARCHARRAY and scan key preprocessing > * MERGE ... WHEN NOT MATCHED BY SOURCE > * Reuse Workers and Replication Slots during Logical Replication -- greg
Hi Greg, > > These patches that are "Needs Review" and have received no comments at > > all since before March 1st are these. If your patch is amongst this > > list I would suggest any of: > > > > 1) Move it yourself to the next CF (or withdraw it) > > 2) Post to the list with any pending questions asking for specific > > feedback -- it's much more likely to get feedback than just a generic > > "here's a patch plz review"... > > 3) Mark it Ready for Committer and possibly post explaining the > > resolution to any earlier questions to make it easier for a committer > > to understand the state Sorry for the late reply. It was a busy week. I see several patches I authored and/or reviewed in the list. I would like to comment on those. * Avoid use deprecated Windows Memory API We can reject or mark as RwF this one due to controversy and the fact that the patch doesn't currently apply. I poked the author today. * Clarify the behavior of the system when approaching XID wraparound This is a wanted [1][see the discussion] and a pretty straightforward change. I think it should be targeting PG16. * Compression dictionaries This one doesn't target PG16. Moved to the next CF. * Add pg_stat_session This patch was in good shape last time I checked but other people had certain questions. The author hasn't replied since Feb 16th. So it's unlikely to end up in PG6 and I suggest moving it to the next CF, unless anyone objects. * ResourceOwner refactoring IMO this one still has a chance to make it to PG16. Let's keep it in the CF for now. Additionally: * Add 64-bit XIDs into PostgreSQL 16 Is not going to make it to PG16, moving to the next CF. * Pluggable toaster The discussion is happening in the "Compression dictionaries" thread now, since we decided to join our efforts in this area, see the latest messages. I suggest marking this thread as RwF, unless anyone objects. [1]: https://www.postgresql.org/message-id/CAH2-Wz%3D3mmHST-t9aR5LNkivXC%2B18JD_XC0ht4y5LQBLzq%2Bpsg%40mail.gmail.com -- Best regards, Aleksander Alekseev
Greg Stark <stark@mit.edu> writes:
>> These patches that are "Needs Review" and have received no comments at
>> all since before March 1st are these.
Just a couple of comments on ones that caught my eye:
>> * Simplify find_my_exec by using realpath(3)
The problem with this one is that Peter would like it to do something
other than what I think it should do. Not sure how to resolve that.
>> * Fix assertion failure with next_phase_at in snapbuild.c
This one, and others that are bug fixes, probably deserve more slack.
>> * Periodic burst growth of the checkpoint_req counter on replica.
There is recent discussion of this one no?
>> * Fix ParamPathInfo for union-all AppendPath
I pushed this yesterday.
>> * Add OR REPLACE option for CREATE OPERATOR
I think this one should be flat-out rejected.
>> * Partial aggregates push down
You've listed a lot of small features here that still have time to
get some love --- it's not like we're hard up against the end of the CF.
If they'd been in Waiting on Author state for awhile, I'd agree with
booting them, but not when they're in Needs Review.
>> * Set arbitrary GUC options during initdb
I do indeed intend to push this one on my own authority at some point,
but I'm happy to leave it there for now in case anyone wants to take
another look.
>> * Check lateral references within PHVs for memoize cache keys
I think this one is a bug fix too.
>> * Data is copied twice when specifying both child and parent table in
>> publication
Isn't there active discussion of this one?
>> * Improve pg_bsd_indent's handling of multiline initialization expressions
This is going to get pushed, it's just waiting until the commitfest
settles. I guess you can move it to the next one if you want, but
that won't accomplish much.
regards, tom lane
On Wed, Mar 15, 2023 at 02:29:26PM -0400, Gregory Stark (as CFM) wrote: > 1) Move it yourself to the next CF (or withdraw it) > 2) Post to the list with any pending questions asking for specific > feedback -- it's much more likely to get feedback than just a generic > "here's a patch plz review"... > 3) Mark it Ready for Committer and possibly post explaining the > resolution to any earlier questions to make it easier for a committer > to understand the state > > If there's still no emails on these at some point I suppose it will > make sense to move them out of the CF. > * Avoid hiding shared filesets in pg_ls_tmpdir (pg_ls_* functions for > showing metadata ...) My patch. I don't care if it's in v1[3456], but I wish it would somehow progress - idk what else is needed. I wrote this after Thomas Munro +1 my thinking that it's absurd for pg_ls_tmpdir() to not show [shared] filesets in tempdirs... > * CREATE INDEX CONCURRENTLY on partitioned table My patch. I think there's agreement that this patch is ready, except that it's waiting on the bugfix for the progress reporting patch. IDK if there's interest in this, but it'd be a good candidate for v16. > * basebackup: support zstd long distance matching My patch. No discussion, but I'm hopeful and don't see why this shouldn't be in v16. > * warn if GUC set to an invalid shared library My patch. I'm waiting for feedback on 0001, which has gotten no response. I moved it. > * ALTER TABLE and CLUSTER fail to use a BulkInsertState for toast tables My patch. There's been no recent discussion, so I guess I'll postpone it for v17. > * Check consistency of GUC defaults between .sample.conf and pg_settings.boot_val IDK what's needed to progress this; If left here, since it will cause *this* patch to fail if someone else forgets to add a new GUC to the sample config. Which is odd. -- Justin
On Fri, 17 Mar 2023 at 10:39, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > You've listed a lot of small features here that still have time to > get some love --- it's not like we're hard up against the end of the CF. > If they'd been in Waiting on Author state for awhile, I'd agree with > booting them, but not when they're in Needs Review. Oh, that's exactly my intent -- when I listed them two days ago it was a list of Waiting on Author patches without updates since March 1. But I didn't recheck them this morning yet. If they've gotten comments in the last two days or had their status updated then great. It's also possible there are threads that aren't attached to the commitfest or are attached to a related patch that I may not be aware of. -- greg
On 2023-Mar-17, Greg Stark wrote: > I'm going to go ahead and do this today. Any of these patches that are > "Waiting on Author" and haven't received any emails or status changes > since March 1 I'm going to move out of the commitfest(*). So I've come around to thinking that booting patches out of commitfest is not really such a great idea. It turns out that the number of active patch submitters seems to have reached a peak during the Postgres 12 timeframe, and has been steadily decreasing since then; and I think this is partly due to frustration caused by our patch process. It turns out that we expect that contributors will keep the patches the submit up to date, rebasing over and over for months on end, with no actual review occurring, and if this rebasing activity stops for a few weeks, we boot these patches out. This is demotivating: people went great lengths to introduce themselves to our admittedly antiquated process (no pull requests, remember), we gave them no feedback, and then we reject their patches with no further effort? I think this is not good. At this point, I'm going to suggest that reviewers should be open to the idea of applying a submitted patch to some older Git commit in order to review it. If we have given feedback, then it's OK to put a patch as "waiting on author" and eventually boot it; but if we have not given feedback, and there is no reason to think that the merge conflicts some how make the patch fundamentally obsolete, then we should *not* set it Waiting on Author. After all, it is quite easy to "git checkout" a slightly older tree to get the patch to apply cleanly and review it there. Authors should, of course, be encouraged to keep patches conflict-free, but this should not be a hard requirement. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
On Sat, Mar 18, 2023 at 1:26 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > At this point, I'm going to suggest that reviewers should be open to the > idea of applying a submitted patch to some older Git commit in order to > review it. If we have given feedback, then it's OK to put a patch as > "waiting on author" and eventually boot it; but if we have not given > feedback, and there is no reason to think that the merge conflicts some > how make the patch fundamentally obsolete, then we should *not* set it > Waiting on Author. After all, it is quite easy to "git checkout" a > slightly older tree to get the patch to apply cleanly and review it > there. It seems plausible that improved tooling that makes it quick and easy to test a given patch locally could improve things for everybody. It's possible to do a git checkout to a slightly older tree today, of course. But in practice it's harder than it really should be. It would be very nice if there was an easy way to fetch from a git remote, and then check out a branch with a given patch applied on top of the "last known good git tip" commit. The tricky part would be systematically tracking which precise master branch commit is the last known "good commit" for a given CF entry. That seems doable to me. I suspect that removing friction when it comes to testing a patch locally (often just "kicking the tires" of a patch) could have an outsized impact. If something is made extremely easy, and requires little or no context to get going with, then people tend to do much more of it. Even when they theoretically don't have a good reason to do so. And even when they theoretically already had a good reason to do so, before the improved tooling/workflow was in place. -- Peter Geoghegan
On Sat, Mar 18, 2023 at 02:43:38PM -0700, Peter Geoghegan wrote: > On Sat, Mar 18, 2023 at 1:26 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > At this point, I'm going to suggest that reviewers should be open to the > > idea of applying a submitted patch to some older Git commit in order to > > review it. If we have given feedback, then it's OK to put a patch as > > "waiting on author" and eventually boot it; but if we have not given > > feedback, and there is no reason to think that the merge conflicts some > > how make the patch fundamentally obsolete, then we should *not* set it > > Waiting on Author. After all, it is quite easy to "git checkout" a > > slightly older tree to get the patch to apply cleanly and review it > > there. > > It seems plausible that improved tooling that makes it quick and easy > to test a given patch locally could improve things for everybody. > > It's possible to do a git checkout to a slightly older tree today, of > course. But in practice it's harder than it really should be. It would > be very nice if there was an easy way to fetch from a git remote, and > then check out a branch with a given patch applied on top of the "last > known good git tip" commit. The tricky part would be systematically > tracking which precise master branch commit is the last known "good > commit" for a given CF entry. That seems doable to me. It's not only doable, but already possible. https://www.postgresql.org/message-id/CA%2BhUKGLW2PnHxabF3JZGoPfcKFYRCtx%2Bhu5a5yw%3DKWy57yW5cg%40mail.gmail.com The only issue with this is that cfbot has squished all the commits into one, and lost the original commit messages (if any). I submitted patches to address that but still waiting for feedback. https://www.postgresql.org/message-id/20220623193125.GB22452@telsasoft.com -- Justin
On Sat, Mar 18, 2023 at 4:19 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > The only issue with this is that cfbot has squished all the commits into > one, and lost the original commit messages (if any). I submitted > patches to address that but still waiting for feedback. > > https://www.postgresql.org/message-id/20220623193125.GB22452@telsasoft.com Right. I would like to see that change. But you still need to have CF tester/the CF app remember the last master branch commit that worked before bitrot. And you have to provide an easy way to get that information. I generally don't care if that means that I have to initdb - I do that all the time. It's a small price to pay for a workflow that I know is practically guaranteed to get me a usable postgres executable on the first try, without requiring any special effort. I don't want to even think about bitrot until I'm at least 10 minutes into looking at something. -- Peter Geoghegan
On Sat, Mar 18, 2023 at 04:28:02PM -0700, Peter Geoghegan wrote: > On Sat, Mar 18, 2023 at 4:19 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > > The only issue with this is that cfbot has squished all the commits into > > one, and lost the original commit messages (if any). I submitted > > patches to address that but still waiting for feedback. > > > > https://www.postgresql.org/message-id/20220623193125.GB22452@telsasoft.com > > Right. I would like to see that change. But you still need to have CF > tester/the CF app remember the last master branch commit that worked > before bitrot. And you have to provide an easy way to get that > information. No - the last in cfbot's repo is from the last time it successfully applied the patch. You can summarily check checkout cfbot's branch to build (or just to git log -p it, if you dislike github's web interface). If you're curious and still wanted to know what commit it was applied on, it's currently the 2nd commit in "git log" (due to squishing all patches into one).
On 17.03.23 15:38, Tom Lane wrote: >>> Simplify find_my_exec by using realpath(3) > The problem with this one is that Peter would like it to do something > other than what I think it should do. Not sure how to resolve that. I have no objection to changing the internal coding of the current behavior.
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> On 17.03.23 15:38, Tom Lane wrote:
>>> Simplify find_my_exec by using realpath(3)
>> The problem with this one is that Peter would like it to do something
>> other than what I think it should do. Not sure how to resolve that.
> I have no objection to changing the internal coding of the current behavior.
Oh ... where the thread trailed off [1] was you not answering whether
you'd accept a compromise behavior. If it's okay to stick with the
behavior we have, then I'll just do the original patch (modulo Munro's
observations about _fullpath's error reporting).
regards, tom lane
[1] https://www.postgresql.org/message-id/2319396.1664978360%40sss.pgh.pa.us
On Sun, Mar 19, 2023 at 12:44 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > On Sat, Mar 18, 2023 at 04:28:02PM -0700, Peter Geoghegan wrote: > > On Sat, Mar 18, 2023 at 4:19 PM Justin Pryzby <pryzby@telsasoft.com> wrote: > > > The only issue with this is that cfbot has squished all the commits into > > > one, and lost the original commit messages (if any). I submitted > > > patches to address that but still waiting for feedback. > > > > > > https://www.postgresql.org/message-id/20220623193125.GB22452@telsasoft.com > > > > Right. I would like to see that change. But you still need to have CF > > tester/the CF app remember the last master branch commit that worked > > before bitrot. And you have to provide an easy way to get that > > information. > > No - the last in cfbot's repo is from the last time it successfully > applied the patch. You can summarily check checkout cfbot's branch to > build (or just to git log -p it, if you dislike github's web interface). > > If you're curious and still wanted to know what commit it was applied > on, it's currently the 2nd commit in "git log" (due to squishing > all patches into one). I realised that part of Alvaro's complaint was probably caused by cfbot's refusal to show any useful information just because it couldn't apply a patch the last time it tried. A small improvement today: now it shows a ♲ symbol (with hover text "Rebase needed") if it doesn't currently apply, but you can still see the most recent CI test results. And from there you can find your way to the parent commit ID. The reason for the previous behaviour is that it had no memory, but I had to give it one that so I can study flapping tests, log highlights, statistical trends etc. Reminds me, I also need to teach it to track the postgres/postgres master mirror's CI results, because it's still (rather stupidly) testing patches when master itself is failing (eg the recent slapd commits), which ought to be easy enough to avoid given the data...
On Mon, Mar 20, 2023 at 11:13 AM Thomas Munro <thomas.munro@gmail.com> wrote: > I realised that part of Alvaro's complaint was probably caused by > cfbot's refusal to show any useful information just because it > couldn't apply a patch the last time it tried. A small improvement > today: now it shows a ♲ symbol (with hover text "Rebase needed") if it > doesn't currently apply, but you can still see the most recent CI test > results. And from there you can find your way to the parent commit > ID. And in the cases where it still shows no results, that'd be because the patch set hasn't successfully applied in the past 46 days, ie since 1 Feb, when cfbot started retaining history. That visible amnesia should gradually disappear as those patches make progress and the history window expands. I suppose then someone might complain that it should be clearer if a patch hasn't applied for a very long time; suggestions for how to show that are welcome. I wondered about making them gradually fade out to white, ghost memories that eventually disappear completely after a few commitfests :-D
Thomas Munro <thomas.munro@gmail.com> writes:
> ... I suppose then someone might complain
> that it should be clearer if a patch hasn't applied for a very long
> time; suggestions for how to show that are welcome.
Can you make the pop-up tooltip text read "Rebase needed since
YYYY-MM-DD"?
regards, tom lane
On Mon, Mar 20, 2023 at 1:10 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Thomas Munro <thomas.munro@gmail.com> writes: > > ... I suppose then someone might complain > > that it should be clearer if a patch hasn't applied for a very long > > time; suggestions for how to show that are welcome. > > Can you make the pop-up tooltip text read "Rebase needed since > YYYY-MM-DD"? Done. It's the GMT date of the first failure to apply.
The next level of this would be something like notifying the committer with a list of patches in the CF that a commit broke. I don't immediately see how to integrate that with our workflow but I have seen something like this work well in a previous job. When committing code you often went and updated other unrelated projects to adapt to the new API (or could adjust the code you were committing to cause less breakage).
On Tue, Mar 21, 2023 at 3:15 AM Greg Stark <stark@mit.edu> wrote: > The next level of this would be something like notifying the committer > with a list of patches in the CF that a commit broke. I don't > immediately see how to integrate that with our workflow but I have > seen something like this work well in a previous job. When committing > code you often went and updated other unrelated projects to adapt to > the new API (or could adjust the code you were committing to cause > less breakage). I've been hesitant to make it send email. The most obvious message to send would be "hello, you posted a patch, but it fails on CI" to the submitter. Cfbot has been running for about 5 years now, and I'd say the rate of spurious/bogus failures has come down a lot over that time as we've chased down the flappers in master, but it's still enough that you would quickly become desensitised/annoyed by the emails, I guess (one of the reasons I recently started keeping history is to be able to do some analysis of that so we can direct attention to chasing down the rest, or have some smart detection of known but not yet resolved flappers, I dunno, something like that). Speaking as someone who occasionally sends patches to other projects, it's confusing and unsettling when you get automated emails from github PRs telling you that this broke, that broke and the other broke, but only the project insiders know which things are newsworthy and which things are "oh yeah that test is a bit noisy, ignore that one".
On Mon, 20 Mar 2023 at 16:08, Thomas Munro <thomas.munro@gmail.com> wrote: > > On Tue, Mar 21, 2023 at 3:15 AM Greg Stark <stark@mit.edu> wrote: > > The next level of this would be something like notifying the committer > > with a list of patches in the CF that a commit broke. I don't > > immediately see how to integrate that with our workflow but I have > > seen something like this work well in a previous job. When committing > > code you often went and updated other unrelated projects to adapt to > > the new API (or could adjust the code you were committing to cause > > less breakage). > > I've been hesitant to make it send email. The most obvious message to > send would be "hello, you posted a patch, but it fails on CI" to the > submitter. Yeah, even aside from flappers there's the problem that it's about as common for real commits to break some test as it is for patches to start failing tests. So often it's a real failure but it's nothing to do with the patch, it has to do with the commit that went into git. What I'm interested in is something like "hey, your commit caused 17 patches to start failing tests". And it doesn't necessarily have to be an email. Having a historical page so when I look at a patch I can go check, "hey did this start failing at the same time as 17 other patches on the same commit?" would be the same question. -- greg
On Tue, Mar 21, 2023 at 9:22 AM Greg Stark <stark@mit.edu> wrote: > Yeah, even aside from flappers there's the problem that it's about as > common for real commits to break some test as it is for patches to > start failing tests. So often it's a real failure but it's nothing to > do with the patch, it has to do with the commit that went into git. That is true. The best solution to that problem, I think, is to teach cfbot that it should test on top of the most recent master commit that succeeded in CI here https://github.com/postgres/postgres/commits/master . It's on my list...
On 2023-Mar-20, Thomas Munro wrote: > I realised that part of Alvaro's complaint was probably caused by > cfbot's refusal to show any useful information just because it > couldn't apply a patch the last time it tried. A small improvement > today: now it shows a ♲ symbol (with hover text "Rebase needed") if it > doesn't currently apply, but you can still see the most recent CI test > results. And from there you can find your way to the parent commit > ID. Thank you for improving and continue to think about further enhancements to the CF bot. It has clearly improved our workflow a lot. My complaint wasn't actually targetted at the CF bot. It turns out that I gave a talk on Friday at a private EDB mini-conference about the PostgreSQL open source process; and while preparing for that one, I ran some 'git log' commands to obtain the number of code contributors for each release, going back to 9.4 (when we started using the 'Authors:' tag more prominently). What I saw is a decline in the number of unique contributors, from its maximum at version 12, down to the numbers we had in 9.5. We went back 4 years. That scared me a lot. So I started a conversation about that and some people told me that it's very easy to be discouraged by our process. I don't need to mention that it's antiquated -- this in itself turns off youngsters. But in addition to that, I think newbies might be discouraged because their contributions seem to go nowhere even after following the process. This led me to suggesting that perhaps we need to be more lenient when it comes to new contributors. As I said, for seasoned contributors, it's not a problem to keep up with our requirements, however silly they are. But people who spend their evenings a whole week or month trying to understand how to patch for one thing that they want, to be received by six months of silence followed by a constant influx of "please rebase please rebase please rebase", no useful feedback, and termination with "eh, you haven't rebased for the 1001th time, your patch has been WoA for X days, we're setting it RwF, feel free to return next year" ... they are most certainly off-put and will *not* try again next year. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Por suerte hoy explotó el califont porque si no me habría muerto de aburrido" (Papelucho)
On Tue, 21 Mar 2023 at 05:59, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2023-Mar-20, Thomas Munro wrote: > > This led me to suggesting that perhaps we need to be more lenient when > it comes to new contributors. As I said, for seasoned contributors, > it's not a problem to keep up with our requirements, however silly they > are. But people who spend their evenings a whole week or month trying > to understand how to patch for one thing that they want, to be received > by six months of silence followed by a constant influx of "please rebase > please rebase please rebase", no useful feedback, and termination with > "eh, you haven't rebased for the 1001th time, your patch has been WoA > for X days, we're setting it RwF, feel free to return next year" ... > they are most certainly off-put and will *not* try again next year. I feel like the "no useful feedback" is the real problem though. If the patches had been reviewed in earlier commitfests the original contributor would still have been around to finish it... Like, I think what would actually solve this problem would be if we kept a "clean" house where patches were committed within one or two commitfests rather than dragged forward until the final commitfest. I do agree though. It would be nice if it was easier for anyone to do trivial merges and update the commitfest entry. That's the kind of thing gitlab/github are better positioned to solve when they can have integral editors and built-in CI... I haven't been RwF or moving to the next commitfest when the merge looked trivial. And in one case I actually did the merge myself :) But that only goes so far. If the merge actually requires understanding the patch in depth then the counter-argument is that the committer might be spending a lot of time on a patch that won't get committed while others sit ignored entirely. -- greg
So a week later Status summary: March 15 March 22 Needs review: 152 128 Waiting on Author: 42 36 Ready for Committer: 39 32 Committed: 61 82 Moved to next CF: 4 15 Withdrawn: 17 16 (?) Rejected: 0 5 Returned with Feedback: 4 5 Total: 319. These patches that are "Needs Review" and have received no comments at all since before March 1st are now below. There are about 20 fewer such patches than there were last week. No emails since August-December 2022: * New hooks in the connection path * Add log messages when replication slots become active and inactive * Remove dead macro exec_subplan_get_plan * Consider parallel for LATERAL subqueries having LIMIT/OFFSET * pg_rewind WAL deletion pitfall * Simplify find_my_exec by using realpath(3) * Move backup-related code to xlogbackup.c/.h * Avoid hiding shared filesets in pg_ls_tmpdir (pg_ls_* functions for showing metadata ...) * Fix bogus error emitted by pg_recvlogical when interrupted * Check consistency of GUC defaults between .sample.conf and pg_settings.boot_val * Code checks for App Devs, using new options for transaction behavior * Lockless queue of waiters based on atomic operations for LWLock * Fix assertion failure with next_phase_at in snapbuild.c * Add sortsupport for range types and btree_gist * asynchronous execution support for Custom Scan * CREATE INDEX CONCURRENTLY on partitioned table * Partial aggregates push down * Non-replayable WAL records through overflows and >MaxAllocSize lengths No emails since January 2023 * Enable jitlink as an alternative jit linker of legacy Rtdyld and add riscv jitting support * basebackup: support zstd long distance matching * pgbench - adding pl/pgsql versions of tests * Function to log backtrace of postgres processes * More scalable multixacts buffers and locking * COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns * postgres_fdw: commit remote (sub)transactions in parallel during pre-commit * Add semi-join pushdown to postgres_fdw * Skip replicating the tables specified in except table option * Post-special Page Storage TDE support * Direct I/O (developer-only feature) * Improve doc for autovacuum on partitioned tables * Set arbitrary GUC options during initdb * An attempt to avoid locally-committed-but-not-replicated-to-standby-transactions in synchronous replication * Check lateral references within PHVs for memoize cache keys * monitoring usage count distribution * Reduce wakeup on idle for bgwriter & walwriter for >5s * Report the query string that caused a memory error under Valgrind No emails since February 2023 * New [relation] options engine * possibility to take name, signature and oid of currently executed function in GET DIAGNOSTICS statement * Named Operators * nbtree performance improvements through specialization on key shape * Fix assertion failure in SnapBuildInitialSnapshot() * Speed up releasing of locks * Improve pg_bsd_indent's handling of multiline initialization expressions * User functions for building SCRAM secrets * Refactoring postgres_fdw/connection.c * Add pg_stat_session * Doc: Improve note about copying into postgres_fdw foreign tables in batch * archive modules loose ends * Fix dsa_free() to re-bin segment * Reduce timing overhead of EXPLAIN ANALYZE using rdtsc * clean up permission checks after 599b33b94 * Some revises in adding sorting path * ResourceOwner refactoring * Fix the description of GUC "max_locks_per_transaction" and "max_pred_locks_per_transaction" in guc_table.c * some namespace.c refactoring * Add function to_oct * Switching XLog source from archive to streaming when primary available * Dynamic result sets from procedures * BRIN - SK_SEARCHARRAY and scan key preprocessing * Reuse Workers and Replication Slots during Logical Replication
On Tue, Mar 21, 2023 at 10:59 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> I gave a talk on Friday at a private EDB mini-conference about the
> PostgreSQL open source process; and while preparing for that one, I
> ran some 'git log' commands to obtain the number of code contributors
> for each release, going back to 9.4 (when we started using the
> 'Authors:' tag more prominently). What I saw is a decline in the number
> of unique contributors, from its maximum at version 12, down to the
> numbers we had in 9.5. We went back 4 years. That scared me a lot.
Can you share the subtotals?
One immediate thought about commit log-based data is that we're not
using git Author, and the Author footer convention is only used by
some committers. So I guess it must have been pretty laborious to
read the prose-form data? We do have machine-readable Discussion
footers though. By scanning those threads for SMTP From headers on
messages that had patches attached, we can find the set of (distinct)
addresses that contributed to each commit. (I understand that some
people are co-authors and may not send an email, but if you counted
those and I didn't then you counted more, not fewer, contributors I
guess? On the other hand if someone posted a patch that wasn't used
in the commit, or posted from two home/work/whatever accounts that's a
false positive for my technique.)
In a quick and dirty attempt at this made from bits of Python I
already had lying around (which may of course later turn out to be
flawed and need refinement), I extracted, for example:
postgres=# select * from t where commit =
'8d578b9b2e37a4d9d6f422ced5126acec62365a7';
commit | time |
address
------------------------------------------+------------------------+----------------------------------------------
8d578b9b2e37a4d9d6f422ced5126acec62365a7 | 2023-03-21 14:29:34+13 |
Melanie Plageman <melanieplageman@gmail.com>
8d578b9b2e37a4d9d6f422ced5126acec62365a7 | 2023-03-21 14:29:34+13 |
Thomas Munro <thomas.munro@gmail.com>
(2 rows)
You can really only go back about 5-7 years before that technique runs
out of steam, as the links run out. For what they're worth, these
numbers seem to suggests around ~260 distinct email addresses send
patches to threads referenced by commits. Maybe we're in a 3-year
long plateau, but I don't see a peak back in r12:
postgres=# select date_trunc('year', time), count(distinct address)
from t group by 1 order by 1;
date_trunc | count
------------------------+-------
2015-01-01 00:00:00+13 | 13
2016-01-01 00:00:00+13 | 37
2017-01-01 00:00:00+13 | 144
2018-01-01 00:00:00+13 | 187
2019-01-01 00:00:00+13 | 225
2020-01-01 00:00:00+13 | 260
2021-01-01 00:00:00+13 | 256
2022-01-01 00:00:00+13 | 262
2023-01-01 00:00:00+13 | 119
(9 rows)
Of course 2023 is only just getting started. Zooming in closer, the
peak period for this measurement is March/April, as I guess a lot of
little things make it into the final push:
postgres=# select date_trunc('month', time), count(distinct address)
from t where time > '2021-01-01' group by 1 order by 1;
date_trunc | count
------------------------+-------
2021-01-01 00:00:00+13 | 83
2021-02-01 00:00:00+13 | 70
2021-03-01 00:00:00+13 | 100
2021-04-01 00:00:00+13 | 109
2021-05-01 00:00:00+12 | 54
2021-06-01 00:00:00+12 | 82
2021-07-01 00:00:00+12 | 86
2021-08-01 00:00:00+12 | 83
2021-09-01 00:00:00+12 | 73
2021-10-01 00:00:00+13 | 68
2021-11-01 00:00:00+13 | 66
2021-12-01 00:00:00+13 | 48
2022-01-01 00:00:00+13 | 68
2022-02-01 00:00:00+13 | 73
2022-03-01 00:00:00+13 | 110
2022-04-01 00:00:00+13 | 90
2022-05-01 00:00:00+12 | 47
2022-06-01 00:00:00+12 | 50
2022-07-01 00:00:00+12 | 72
2022-08-01 00:00:00+12 | 81
2022-09-01 00:00:00+12 | 105
2022-10-01 00:00:00+13 | 68
2022-11-01 00:00:00+13 | 74
2022-12-01 00:00:00+13 | 58
2023-01-01 00:00:00+13 | 65
2023-02-01 00:00:00+13 | 61
2023-03-01 00:00:00+13 | 64
(27 rows)
Perhaps the present March is looking a little light compared to the
usual 100+ number, but actually if you take just the 1st to the 21st
of previous Marches, they were similar sorts of numbers.
postgres=# select date_trunc('month', time), count(distinct address)
from t
where (time >= '2022-03-01' and time <= '2022-03-21') or
(time >= '2021-03-01' and time <= '2021-03-21') or
(time >= '2020-03-01' and time <= '2020-03-21') or
(time >= '2019-03-01' and time <= '2019-03-21')
group by 1 order by 1;
date_trunc | count
------------------------+-------
2019-03-01 00:00:00+13 | 57
2020-03-01 00:00:00+13 | 57
2021-03-01 00:00:00+13 | 77
2022-03-01 00:00:00+13 | 72
(4 rows)
Another thing we could count is distinct names in the Commitfest app.
I count 162 names in Commitfest 42 today. Unfortunately I don't have
the data to hand to look at earlier Commitfests. That'd be
interesting. I've plotted that before back in 2018 for some
conference talk, and it was at ~100 and climbing back then.
> So I started a conversation about that and some people told me that it's
> very easy to be discouraged by our process. I don't need to mention
> that it's antiquated -- this in itself turns off youngsters. But in
> addition to that, I think newbies might be discouraged because their
> contributions seem to go nowhere even after following the process.
I don't disagree with your sentiment, though.
> This led me to suggesting that perhaps we need to be more lenient when
> it comes to new contributors. As I said, for seasoned contributors,
> it's not a problem to keep up with our requirements, however silly they
> are. But people who spend their evenings a whole week or month trying
> to understand how to patch for one thing that they want, to be received
> by six months of silence followed by a constant influx of "please rebase
> please rebase please rebase", no useful feedback, and termination with
> "eh, you haven't rebased for the 1001th time, your patch has been WoA
> for X days, we're setting it RwF, feel free to return next year" ...
> they are most certainly off-put and will *not* try again next year.
Right, that is pretty discouraging.
Thomas Munro <thomas.munro@gmail.com> writes:
> On Tue, Mar 21, 2023 at 10:59 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>> This led me to suggesting that perhaps we need to be more lenient when
>> it comes to new contributors. As I said, for seasoned contributors,
>> it's not a problem to keep up with our requirements, however silly they
>> are. But people who spend their evenings a whole week or month trying
>> to understand how to patch for one thing that they want, to be received
>> by six months of silence followed by a constant influx of "please rebase
>> please rebase please rebase", no useful feedback, and termination with
>> "eh, you haven't rebased for the 1001th time, your patch has been WoA
>> for X days, we're setting it RwF, feel free to return next year" ...
>> they are most certainly off-put and will *not* try again next year.
> Right, that is pretty discouraging.
It is that. I think that the fundamental problem is that we don't have
enough reviewing/committing manpower to deal with all this stuff in a
timely fashion. That doesn't seem to have an easy fix :-(.
regards, tom lane
On 21.03.23 10:59, Alvaro Herrera wrote: > This led me to suggesting that perhaps we need to be more lenient when > it comes to new contributors. As I said, for seasoned contributors, > it's not a problem to keep up with our requirements, however silly they > are. But people who spend their evenings a whole week or month trying > to understand how to patch for one thing that they want, to be received > by six months of silence followed by a constant influx of "please rebase > please rebase please rebase", no useful feedback, and termination with > "eh, you haven't rebased for the 1001th time, your patch has been WoA > for X days, we're setting it RwF, feel free to return next year" ... > they are most certainly off-put and will*not* try again next year. Personally, if a patch isn't rebased up to the minute doesn't bother me at all. It's easy to check out as of when the email was sent (or extra bonus points for using git format-patch --base). Now, rebasing every month or so is nice, but daily rebases during a commit fest are almost more distracting than just leaving it.
> On 22 Mar 2023, at 10:39, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote: > Personally, if a patch isn't rebased up to the minute doesn't bother me at all. It's easy to check out as of when theemail was sent (or extra bonus points for using git format-patch --base). Now, rebasing every month or so is nice, butdaily rebases during a commit fest are almost more distracting than just leaving it. +1. As long as the patch is rebased and builds/tests green when the CF starts I'm not too worried about not having it always rebased during the CF. If resolving the conflicts are non-trivial/obvious then of course, but if only to stay recent and avoid fuzz in applying then it's more distracting. -- Daniel Gustafsson
So of the patches with no emails since August-December 2022: * New hooks in the connection path * Add log messages when replication slots become active and inactive - Peter Smith and Alvaro Herrera have picked up this one * Remove dead macro exec_subplan_get_plan - Minor cleanup * Consider parallel for LATERAL subqueries having LIMIT/OFFSET - No response since Sept 2022. There was quite a lot of discussion and Tom Lane and Robert Haas expressed some safety concerns which were responded to but I guess people have put in as much time as they can afford on this. I'll mark this Returned with Feedback. * pg_rewind WAL deletion pitfall - I think this is a bug fix to pg_rewind that maybe should be Ready for Committer? * Simplify find_my_exec by using realpath(3) - Tom Lane is author but I don't know if he intends to apply this in this release * Move backup-related code to xlogbackup.c/.h - It looks like neither Alvaro Herrera nor Michael Paquier are particularly convinced this is an improvement and nobody has put more time in this since last October. I'm inclined to mark this Rejected? * Avoid hiding shared filesets in pg_ls_tmpdir (pg_ls_* functions for showing metadata ...) - According to the internet "As part of their 39 month old development and milestones, your patch should be able to see like an adult (20/20 vision), be able to run, walk, hop and balance himself while standing with one foot quite confidently." Can it do all those things yet? - Should this be broken up into smaller CF entries so at least some of them can be Ready for Committer and closed? > * Fix bogus error emitted by pg_recvlogical when interrupted - Is this a minor cleanup? > * Check consistency of GUC defaults between .sample.conf and pg_settings.boot_val - It looks like this was pretty active until last October and might have been ready to apply at least partially? But no further work or review has happened since. > * Code checks for App Devs, using new options for transaction behavior - This is an interesting set of features from Simon Riggs to handle "dry-run" style SQL execution by changing the semantics of BEGIN and END and COMMIT. It has feedback from Erik Rijkers and Dilip Kumar but I don't think it's gotten any serious design discussion. I posted a quick review myself just now but still the point remains. I think features supporting a "dry-run" mode would be great but judging by the lack of response this doesn't look like the version of that people are interested in. I'm inclined to mark this Rejected, even if that's only by default. If someone is interested in running with this in the future maybe Returned with Feedback would be better even there really wasn't much feedback. In practice it amounts to the same thing I think. > * Lockless queue of waiters based on atomic operations for LWLock - Is this going in this CF? It looks like something we don't want to lose though > * Fix assertion failure with next_phase_at in snapbuild.c - It's a bug fix but it doesn't look like the bug has been entirely fixed? > * Add sortsupport for range types and btree_gist - It doesn't l look like anyone has interested in reviewing this patch. It's been bouncing forward from CF to CF since last August. I'm not sure what to do. Maybe we just have to say it's rejected for lack of reviewers interested/competent to review this area of the code. > * asynchronous execution support for Custom Scan - This is a pretty small isolated feature. > * CREATE INDEX CONCURRENTLY on partitioned table - I'm guessing this patch is too big and too late to go in this CF. And it sounds like there's still work to be done? Should this be marked RwF? > * Partial aggregates push down - I'm not sure what the state of this is, it's had about a year and a half of work and seems to have had real work going into it during all that time. It's just a big change. Is it ready for commit or are there still open questions? Is it for this release or next release? > * Non-replayable WAL records through overflows and >MaxAllocSize lengths - Andres says it's a bug fix -- greg
Greg Stark <stark@mit.edu> writes:
> * Simplify find_my_exec by using realpath(3)
> - Tom Lane is author but I don't know if he intends to apply this in
> this release
I'd like to get it done. It's currently stuck because Peter asked
for some behavioral changes that I was dubious about, and we're trying
to come to a mutually-agreeable idea. However ... the only relation
between that issue and what I actually want to do is that it's touching
the same code. Maybe we should put the behavioral-change ideas on the
shelf and just get the realpath() reimplementation done for now.
I'm getting antsy about letting that wait much longer, because it's
entirely possible that there will be exciting portability problems;
waiting till almost feature freeze to find out doesn't seem wise.
>> * Code checks for App Devs, using new options for transaction behavior
> I think features supporting a "dry-run" mode would be great but
> judging by the lack of response this doesn't look like the version of
> that people are interested in.
This version of it seems quite unsafe. I wonder if we could have some
session-level property that says "no changes made by this session will
actually get committed, but it will look to the session as if they did"
(thus fixing the inability to test DDL that you noted). I'm not sure
how well that could work, or what leakiness there might be in the
abstraction. There would probably be locking oddities at the least.
I agree with RwF, or maybe Withdrawn given that Simon's retired?
The same for his other patches --- somebody else will need to push
them forward if anything's to come of them.
regards, tom lane
On Thu, Mar 23, 2023 at 04:41:39PM -0400, Greg Stark wrote: > * Avoid hiding shared filesets in pg_ls_tmpdir (pg_ls_* functions for > showing metadata ...) > - According to the internet "As part of their 39 month old > development and milestones, your patch should be able to see like an > adult (20/20 vision), be able to run, walk, hop and balance himself > while standing with one foot quite confidently." Can it do all those > things yet? It's not a large patch, and if you read the summaries that I've written, you'll see that I've presented it as several patches specifically to allow the essential, early patches to progress; the later patches are optional - I stopped sending them since people were evidently distracted by the optional patches at the exclusion of the essential patches. > - Should this be broken up into smaller CF entries so at least some > of them can be Ready for Committer and closed? Opening and closing CF entries sounds like something which would maximize the administrative overhead of the process rather than progressing the patch. > > * CREATE INDEX CONCURRENTLY on partitioned table > - I'm guessing this patch is too big and too late to go in this CF. > And it sounds like there's still work to be done? Should this be > marked RwF? If you look, you'll see that's it's straightforward and *also* small. As I wrote last week, it's very viable for v16. On Fri, Mar 17, 2023 at 09:56:10AM -0500, Justin Pryzby wrote: > > * CREATE INDEX CONCURRENTLY on partitioned table > > My patch. I think there's agreement that this patch is ready, except > that it's waiting on the bugfix for the progress reporting patch. IDK > if there's interest in this, but it'd be a good candidate for v16. -- Justin
On Fri, Mar 24, 2023 at 5:42 AM Greg Stark <stark@mit.edu> wrote: > > > > * Fix assertion failure with next_phase_at in snapbuild.c > - It's a bug fix but it doesn't look like the bug has been entirely fixed? > We have a patch fixing the issue and reproducible steps. Another bug was reported late in the discussion but it was the same issue as CF item "Assertion failure in SnapBuildInitialSnapshot()". Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
On Thu, Mar 23, 2023 at 04:41:39PM -0400, Greg Stark wrote: > * Move backup-related code to xlogbackup.c/.h > - It looks like neither Alvaro Herrera nor Michael Paquier are > particularly convinced this is an improvement and nobody has put more > time in this since last October. I'm inclined to mark this Rejected? Agreed. >> * Check consistency of GUC defaults between .sample.conf and pg_settings.boot_val > - It looks like this was pretty active until last October and might > have been ready to apply at least partially? But no further work or > review has happened since. FWIW, I don't find much appealing the addition of two GUC flags for only the sole purpose of that, particularly as we get a stronger dependency between GUCs that can be switched dynamically at initialization and at compile-time. >> * Non-replayable WAL records through overflows and >MaxAllocSize lengths > > - Andres says it's a bug fix It is a bug fix. Something I would dare backpatch? Perhaps not. The hardcoded limit of MaxXLogRecordSize makes me feel a bit uncomfortable, though perhaps we could live with that. -- Michael
Attachment
On Fri, Mar 24, 2023 at 10:24:43AM +0900, Michael Paquier wrote: > >> * Check consistency of GUC defaults between .sample.conf and pg_settings.boot_val > > - It looks like this was pretty active until last October and might > > have been ready to apply at least partially? But no further work or > > review has happened since. > > FWIW, I don't find much appealing the addition of two GUC flags for > only the sole purpose of that, The flags seem independently interesting - adding them here follows a suggestion Andres made in response to your complaint. 20220713234900.z4rniuaerkq34s4v@awork3.anarazel.de > particularly as we get a stronger > dependency between GUCs that can be switched dynamically at > initialization and at compile-time. What do you mean by "stronger dependency between GUCs" ? -- Justin
On Fri, Mar 24, 2023 at 5:42 AM Greg Stark <stark@mit.edu> wrote: > > * asynchronous execution support for Custom Scan > - This is a pretty small isolated feature. I was planning to review this patch, but unfortunately, I did not have time for that, and I do not think I will for v16. So if anyone wants to work on this, please do so; if not, I want to in the next development cycle for v17. Best regards, Etsuro Fujita
Status summary: Needs review: 116. Waiting on Author: 30. Ready for Committer: 32. Committed: 94. Moved to next CF: 17. Returned with Feedback: 6. Rejected: 6. Withdrawn: 18. Total: 319. Ok, here are the patches that have been stuck in "Waiting on Author" for a while. I divided them into three groups. * The first group have been stuck for over a week and mostly look like they should be RwF. Some I guess just moved to next CF. But some of them I'm uncertain if I should leave or if they really should be RfC or NR. * The other two groups have had some updates in the last week (actually I used 10 days). But some of them still look like they're pretty much dead for this CF and should either be moved forward or Rwf or Rejected. So here's the triage list. I'm going to send emails and start clearing out the patches pretty much right away. Some of these are pretty clearcut. Nothing in over a week: ---------------------- * Better infrastructure for automated testing of concurrency issues - Consensus that this is desirable. But it's not clear what it's actually waiting on Author for. RwF? * Provide the facility to set binary format output for specific OID's per session - I think Dave was looking for feedback and got it from Tom and Peter. I don't actually see a specific patch here but there are two patches linked in the original message. There seems to be enough feedback to proceed but nobody's working on it. RwF? * pg_visibility's pg_check_visible() yields false positive when working in parallel with autovacuum - Bug, but tentatively a false positive... * CAST( ... ON DEFAULT) - it'll have to wait till there's something solid from the committee" -- Rejected? * Fix tab completion MERGE - Partly committed but v9-0002-psql-Add-PartialMatches-macro-for-better-tab-complet.patch remains. There was a review from Dean Rasheed. Move to next CF? * Fix recovery conflict SIGUSR1 handling - This looks like a suite of bug fixes and looks like it should be Needs Review or Ready for Commit * Prefetch the next tuple's memory during seqscans - David Rowley said it was dependeny on "heapgettup() refactoring" which has been refactored. So is it now Needs Review or Ready for Commit? Is it likely to happen this CF? * Pluggable toaster - This seems to have digressed from the original patch. There were patches early on and a lot of feedback. Is the result that the original patches are Rejected or are they still live? * psql - refactor echo code - "I think this patch requires an up-to-date summary and explanation" from Peter. But it seems like Tom was ok with it and just had some additional improvements he wanted that were added. It sounds like this might be "Ready for Commit" if someone familiar with the patch looked at it. * Push aggregation down to base relations and joins - Needs a significant rebase (since March 1). * Remove self join on a unique column - An offer of a Bounty! There was one failing test which was apparently fixed? But it looks like this should be in Needs Review or Ready for Commit. * Split index and table statistics into different types of stats - Was stuck on "Generate pg_stat_get_xact*() functions with Macros" which was committed. So "Ready for Commit" now? * Default to ICU during initdb - Partly committed, 0001 waiting until after CF * suppressing useless wakeups in logical/worker.c - Got feedback March 17. Doesn't look like it's going to be ready this CF. * explain analyze rows=%.0f - Patch updated January, but I think Tom still had some simple if tedious changes he asked for * Fix order of checking ICU options in initdb and create database - Feedback Last November but no further questions or progress * Introduce array_shuffle() and array_sample() functions - Feedback from Tom last September. No further questions or progress Status Updates in last week: ---------------------------- * Some revises in adding sorting path - Got feedback Feb 21 and author responded but doesn't look like it's going to be ready this CF * Add TAP tests for psql \g piped into program - Peter Eisentraut asked for a one-line change, otherwise it looks like it's Ready for Commit? * Improvements to Meson docs - Some feedback March 15 but no response. I assume this is still in play Emails in last week: ------------------- * RADIUS tests and improvements - last feedback March 20, last patch March 4. Should probably be moved to the next CF unless there's progress soon. * Direct SSL Connections - (This is mine) Code for SSL is pretty finished. The last patch for ALPN support needs a bit of polish. I'll be doing that momentarily. * Fix alter subscription concurrency errors - "patch as-submitted is pretty uninteresting" and "patch that I don't care much about" ... I guess this is Rejected or Withdrawn * Fix improper qual pushdown after applying outer join identity 3 - Tom Lane's patch. Active discussion as of March 21. * Error "initial slot snapshot too large" in create replication slot - Active discussion as of March 24. Is this now Needs Review or Ready for Committer? * Transparent column encryption - Active discussion as of March 24 * Make ON_ERROR_STOP stop on shell script failure - I rebased this but I think it needs a better review. I may have a chance to do that or someone else could. The original author bt22nakamorit@oss.nttdata.com seems to have disappeared but the patch seems to be perhaps committable? * pg_stats and range statistics - Updated patch as of March 24, should be Needs Review I guess? * TDE key management patches - Actively under discussion * Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry - Actively under discussion -- Gregory Stark As Commitfest Manager
Hi, > * Pluggable toaster > - This seems to have digressed from the original patch. There were > patches early on and a lot of feedback. Is the result that the > original patches are Rejected or are they still live? We agreed to work on a completely new RFC which is currently discussed within the "Compression dictionaries" thread, see [1][2] and below. I guess it means either Rejected or RwF. [1]: https://www.postgresql.org/message-id/20230203095540.zutul5vmsbmantbm%40alvherre.pgsql [2]: https://www.postgresql.org/message-id/20230203095658.imkcw2sypawe3py3%40alvherre.pgsql -- Best regards, Aleksander Alekseev
Only a few more days remain before feature freeze. We've now crossed over 100 patches committed according to the CF app: Status summary: March 15 March 22 March 28 April 4 Needs review: 152 128 116 96 Waiting on Author: 42 36 30 21 Ready for Committer: 39 32 32 35 Committed: 61 82 94 101 Moved to next CF: 4 15 17 28 Withdrawn: 17 16 18 20 Rejected: 0 5 6 8 Returned with Feedback: 4 5 6 10 Total: 319. Perhaps more importantly we've crossed *under* 100 patches waiting for review. However I tried to do a pass of the Needs Review patches and found that a lot of them were really waiting for comment and seemed to be useful features or bug fixes that already had a significant amount of work put into them and seemed likely to get committed if there was time available to work on them. There seems to be a bit of a mix of either a) patches that just never got any feedback -- in some cases presumably because the patch required special competency in a niche area or b) patches that had active discussion and patches being updated until discussion died out. Presumably because the author either got busy elsewhere or perhaps the discussion seemed unproductive and exhausted them. What I didn't see, that I expected to see, was patches that were just uninteresting to anyone other than the author but that people were just too polite to reject. So I think these patches are actual useful patches that we would want to have but are likely, modulo some bug fixes, to get moved along to the next CF again without any progress this CF: * Remove dead macro exec_subplan_get_plan * pg_rewind WAL deletion pitfall * Avoid hiding shared filesets in pg_ls_tmpdir (pg_ls_* functions for showing metadata ...) * Fix bogus error emitted by pg_recvlogical when interrupted * Lockless queue of waiters based on atomic operations for LWLock * Add sortsupport for range types and btree_gist * Enable jitlink as an alternative jit linker of legacy Rtdyld and add riscv jitting support * basebackup: support zstd long distance matching * Function to log backtrace of postgres processes * More scalable multixacts buffers and locking * COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns * Add semi-join pushdown to postgres_fdw * Skip replicating the tables specified in except table option * Post-special Page Storage TDE support * Direct I/O (developer-only feature) * Improve doc for autovacuum on partitioned tables * An attempt to avoid locally-committed-but-not-replicated-to-standby-transactions in synchronous replication * Check lateral references within PHVs for memoize cache keys * monitoring usage count distribution * New [relation] options engine * nbtree performance improvements through specialization on key shape * Fix assertion failure in SnapBuildInitialSnapshot() * Speed up releasing of locks * Improve pg_bsd_indent's handling of multiline initialization expressions * Refactoring postgres_fdw/connection.c * Add pg_stat_session * archive modules loose ends * Fix dsa_free() to re-bin segment * Reduce timing overhead of EXPLAIN ANALYZE using rdtsc * clean up permission checks after 599b33b94 * Fix the description of GUC "max_locks_per_transaction" and "max_pred_locks_per_transaction" in guc_table.c * some namespace.c refactoring * Add function to_oct * Switching XLog source from archive to streaming when primary available * BRIN - SK_SEARCHARRAY and scan key preprocessing * Reuse Workers and Replication Slots during Logical Replication
"Gregory Stark (as CFM)" <stark.cfm@gmail.com> writes:
> However I tried to do a pass of the Needs Review patches and found
> that a lot of them were really waiting for comment and seemed to be
> useful features or bug fixes that already had a significant amount of
> work put into them and seemed likely to get committed if there was
> time available to work on them.
Yeah, we just don't have enough people ...
> So I think these patches are actual useful patches that we would want
> to have but are likely, modulo some bug fixes, to get moved along to
> the next CF again without any progress this CF:
I have comments on a few of these:
> * Remove dead macro exec_subplan_get_plan
TBH, I'd reject this one as not being worth the trouble.
> * monitoring usage count distribution
And I'm dubious how many people care about this, either.
> * Improve pg_bsd_indent's handling of multiline initialization expressions
This is going to go in once the commit fest is done; we're just holding
off to avoid creating merge issues during the CF time crunch.
> * clean up permission checks after 599b33b94
I believe that the actual bug fixes are in, and what's left is just a test
case that people weren't very excited about adding. So maybe this should
get closed out as committed.
Perhaps we'll get some of the others done by the end of the week.
regards, tom lane
On Tue, 4 Apr 2023 at 11:18, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > * clean up permission checks after 599b33b94 > > I believe that the actual bug fixes are in, and what's left is just a test > case that people weren't very excited about adding. So maybe this should > get closed out as committed. I'm not super convinced about this one. I'm not a big "all tests are good tests" believer but this test seems like a pretty reasonable one. Permissions checks and user mappings are user-visible behaviour that are easy to overlook when making changes with unexpected side effects. It seems like the test would be just as easy to commit as to not commit and I don't see anything tricky about it that would necessitate a more in depth review. -- greg
> On 4 Apr 2023, at 20:36, Greg Stark <stark@mit.edu> wrote: > > On Tue, 4 Apr 2023 at 11:18, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> >>> * clean up permission checks after 599b33b94 >> >> I believe that the actual bug fixes are in, and what's left is just a test >> case that people weren't very excited about adding. So maybe this should >> get closed out as committed. > > I'm not super convinced about this one. I'm not a big "all tests are > good tests" believer but this test seems like a pretty reasonable one. > Permissions checks and user mappings are user-visible behaviour that > are easy to overlook when making changes with unexpected side effects. > > It seems like the test would be just as easy to commit as to not > commit and I don't see anything tricky about it that would necessitate > a more in depth review. Agreed, I think this test has value and don't see a strong reason not to commit it. -- Daniel Gustafsson
As announced on this list feature freeze is at 00:00 April 8 AoE. That's less than 24 hours away. If you need to set your watches to AoE timezone it's currently: $ TZ=AOE+12 date Fri 07 Apr 2023 02:05:50 AM AOE As we stand we have: Status summary: Needs review: 82 Waiting on Author: 16 Ready for Committer: 27 Committed: 115 Moved to next CF: 38 Returned with Feedback: 10 Rejected: 9 Withdrawn: 22 Total: 319. In less than 24h most of the remaining patches will get rolled forward to the next CF. The 16 that are Waiting on Author might be RwF perhaps. The only exceptions would be non-features like Bug Fixes and cleanup patches that have been intentionally held until the end -- those become Open Issues for the release. So if we move forward all the remaining patches (so these numbers are high by about half a dozen) the *next* CF would look like: Commitfest 2023-07: Now April 8 Needs review: 46. 128 Waiting on Author: 17. 33 Ready for Committer: 3. 30 Total: 66 191 I suppose that's better than the 319 we came into this CF with but there's 3 months to accumulate more unreviewed patches... I had hoped to find lots of patches that I could bring the hammer down on and say there's just no interest in or there's no author still maintaining. But that wasn't the case. Nearly all the patches still had actively interested authors and looked like they were legitimately interesting and worthwhile features that people just haven't had the time to review or commit. -- greg
On Fri, Apr 7, 2023 at 10:21 AM Greg Stark <stark@mit.edu> wrote:
As announced on this list feature freeze is at 00:00 April 8 AoE.
That's less than 24 hours away. If you need to set your watches to AoE
timezone it's currently:
$ TZ=AOE+12 date
Fri 07 Apr 2023 02:05:50 AM AOE
As we stand we have:
Status summary:
Needs review: 82
Waiting on Author: 16
Ready for Committer: 27
Committed: 115
Moved to next CF: 38
Returned with Feedback: 10
Rejected: 9
Withdrawn: 22
Total: 319.
In less than 24h most of the remaining patches will get rolled forward
to the next CF. The 16 that are Waiting on Author might be RwF
perhaps. The only exceptions would be non-features like Bug Fixes and
cleanup patches that have been intentionally held until the end --
those become Open Issues for the release.
So if we move forward all the remaining patches (so these numbers are
high by about half a dozen) the *next* CF would look like:
Commitfest 2023-07: Now April 8
Needs review: 46. 128
Waiting on Author: 17. 33
Ready for Committer: 3. 30
Total: 66 191
I suppose that's better than the 319 we came into this CF with but
there's 3 months to accumulate more unreviewed patches...
I had hoped to find lots of patches that I could bring the hammer down
on and say there's just no interest in or there's no author still
maintaining. But that wasn't the case. Nearly all the patches still
had actively interested authors and looked like they were legitimately
interesting and worthwhile features that people just haven't had the
time to review or commit.
--
greg
The %T added to the PSQL Prompt is about 5 lines of code. Reviewed and Ready to commit.
That could knock one more off really quickly :-)
Excellent work to everyone.
Thanks, Kirk
Kirk Wolak <wolakk@gmail.com> writes:
> The %T added to the PSQL Prompt is about 5 lines of code. Reviewed and
> Ready to commit.
> That could knock one more off really quickly :-)
I'm still objecting to it, for the same reason as before.
regards, tom lane
On Fri, Apr 7, 2023 at 6:29 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Kirk Wolak <wolakk@gmail.com> writes:
> The %T added to the PSQL Prompt is about 5 lines of code. Reviewed and
> Ready to commit.
> That could knock one more off really quickly :-)
I'm still objecting to it, for the same reason as before.
regards, tom lane
Tom,
I got no response to my point that the backquote solution is cumbersome because I have to use psql in both windows
and in linux environments (realizing I am the odd duck in this group). But my fall back was a common script file. Then I shared my
psqlrc file with a co-worker, and they ran into the missing script file. [ie, the same command does not work in both systems].
I won't argue beyond this point, I'd just like to hear that you considered this final point...
and I can move on.
Thanks, Kirk
So here we are at the end of the CF: Status summary: March 15 March 22 March 28 April 4 April 8 Needs review: 152 128 116 96 74 Waiting on Author: 42 36 30 21 14 Ready for Committer: 39 32 32 35 27 Committed: 61 82 94 101 124 Moved to next CF: 4 15 17 28 39 Withdrawn: 17 16 18 20 10 Rejected: 0 5 6 8 9 Returned with Feedback: 4 5 6 10 22 Total: 319. I'm now going to go through and: a) Mark Waiting on Author any patches that aren't building Waiting on Author. There was some pushback about asking authors to do trivial rebases but in three months it won't make sense to start a CF with already-non-applying patches. b) Mark any patches that received solid feedback in the last week with either Returned with Feedback or Rejected. I think this was already done though with 12 RwF and 1 Rejected in the past four days alone. c) Pick out the Bug Fixes, cleanup patches, and other non-feature patches that might be open issues for v16. d) Move to Next CF any patches that remain. -- greg
On Sat, 8 Apr 2023 at 11:37, Greg Stark <stark@mit.edu> wrote: > > c) Pick out the Bug Fixes, cleanup patches, and other non-feature > patches that might be open issues for v16. So on further examination it seems there are multiple category of patches that are worth holding onto rather than shifting to the next release: * Bug Fixes * Documentation patches * Build system patches, especially meson-related patches * Testing patches, especially if they're testing new features * Patches that are altering new features that were committed in this release Some of these, especially the last category, are challenging for me to determine. If I move forward a patch of yours that you think makes sense to treat as an open issue that should be resolved in this release then feel free to say. -- Gregory Stark As Commitfest Manager
"Gregory Stark (as CFM)" <stark.cfm@gmail.com> writes: > Some of these, especially the last category, are challenging for me to > determine. If I move forward a patch of yours that you think makes > sense to treat as an open issue that should be resolved in this > release then feel free to say. I think that's largely independent. We don't look back at closed-out CFs as a kind of TODO list; anything that's left behind there is basically never going to be seen again, until the author makes a new CF entry. Anything that's to be treated as an open item for v16 should get added to the wiki page at https://wiki.postgresql.org/wiki/PostgreSQL_16_Open_Items It's not necessary that a patch exist to do that. regards, tom lane
On Fri, Apr 07, 2023 at 10:40:01PM -0400, Kirk Wolak wrote: > I got no response to my point that the backquote solution is cumbersome > because I have to use* psql in both windows* > *and in linux environments* (realizing I am the odd duck in this group). > But my fall back was a common script file. Then I shared my > psqlrc file with a co-worker, and they ran into the missing script file. > [ie, the same command does not work in both systems]. > > I won't argue beyond this point, I'd just like to hear that you > considered this final point... > and I can move on. FYI, this specific patch has been moved to the next commit fest of 2023-07: https://commitfest.postgresql.org/43/4227/ This implies that this discussion will be considered for the development cycle of v17, planned to begin in July. -- Michael
Attachment
On Sat, Apr 08, 2023 at 09:50:49PM -0400, Tom Lane wrote: > I think that's largely independent. We don't look back at closed-out CFs > as a kind of TODO list; anything that's left behind there is basically > never going to be seen again, until the author makes a new CF entry. > > Anything that's to be treated as an open item for v16 should get added > to the wiki page at > > https://wiki.postgresql.org/wiki/PostgreSQL_16_Open_Items > > It's not necessary that a patch exist to do that. And patches that are marked as bug fixes in the CF app had better not be lost in the long run, even if they are not listed as an open item as a live issue. Sometimes people apply "Bug Fix" as a category which would imply a backpatch in most cases, and looking at the patch proves that this can be wrong. -- Michael
Attachment
So here's the list of CF entries that I thought *might* still get committed either because they're an Open Issue or they're one of those other categories. I had intended to go through and add them all to the Open Issues but it turns out I only feel confident in a couple of them qualifying for that: Already added: * Default to ICU during initdb * Assertion failure with parallel full hash join * RecoveryConflictInterrupt() is unsafe in a signal handler * pg_visibility's pg_check_visible() yields false positive when working in parallel with autovacuum Not added: * Fix bogus error emitted by pg_recvlogical when interrupted * clean up permission checks after 599b33b94 * Fix assertion failure with next_phase_at in snapbuild.c * Fix assertion failure in SnapBuildInitialSnapshot() * Fix improper qual pushdown after applying outer join identity 3 * Add document is_superuser * Improve doc for autovacuum on partitioned tables * Create visible links for HTML elements that have an id to make them discoverable via the web interface * Fix inconsistency in reporting checkpointer stats * pg_rewind WAL deletion pitfall * Update relfrozenxmin when truncating temp tables * Testing autovacuum wraparound * Add TAP tests for psql \g piped into program I'll move these CF entries to the next CF now. I think they all are arguably open issues though of varying severity. -- greg
Greg Stark <stark@mit.edu> writes:
> Not added:
> * Fix improper qual pushdown after applying outer join identity 3
I already made an open item for that one.
regards, tom lane
Hi, After catching up with this thread, where pending bugs are listed and discussed, I wonder if the current patches trying to lower the HashJoin memory explosion[1] could be added to the "Older bugs affecting stable branches" list of https://wiki.postgresql.org/wiki/PostgreSQL_16_Open_Items as I think they deserve some discussion/triage for v16? The patch as it stands is not invasive. As I wrote previously[2], if we couldn't handle such situation better in v16, and if this patch is not backpatch-able in a minor release, then we will keep living another year, maybe more, with this bad memory behavior. Thank you, [1] https://www.postgresql.org/message-id/20230408020119.32a0841b%40karst [2] https://www.postgresql.org/message-id/20230320151234.38b2235e%40karst
Jehan-Guillaume de Rorthais <jgdr@dalibo.com> writes: > After catching up with this thread, where pending bugs are listed and discussed, > I wonder if the current patches trying to lower the HashJoin memory explosion[1] > could be added to the "Older bugs affecting stable branches" list of > https://wiki.postgresql.org/wiki/PostgreSQL_16_Open_Items as I think they > deserve some discussion/triage for v16? They do not. That patch is clearly nowhere near ready to commit, and even if it was, I don't think we'd consider it post-feature-freeze. Any improvement in this space would be a feature, not a bug fix, despite anyone's attempts to label it a bug fix. regards, tom lane
On Fri, Apr 21, 2023 at 9:50 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Jehan-Guillaume de Rorthais <jgdr@dalibo.com> writes: > > After catching up with this thread, where pending bugs are listed and discussed, > > I wonder if the current patches trying to lower the HashJoin memory explosion[1] > > could be added to the "Older bugs affecting stable branches" list of > > https://wiki.postgresql.org/wiki/PostgreSQL_16_Open_Items as I think they > > deserve some discussion/triage for v16? > > They do not. That patch is clearly nowhere near ready to commit, and > even if it was, I don't think we'd consider it post-feature-freeze. > Any improvement in this space would be a feature, not a bug fix, > despite anyone's attempts to label it a bug fix. So, I think this may be a bit harsh. The second patch in the set only moves hash join batch buffile creation into a more granular memory context to make it easier to identify instances of this bug (which causes OOMs). It is missing a parallel hash join implementation and a bit more review. But it is not changing any behavior. If using a separate memory context solely for the purpose of accounting is considered an anti-pattern, we could use some arithmetic like hash_agg_update_metrics() to calculate how much space is taken up by these temporary file buffers. Ultimately, either method is a relatively small change (both LOC and impact AFAICT). Currently, it isn't possible for a user to understand what is consuming so much memory when hash join batch file buffers substantially exceed the size of the actual hashtable. This memory usage is not displayed in EXPLAIN ANALYZE or anywhere else. I think adding a debugging message with some advice for is a reasonable concession to the user. This may not constitute a bug "fix", but I don't really see how this is a feature. - Melanie
On Fri, Apr 21, 2023 at 12:49 PM Melanie Plageman <melanieplageman@gmail.com> wrote: > If using a separate memory context solely for the purpose of accounting > is considered an anti-pattern, ... This thread isn't the right place to argue about the merits of that patch, at least IMHO, but I don't think that's an anti-pattern. If we need to keep track of how much memory is being used, it sure sounds like a better idea to use a memory context for that than to invent some bespoke infrastructure. -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Apr 21, 2023 at 12:49 PM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
>> If using a separate memory context solely for the purpose of accounting
>> is considered an anti-pattern, ...
> This thread isn't the right place to argue about the merits of that
> patch, at least IMHO, but I don't think that's an anti-pattern.
I didn't say that either. If the proposal is to apply only that change,
I could be on board with doing that post-feature-freeze. I just don't
think the parts of the patch that change the memory management heuristics
are ready.
regards, tom lane