public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v36 1/6] Introduce RelOptInfo->notnullattrs attribute
21+ messages / 15 participants
[nested] [flat]
* [PATCH v36 1/6] Introduce RelOptInfo->notnullattrs attribute
@ 2020-05-03 14:37 一挃 <[email protected]>
0 siblings, 0 replies; 21+ messages in thread
From: 一挃 @ 2020-05-03 14:37 UTC (permalink / raw)
The notnullattrs is calculated from catalog and run-time query. That
infomation is translated to child relation as well for partitioned
table.
---
src/backend/optimizer/path/allpaths.c | 31 ++++++++++++++++++++++++++
src/backend/optimizer/plan/initsplan.c | 10 +++++++++
src/backend/optimizer/util/plancat.c | 10 +++++++++
src/include/nodes/pathnodes.h | 2 ++
4 files changed, 53 insertions(+)
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 6da0dcd61c..484dab0a1a 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -1005,6 +1005,7 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
RelOptInfo *childrel;
ListCell *parentvars;
ListCell *childvars;
+ int i = -1;
/* append_rel_list contains all append rels; ignore others */
if (appinfo->parent_relid != parentRTindex)
@@ -1061,6 +1062,36 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
(Node *) rel->reltarget->exprs,
1, &appinfo);
+ /* Copy notnullattrs. */
+ while ((i = bms_next_member(rel->notnullattrs, i)) > 0)
+ {
+ AttrNumber attno = i + FirstLowInvalidHeapAttributeNumber;
+ AttrNumber child_attno;
+ if (attno == 0)
+ {
+ /* Whole row is not null, so must be same for child */
+ childrel->notnullattrs = bms_add_member(childrel->notnullattrs,
+ attno - FirstLowInvalidHeapAttributeNumber);
+ break;
+ }
+ if (attno < 0 )
+ /* no need to translate system column */
+ child_attno = attno;
+ else
+ {
+ Node * node = list_nth(appinfo->translated_vars, attno - 1);
+ if (!IsA(node, Var))
+ /* This may happens at UNION case, like (SELECT a FROM t1 UNION SELECT a + 3
+ * FROM t2) t and we know t.a is not null
+ */
+ continue;
+ child_attno = castNode(Var, node)->varattno;
+ }
+
+ childrel->notnullattrs = bms_add_member(childrel->notnullattrs,
+ child_attno - FirstLowInvalidHeapAttributeNumber);
+ }
+
/*
* We have to make child entries in the EquivalenceClass data
* structures as well. This is needed either if the parent
diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c
index e978b491f6..95b1b14cd3 100644
--- a/src/backend/optimizer/plan/initsplan.c
+++ b/src/backend/optimizer/plan/initsplan.c
@@ -830,6 +830,16 @@ deconstruct_recurse(PlannerInfo *root, Node *jtnode, bool below_outer_join,
{
Node *qual = (Node *) lfirst(l);
+ /* Set the not null info now */
+ ListCell *lc;
+ List *non_nullable_vars = find_nonnullable_vars(qual);
+ foreach(lc, non_nullable_vars)
+ {
+ Var *var = lfirst_node(Var, lc);
+ RelOptInfo *rel = root->simple_rel_array[var->varno];
+ rel->notnullattrs = bms_add_member(rel->notnullattrs,
+ var->varattno - FirstLowInvalidHeapAttributeNumber);
+ }
distribute_qual_to_rels(root, qual,
false, below_outer_join, JOIN_INNER,
root->qual_security_level,
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index 25545029d7..0b2f9d398a 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -117,6 +117,7 @@ get_relation_info(PlannerInfo *root, Oid relationObjectId, bool inhparent,
Relation relation;
bool hasindex;
List *indexinfos = NIL;
+ int i;
/*
* We need not lock the relation since it was already locked, either by
@@ -463,6 +464,15 @@ get_relation_info(PlannerInfo *root, Oid relationObjectId, bool inhparent,
if (inhparent && relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
set_relation_partition_info(root, rel, relation);
+ Assert(rel->notnullattrs == NULL);
+ for(i = 0; i < relation->rd_att->natts; i++)
+ {
+ FormData_pg_attribute attr = relation->rd_att->attrs[i];
+ if (attr.attnotnull)
+ rel->notnullattrs = bms_add_member(rel->notnullattrs,
+ attr.attnum - FirstLowInvalidHeapAttributeNumber);
+ }
+
table_close(relation, NoLock);
/*
diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h
index 485d1b06c9..9e3ebd488a 100644
--- a/src/include/nodes/pathnodes.h
+++ b/src/include/nodes/pathnodes.h
@@ -709,6 +709,8 @@ typedef struct RelOptInfo
PlannerInfo *subroot; /* if subquery */
List *subplan_params; /* if subquery */
int rel_parallel_workers; /* wanted number of parallel workers */
+ /* Not null attrs, start from -FirstLowInvalidHeapAttributeNumber */
+ Bitmapset *notnullattrs;
/* Information about foreign tables and foreign joins */
Oid serverid; /* identifies server for the table or join */
--
2.21.0
--op7mf2i72xk5yu7c
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v36-0002-Introduce-UniqueKey-attributes-on-RelOptInfo-str.patch"
^ permalink raw reply [nested|flat] 21+ messages in thread
* commitfest.postgresql.org is no longer fit for purpose
@ 2024-05-16 18:30 Robert Haas <[email protected]>
2024-05-16 19:13 ` Re: commitfest.postgresql.org is no longer fit for purpose David G. Johnston <[email protected]>
2024-05-16 19:25 ` Re: commitfest.postgresql.org is no longer fit for purpose Maciek Sakrejda <[email protected]>
2024-05-16 19:30 ` Re: commitfest.postgresql.org is no longer fit for purpose Daniel Gustafsson <[email protected]>
2024-05-16 20:46 ` Re: commitfest.postgresql.org is no longer fit for purpose Melanie Plageman <[email protected]>
2024-05-19 09:37 ` Re: commitfest.postgresql.org is no longer fit for purpose Dmitry Dolgov <[email protected]>
0 siblings, 5 replies; 21+ messages in thread
From: Robert Haas @ 2024-05-16 18:30 UTC (permalink / raw)
To: pgsql-hackers
Hi,
The original intent of CommitFests, and of commitfest.postgresql.org
by extension, was to provide a place where patches could be registered
to indicate that they needed to be reviewed, thus enabling patch
authors and patch reviewers to find each other in a reasonably
efficient way. I don't think it's working any more. I spent a good
deal of time going through the CommitFest this week, and I didn't get
through a very large percentage of it, and what I found is that the
status of the patches registered there is often much messier than can
be captured by a simple "Needs Review" or "Waiting on Author," and the
number of patches that are actually in need of review is not all that
large. For example, there are:
- patches parked there by a committer who will almost certainly do
something about them after we branch
- patches parked there by a committer who probably won't do something
about them after we branch, but maybe they will, or maybe somebody
else will, and anyway this way we at least run CI
- patches parked there by a committer who may well do something about
them before we even branch, because they're not actually subject to
the feature freeze
- patches that we've said we don't want but the author thinks we do
(sometimes i agree with the author, sometimes not)
- patches that have long-unresolved difficulties which the author
either doesn't know how to solve or is in no hurry to solve
- patches that have already been reviewed by multiple people, often
including several committers, and which have been updated multiple
times, but for one reason or another, not committed
- patches that actually do need to be reviewed
What's a bit depressing is that this last category is a relatively
small percentage of the total. If you'd like to sit down and review a
bunch of patches, you'll probably spend AT LEAST as much time trying
to identify which CommitFest entries are worth your time as you will
actually reviewing. I suspect you could easily spend 2 or 3 times as
much time finding things to review as actually reviewing them,
honestly. And the chances that you're going to find the things to
review that most need your attention are pretty much nil. You could
happen just by chance to discover a patch that was worth weeks of your
time to review, but you could also miss that patch forever amidst all
the clutter.
I think there are a couple of things that have led to this state of
affairs. First, we got tired of making people mad by booting their
stuff out of the CommitFest, so we mostly just stopped doing it, even
if it had 0% chance of being committed this CommitFest, and really
even if it had a 0% chance of being committed ever. Second, we added
CI, which means that there is positive value to registering the patch
in the CommitFest even if committing it is not in the cards. And those
things together have created a third problem, which is that the list
is now so long and so messy that even the CommitFest managers probably
don't manage to go through the whole thing thoroughly in a month.
So, our CommitFest application has turned into a patch tracker. IMHO,
patch trackers intrinsically tend to suck, because they fill up with
garbage that nobody cares about, and nobody wants to do the colossal
amount of work that it takes to maintain them. But our patch tracker
sucks MORE, because it's not even intended to BE a general-purpose
patch tracker. I'm not saying that replacing it with (let me show how
old I am) bugzilla or whatever the hip modern equivalent of that may
be these days is the right thing to do, but it's probably worth
considering. If we decide to roll our own, that might be OK too, but
we have to come up with some way of organizing this stuff that's
better than what we have today, some way that actually lets you find
the stuff that you care about.
To give just one example that I think highlights the issues pretty
well, consider the "Refactoring" section of the current CommitFest.
There are 24 patches in there, and 13 of them are by committers. Now,
maybe some of those patches are things that the committer really wants
someone else to review, e.g.
https://commitfest.postgresql.org/48/3998/ seems like it might be
that. On the other hand, that one could also just be an idea Thomas
had that he doesn't really intend to pursue even if the reviews are
absolutely glowing, so maybe it's not worth spending time on after
all. Then there are things that are probably 100% likely to get
committed unless somebody objects, so I shouldn't bother looking at
them unless I want to object, e.g.
https://commitfest.postgresql.org/48/4939/ seems like it's probably
that. And, also, regardless of authorship, some of these patches have
already had a great deal of discussion, and some have had none, and
you can sort of tell that from looking at the time the patch was
created vs. the last activity, but it's really not that obvious. So
overall it's just really unclear where to spend time.
I wonder what ideas people have for improving this situation. I doubt
that there's any easy answer that just makes the problem go away --
keeping large groups of people organized is a tremendously difficult
task under pretty much all circumstances, and the fact that, in this
context, nobody's really the boss, makes it a whole lot harder. But I
also feel like what we're doing right now can't possibly be the best
that we can do.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: commitfest.postgresql.org is no longer fit for purpose
2024-05-16 18:30 commitfest.postgresql.org is no longer fit for purpose Robert Haas <[email protected]>
@ 2024-05-16 19:13 ` David G. Johnston <[email protected]>
2024-05-16 20:50 ` Re: commitfest.postgresql.org is no longer fit for purpose Jacob Champion <[email protected]>
4 siblings, 1 reply; 21+ messages in thread
From: David G. Johnston @ 2024-05-16 19:13 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: pgsql-hackers
On Thu, May 16, 2024 at 11:30 AM Robert Haas <[email protected]> wrote:
> Hi,
>
> The original intent of CommitFests, and of commitfest.postgresql.org
> by extension, was to provide a place where patches could be registered
> to indicate that they needed to be reviewed, thus enabling patch
> authors and patch reviewers to find each other in a reasonably
> efficient way. I don't think it's working any more. I spent a good
> deal of time going through the CommitFest this week, and I didn't get
> through a very large percentage of it, and what I found is that the
> status of the patches registered there is often much messier than can
> be captured by a simple "Needs Review" or "Waiting on Author," and the
> number of patches that are actually in need of review is not all that
> large. For example, there are:
>
> - patches parked there by a committer who will almost certainly do
> something about them after we branch
> - patches parked there by a committer who probably won't do something
> about them after we branch, but maybe they will, or maybe somebody
> else will, and anyway this way we at least run CI
> - patches parked there by a committer who may well do something about
> them before we even branch, because they're not actually subject to
> the feature freeze
>
If a committer has a patch in the CF that is going to be committed in the
future unless there is outside action those should be put under a "Pending
Commit" status.
- patches that we've said we don't want but the author thinks we do
> (sometimes i agree with the author, sometimes not)
> - patches that have long-unresolved difficulties which the author
> either doesn't know how to solve or is in no hurry to solve
> - patches that have already been reviewed by multiple people, often
> including several committers, and which have been updated multiple
> times, but for one reason or another, not committed
>
Use the same software but a different endpoint - Collaboration. Or even
the same endpoint just add an always open slot named "Work In Process"
(WIP). If items can be moved from there to another open or future
commitfest slot so much the better.
- patches that actually do need to be reviewed
>
If we can distinguish between needs to be reviewed by a committer
(commitfest dated slots - bimonthlies) and reviewed by someone other than
the author (work in process slot) that should help everyone. Of course,
anyone is welcome to review a patch that has been marked ready to commit.
I suppose "ready to commit" already sorta does this without the need for
WIP but a quick sanity check would be that ready to commit shouldn't (not
mustn't) be seen in WIP and needs review shouldn't be seen in the
bimonthlies. A needs review in WIP means that the patch has been seen by a
committer and sent back for more work but that the scope and intent are
such that it will make it into the upcoming major release. Or is something
like a bug fix that just goes right into the bimonthly instead of starting
out as a WIP item.
> I think there are a couple of things that have led to this state of
> affairs. First, we got tired of making people mad by booting their
> stuff out of the CommitFest, so we mostly just stopped doing it, even
> if it had 0% chance of being committed this CommitFest, and really
> even if it had a 0% chance of being committed ever.
Those likely never get out of the new WIP slot discussed above. Your patch
tracker basically. And there should be less angst in moving something in
the bimonthly into WIP rather than dropping it outright. There is
discussion to be had regarding WIP/patch tracking should we go this
direction but even if it is just movIng clutter from one room to another
there seems like a clear benefit and need to tighten up what it means to be
in the bimonthly slot - to have qualifications laid out for a patch to earn
its way there, either by effort (authored and reviewed) or need (i.e., bug
fixes), or, realistically, being authored by a committer and being mostly
trivial in nature.
> Second, we added
> CI, which means that there is positive value to registering the patch
> in the CommitFest even if committing it is not in the cards.
The new slot retains this benefit.
And those
> things together have created a third problem, which is that the list
> is now so long and so messy that even the CommitFest managers probably
> don't manage to go through the whole thing thoroughly in a month.
>
The new slot wouldn't be subject to this.
We'll still have a problem with too many WIP patches and not enough ability
or desire to resolve them. But setting a higher bar to get onto the
bimonthly slot while still providing a place for collaboration is a step
forward that configuring technology can help with. As for WIP, maybe
adding thumbs-up and thumbs-down support tracking widgets will help draw
attention to more desired things.
David J.
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: commitfest.postgresql.org is no longer fit for purpose
2024-05-16 18:30 commitfest.postgresql.org is no longer fit for purpose Robert Haas <[email protected]>
2024-05-16 19:13 ` Re: commitfest.postgresql.org is no longer fit for purpose David G. Johnston <[email protected]>
@ 2024-05-16 20:50 ` Jacob Champion <[email protected]>
0 siblings, 0 replies; 21+ messages in thread
From: Jacob Champion @ 2024-05-16 20:50 UTC (permalink / raw)
To: David G. Johnston <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers
On Thu, May 16, 2024 at 12:14 PM David G. Johnston
<[email protected]> wrote:
> Those likely never get out of the new WIP slot discussed above. Your patch tracker basically. And there should be less angst in moving something in the bimonthly into WIP rather than dropping it outright. There is discussion to be had regarding WIP/patch tracking should we go this direction but even if it is just movIng clutter from one room to another there seems like a clear benefit
Yeah, IMO we're unlikely to get around the fact that it's a patch
tracker -- even if patch trackers inherently tend to suck, as Robert
put it, they tend to have lots of value too. May as well embrace the
need for a tracker and make it more helpful.
> We'll still have a problem with too many WIP patches and not enough ability or desire to resolve them. But setting a higher bar to get onto the bimonthly slot while still providing a place for collaboration is a step forward that configuring technology can help with.
+1. I think _any_ way to better communicate "what the author needs
right now" would help a lot.
> As for WIP, maybe adding thumbs-up and thumbs-down support tracking widgets will help draw attention to more desired things.
Personally I'd like a way to gauge general interest without
introducing a voting system. "Stars", if you will, rather than
"thumbs". In the context of the CF, it's valuable to me as an author
that you care about what I'm trying to do; if you don't like my
implementation, that's what reviews on the list are for. And I see no
way that the meaning of a thumbs-down button wouldn't degrade
immediately.
I have noticed that past proposals for incremental changes to the CF
app (mine and others') are met with a sort of resigned inertia --
sometimes disagreement, but more often "meh, sounds all right, maybe".
Maybe my suggestions are just meh, and that's fine. But if we can't
tweak small things as we go -- and be generally okay with trying and
reverting failed experiments sometimes -- frustrations are likely to
pile up until someone writes another biyearly manifesto thread.
--Jacob
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: commitfest.postgresql.org is no longer fit for purpose
2024-05-16 18:30 commitfest.postgresql.org is no longer fit for purpose Robert Haas <[email protected]>
@ 2024-05-16 19:25 ` Maciek Sakrejda <[email protected]>
2024-05-17 07:19 ` Re: commitfest.postgresql.org is no longer fit for purpose Yasir <[email protected]>
4 siblings, 1 reply; 21+ messages in thread
From: Maciek Sakrejda @ 2024-05-16 19:25 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: pgsql-hackers
Thanks for raising this. As someone who is only modestly familiar with
Postgres internals or even C, but would still like to contribute through
review, I find the current process of finding a suitable patch both tedious
and daunting. The Reviewing a Patch article on the wiki [0] says reviews
like mine are still welcome, but it's hard to get started. I'd love to see
this be more approachable.
Thanks,
Maciek
[0]: https://wiki.postgresql.org/wiki/Reviewing_a_Patch
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: commitfest.postgresql.org is no longer fit for purpose
2024-05-16 18:30 commitfest.postgresql.org is no longer fit for purpose Robert Haas <[email protected]>
2024-05-16 19:25 ` Re: commitfest.postgresql.org is no longer fit for purpose Maciek Sakrejda <[email protected]>
@ 2024-05-17 07:19 ` Yasir <[email protected]>
0 siblings, 0 replies; 21+ messages in thread
From: Yasir @ 2024-05-17 07:19 UTC (permalink / raw)
To: Maciek Sakrejda <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers
On Fri, May 17, 2024 at 12:25 AM Maciek Sakrejda <[email protected]>
wrote:
> Thanks for raising this. As someone who is only modestly familiar with
> Postgres internals or even C, but would still like to contribute through
> review, I find the current process of finding a suitable patch both tedious
> and daunting. The Reviewing a Patch article on the wiki [0] says reviews
> like mine are still welcome, but it's hard to get started. I'd love to see
> this be more approachable.
>
>
I totally agreed with Maciek. Its really hard for even an experience
developer to become a PG contributor or be able to contribute effectively.
> Thanks,
> Maciek
>
> [0]: https://wiki.postgresql.org/wiki/Reviewing_a_Patch
>
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: commitfest.postgresql.org is no longer fit for purpose
2024-05-16 18:30 commitfest.postgresql.org is no longer fit for purpose Robert Haas <[email protected]>
@ 2024-05-16 19:30 ` Daniel Gustafsson <[email protected]>
4 siblings, 0 replies; 21+ messages in thread
From: Daniel Gustafsson @ 2024-05-16 19:30 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: pgsql-hackers
> On 16 May 2024, at 20:30, Robert Haas <[email protected]> wrote:
> The original intent of CommitFests, and of commitfest.postgresql.org
> by extension, was to provide a place where patches could be registered
> to indicate that they needed to be reviewed, thus enabling patch
> authors and patch reviewers to find each other in a reasonably
> efficient way. I don't think it's working any more.
But which part is broken though, the app, our commitfest process and workflow
and the its intent, or our assumption that we follow said process and workflow
which may or may not be backed by evidence? IMHO, from being CMF many times,
there is a fair bit of the latter, which excacerbates the problem. This is
harder to fix with more or better software though.
> I spent a good deal of time going through the CommitFest this week
And you deserve a big Thank You for that.
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: commitfest.postgresql.org is no longer fit for purpose
2024-05-16 18:30 commitfest.postgresql.org is no longer fit for purpose Robert Haas <[email protected]>
@ 2024-05-16 20:46 ` Melanie Plageman <[email protected]>
2024-05-16 20:54 ` Re: commitfest.postgresql.org is no longer fit for purpose Tom Lane <[email protected]>
2024-05-16 21:35 ` Re: commitfest.postgresql.org is no longer fit for purpose Peter Eisentraut <[email protected]>
4 siblings, 2 replies; 21+ messages in thread
From: Melanie Plageman @ 2024-05-16 20:46 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: pgsql-hackers
On Thu, May 16, 2024 at 2:30 PM Robert Haas <[email protected]> wrote:
>
> Hi,
>
> The original intent of CommitFests, and of commitfest.postgresql.org
> by extension, was to provide a place where patches could be registered
> to indicate that they needed to be reviewed, thus enabling patch
> authors and patch reviewers to find each other in a reasonably
> efficient way. I don't think it's working any more. I spent a good
> deal of time going through the CommitFest this week, and I didn't get
> through a very large percentage of it, and what I found is that the
> status of the patches registered there is often much messier than can
> be captured by a simple "Needs Review" or "Waiting on Author," and the
> number of patches that are actually in need of review is not all that
> large. For example, there are:
>
> - patches parked there by a committer who will almost certainly do
> something about them after we branch
> - patches parked there by a committer who probably won't do something
> about them after we branch, but maybe they will, or maybe somebody
> else will, and anyway this way we at least run CI
-- snip --
> So, our CommitFest application has turned into a patch tracker. IMHO,
> patch trackers intrinsically tend to suck, because they fill up with
> garbage that nobody cares about, and nobody wants to do the colossal
> amount of work that it takes to maintain them. But our patch tracker
> sucks MORE, because it's not even intended to BE a general-purpose
> patch tracker.
I was reflecting on why a general purpose patch tracker sounded
appealing to me, and I realized that, at least at this time of year, I
have a few patches that really count as "waiting on author" that I
know I need to do additional work on before they need more review but
which aren't currently my top priority. I should probably simply
withdraw and re-register them. My justification was that I'll lose
them if I don't keep them in the commitfest app. But, I could just,
you know, save them somewhere myself instead of polluting the
commitfest app with them. I don't know if others are in this
situation. Anyway, I'm definitely currently guilty of parking.
- Melanie
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: commitfest.postgresql.org is no longer fit for purpose
2024-05-16 18:30 commitfest.postgresql.org is no longer fit for purpose Robert Haas <[email protected]>
2024-05-16 20:46 ` Re: commitfest.postgresql.org is no longer fit for purpose Melanie Plageman <[email protected]>
@ 2024-05-16 20:54 ` Tom Lane <[email protected]>
1 sibling, 0 replies; 21+ messages in thread
From: Tom Lane @ 2024-05-16 20:54 UTC (permalink / raw)
To: Melanie Plageman <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers
Melanie Plageman <[email protected]> writes:
> I was reflecting on why a general purpose patch tracker sounded
> appealing to me, and I realized that, at least at this time of year, I
> have a few patches that really count as "waiting on author" that I
> know I need to do additional work on before they need more review but
> which aren't currently my top priority. I should probably simply
> withdraw and re-register them. My justification was that I'll lose
> them if I don't keep them in the commitfest app. But, I could just,
> you know, save them somewhere myself instead of polluting the
> commitfest app with them. I don't know if others are in this
> situation. Anyway, I'm definitely currently guilty of parking.
It's also nice that the CF app will run CI for you, so at least
you can keep the patch building if you're so inclined.
David J. had a suggestion for this too upthread, which was to create a
separate slot for WIP patches that isn't one of the dated CF slots.
It's hard to argue that such patches need to be in "the CF app" at
all, if you're not actively looking for review. But the CI runs
and the handy per-author patch status list make the app very tempting
infrastructure for parked patches. Maybe we could have a not-the-CF
app that offers those amenities?
regards, tom lane
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: commitfest.postgresql.org is no longer fit for purpose
2024-05-16 18:30 commitfest.postgresql.org is no longer fit for purpose Robert Haas <[email protected]>
2024-05-16 20:46 ` Re: commitfest.postgresql.org is no longer fit for purpose Melanie Plageman <[email protected]>
@ 2024-05-16 21:35 ` Peter Eisentraut <[email protected]>
2024-05-16 21:46 ` Re: commitfest.postgresql.org is no longer fit for purpose Tom Lane <[email protected]>
1 sibling, 1 reply; 21+ messages in thread
From: Peter Eisentraut @ 2024-05-16 21:35 UTC (permalink / raw)
To: Melanie Plageman <[email protected]>; Robert Haas <[email protected]>; +Cc: pgsql-hackers
On 16.05.24 22:46, Melanie Plageman wrote:
> I was reflecting on why a general purpose patch tracker sounded
> appealing to me, and I realized that, at least at this time of year, I
> have a few patches that really count as "waiting on author" that I
> know I need to do additional work on before they need more review but
> which aren't currently my top priority. I should probably simply
> withdraw and re-register them. My justification was that I'll lose
> them if I don't keep them in the commitfest app. But, I could just,
> you know, save them somewhere myself instead of polluting the
> commitfest app with them. I don't know if others are in this
> situation. Anyway, I'm definitely currently guilty of parking.
I don't have a problem with that at all. It's pretty understandable
that many patches are parked between say April and July.
The key is the keep the status up to date.
If we have 890 patches in Waiting for Author and 100 patches in Ready
for Committer and 10 patches in Needs Review, and those 10 patches are
actually reviewable, then that's good. There might need to be a
"background process" to make sure those 890 waiting patches are not
parked forever and those 100 patches actually get committed sometime,
but in principle this is the system working.
The problem is if we have 180 patches in Needs Review, and only 20 are
really actually ready to be reviewed. And a second-order problem is
that if you already know that this will be the case, you give up before
even looking.
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: commitfest.postgresql.org is no longer fit for purpose
2024-05-16 18:30 commitfest.postgresql.org is no longer fit for purpose Robert Haas <[email protected]>
2024-05-16 20:46 ` Re: commitfest.postgresql.org is no longer fit for purpose Melanie Plageman <[email protected]>
2024-05-16 21:35 ` Re: commitfest.postgresql.org is no longer fit for purpose Peter Eisentraut <[email protected]>
@ 2024-05-16 21:46 ` Tom Lane <[email protected]>
2024-05-16 22:03 ` Re: commitfest.postgresql.org is no longer fit for purpose Peter Eisentraut <[email protected]>
2024-05-17 02:26 ` Re: commitfest.postgresql.org is no longer fit for purpose Robert Haas <[email protected]>
0 siblings, 2 replies; 21+ messages in thread
From: Tom Lane @ 2024-05-16 21:46 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Melanie Plageman <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers
Peter Eisentraut <[email protected]> writes:
> The problem is if we have 180 patches in Needs Review, and only 20 are
> really actually ready to be reviewed. And a second-order problem is
> that if you already know that this will be the case, you give up before
> even looking.
Right, so what can we do about that? Does Needs Review state need to
be subdivided, and if so how?
If it's just that a patch should be in some other state altogether,
we should simply encourage people to change the state as soon as they
discover that. I think the problem is not so much "90% are in the
wrong state" as "each potential reviewer has to rediscover that".
At this point it seems like there's consensus to have a "parking"
section of the CF app, separate from the time-boxed CFs, and I hope
somebody will go make that happen. But I don't think that's our only
issue, so we need to keep thinking about what should be improved.
regards, tom lane
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: commitfest.postgresql.org is no longer fit for purpose
2024-05-16 18:30 commitfest.postgresql.org is no longer fit for purpose Robert Haas <[email protected]>
2024-05-16 20:46 ` Re: commitfest.postgresql.org is no longer fit for purpose Melanie Plageman <[email protected]>
2024-05-16 21:35 ` Re: commitfest.postgresql.org is no longer fit for purpose Peter Eisentraut <[email protected]>
2024-05-16 21:46 ` Re: commitfest.postgresql.org is no longer fit for purpose Tom Lane <[email protected]>
@ 2024-05-16 22:03 ` Peter Eisentraut <[email protected]>
2024-05-17 08:58 ` Re: commitfest.postgresql.org is no longer fit for purpose Daniel Gustafsson <[email protected]>
1 sibling, 1 reply; 21+ messages in thread
From: Peter Eisentraut @ 2024-05-16 22:03 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Melanie Plageman <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers
On 16.05.24 23:46, Tom Lane wrote:
> Peter Eisentraut <[email protected]> writes:
>> The problem is if we have 180 patches in Needs Review, and only 20 are
>> really actually ready to be reviewed. And a second-order problem is
>> that if you already know that this will be the case, you give up before
>> even looking.
>
> Right, so what can we do about that? Does Needs Review state need to
> be subdivided, and if so how?
Maybe a new state "Unclear". Then a commitfest manager, or someone like
Robert just now, can more easily power through the list and set
everything that's doubtful to Unclear, with the understanding that the
author can set it back to Needs Review to signal that they actually
think it's ready. Or, as a variant of what someone else was saying,
just set all patches that carry over from March to July as Unclear. Or
something like that.
I think, if we consider the core mission of the commitfest app, we need
to be more protective of the Needs Review state.
I have been, from time to time, when I'm in commitfest management mode,
aggressive in setting entries back to Waiting on Author, but that's not
always optimal.
So a third status that encompasses the various other situations like
maybe forgotten by author, disagreements between author and reviewer,
process difficulties, needs some senior developer intervention, etc.
could be helpful.
This could also help scale the commitfest manager role, because then the
head CFM could have like three helpers and just tell them, look at all
the "Unclear" ones and help resolve them.
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: commitfest.postgresql.org is no longer fit for purpose
2024-05-16 18:30 commitfest.postgresql.org is no longer fit for purpose Robert Haas <[email protected]>
2024-05-16 20:46 ` Re: commitfest.postgresql.org is no longer fit for purpose Melanie Plageman <[email protected]>
2024-05-16 21:35 ` Re: commitfest.postgresql.org is no longer fit for purpose Peter Eisentraut <[email protected]>
2024-05-16 21:46 ` Re: commitfest.postgresql.org is no longer fit for purpose Tom Lane <[email protected]>
2024-05-16 22:03 ` Re: commitfest.postgresql.org is no longer fit for purpose Peter Eisentraut <[email protected]>
@ 2024-05-17 08:58 ` Daniel Gustafsson <[email protected]>
0 siblings, 0 replies; 21+ messages in thread
From: Daniel Gustafsson @ 2024-05-17 08:58 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Tom Lane <[email protected]>; Melanie Plageman <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers
> On 17 May 2024, at 00:03, Peter Eisentraut <[email protected]> wrote:
> I think, if we consider the core mission of the commitfest app, we need to be more protective of the Needs Review state.
IMHO this is a very very good summary of what we should focus on with this
work.
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: commitfest.postgresql.org is no longer fit for purpose
2024-05-16 18:30 commitfest.postgresql.org is no longer fit for purpose Robert Haas <[email protected]>
2024-05-16 20:46 ` Re: commitfest.postgresql.org is no longer fit for purpose Melanie Plageman <[email protected]>
2024-05-16 21:35 ` Re: commitfest.postgresql.org is no longer fit for purpose Peter Eisentraut <[email protected]>
2024-05-16 21:46 ` Re: commitfest.postgresql.org is no longer fit for purpose Tom Lane <[email protected]>
@ 2024-05-17 02:26 ` Robert Haas <[email protected]>
2024-05-17 07:32 ` Re: commitfest.postgresql.org is no longer fit for purpose Heikki Linnakangas <[email protected]>
2024-05-17 12:19 ` Re: commitfest.postgresql.org is no longer fit for purpose Joe Conway <[email protected]>
1 sibling, 2 replies; 21+ messages in thread
From: Robert Haas @ 2024-05-17 02:26 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Melanie Plageman <[email protected]>; pgsql-hackers
On Thu, May 16, 2024 at 5:46 PM Tom Lane <[email protected]> wrote:
> Right, so what can we do about that? Does Needs Review state need to
> be subdivided, and if so how?
It doesn't really matter how many states we have if they're not set accurately.
> At this point it seems like there's consensus to have a "parking"
> section of the CF app, separate from the time-boxed CFs, and I hope
> somebody will go make that happen. But I don't think that's our only
> issue, so we need to keep thinking about what should be improved.
I do *emphatically* think we need a parking lot. And a better
integration between commitfest.postgresql.org and the cfbot, too. It's
just ridiculous that more work hasn't been put into this. I'm not
faulting Thomas or anyone else -- I mean, it's not his job to build
the infrastructure the project needs any more than it is anyone else's
-- but for a project of the size and importance of PostgreSQL to take
years to make minor improvements to this kind of critical
infrastructure is kind of nuts. If we don't have enough volunteers,
let's go recruit some more and promise them cookies.
I think you're overestimating the extent to which the problem is "we
don't reject enough patches". That *is* a problem, but it seems we
have also started rejecting some patches that just never really got
much attention for no particularly good reason, while letting other
things linger that didn't really get that much more attention, or
which were objectively much worse ideas. I think that one place where
the process is breaking down is in the tacit assumption that the
person who wrote the patch wants to get it committed. In some cases,
people park things in the CF for CI runs without a strong intention of
pursuing them; in other cases, the patch author is in no kind of rush.
I think we need to move to a system where patches exist independent of
a CommitFest, and get CI run on them unless they get explicitly closed
or are too inactive or some other criterion that boils down to nobody
cares any more. Then, we need to get whatever subset of those patches
need to be reviewed in a particular CommitFest added to that
CommitFest. For example, imagine that the CommitFest is FORCIBLY empty
until a week before it starts. You can still register patches in the
system generally, but that just means they get CI runs, not that
they're scheduled to be reviewed. A week before the CommitFest,
everyone who has a patch registered in the system that still applies
gets an email saying "click here if you think this patch should be
reviewed in the upcoming CommitFest -- if you don't care about the
patch any more or it needs more work before other people review it,
don't click here". Then, the CommitFest ends up containing only the
things where the patch author clicked there during that week.
For bonus points, suppose we make it so that when you click the link,
it takes you to a box where you can type in a text comment that will
display in the app, explaining why your patch needs review: "Robert
says the wire protocol changes in this patch are wrong, but I think
he's full of hot garbage and want a second opinion!" (a purely
hypothetical example, I'm sure) If you put in a comment like this when
you register your patch for the CommitFest, it gets a sparkly gold
border and floats to the top of the list, or we mail you a Kit-Kat
bar, or something. I don't know.
The point is - we need a much better signal to noise ratio here. I bet
the number of patches in the CommitFest that actually need review is
something like 25% of the total. The rest are things that are just
parked there by a committer, or that the author doesn't care about
right now, or that are already being actively discussed, or where
there's not a clear way forward. We could create new statuses for all
of those states - "Parked", "In Hibernation," "Under Discussion," and
"Unclear" - but I think that's missing the point. What we really want
is to not see that stuff in the first place. It's a CommitFest, not
once-upon-a-time-I-wrote-a-patch-Fest.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: commitfest.postgresql.org is no longer fit for purpose
2024-05-16 18:30 commitfest.postgresql.org is no longer fit for purpose Robert Haas <[email protected]>
2024-05-16 20:46 ` Re: commitfest.postgresql.org is no longer fit for purpose Melanie Plageman <[email protected]>
2024-05-16 21:35 ` Re: commitfest.postgresql.org is no longer fit for purpose Peter Eisentraut <[email protected]>
2024-05-16 21:46 ` Re: commitfest.postgresql.org is no longer fit for purpose Tom Lane <[email protected]>
2024-05-17 02:26 ` Re: commitfest.postgresql.org is no longer fit for purpose Robert Haas <[email protected]>
@ 2024-05-17 07:32 ` Heikki Linnakangas <[email protected]>
2024-05-17 08:15 ` Re: commitfest.postgresql.org is no longer fit for purpose Jelte Fennema-Nio <[email protected]>
2024-05-17 08:47 ` Re: commitfest.postgresql.org is no longer fit for purpose Daniel Gustafsson <[email protected]>
2024-05-17 09:19 ` Re: commitfest.postgresql.org is no longer fit for purpose Chris Travers <[email protected]>
1 sibling, 3 replies; 21+ messages in thread
From: Heikki Linnakangas @ 2024-05-17 07:32 UTC (permalink / raw)
To: Robert Haas <[email protected]>; Tom Lane <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Melanie Plageman <[email protected]>; pgsql-hackers
On 17/05/2024 05:26, Robert Haas wrote:
> For bonus points, suppose we make it so that when you click the link,
> it takes you to a box where you can type in a text comment that will
> display in the app, explaining why your patch needs review: "Robert
> says the wire protocol changes in this patch are wrong, but I think
> he's full of hot garbage and want a second opinion!" (a purely
> hypothetical example, I'm sure) If you put in a comment like this when
> you register your patch for the CommitFest, it gets a sparkly gold
> border and floats to the top of the list, or we mail you a Kit-Kat
> bar, or something. I don't know.
Dunno about having to click a link or sparkly gold borders, but +1 on
having a free-form text box for notes like that. Things like "cfbot says
this has bitrotted" or "Will review this after other patch this depends
on". On the mailing list, notes like that are both noisy and easily lost
in the threads. But as a little free-form text box on the commitfest, it
would be handy.
One risk is that if we start to rely too much on that, or on the other
fields in the commitfest app for that matter, we de-value the mailing
list archives. I'm not too worried about it, the idea is that the
summary box just summarizes what's already been said on the mailing
list, or is transient information like "I'll get to this tomorrow"
that's not interesting to archive.
> The point is - we need a much better signal to noise ratio here. I bet
> the number of patches in the CommitFest that actually need review is
> something like 25% of the total. The rest are things that are just
> parked there by a committer, or that the author doesn't care about
> right now, or that are already being actively discussed, or where
> there's not a clear way forward. We could create new statuses for all
> of those states - "Parked", "In Hibernation," "Under Discussion," and
> "Unclear" - but I think that's missing the point. What we really want
> is to not see that stuff in the first place. It's a CommitFest, not
> once-upon-a-time-I-wrote-a-patch-Fest.
Yeah, I'm also skeptical of adding new categories or statuses to the
commitfest.
I sometimes add patches to the commitfest that are not ready to be
committed, because I want review on the general idea or approach, before
polishing the patch to final state. That's also a fine use of the
commitfest app. The expected workflow is to get some review on the
patch, and then move it back to Waiting on Author.
--
Heikki Linnakangas
Neon (https://neon.tech)
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: commitfest.postgresql.org is no longer fit for purpose
2024-05-16 18:30 commitfest.postgresql.org is no longer fit for purpose Robert Haas <[email protected]>
2024-05-16 20:46 ` Re: commitfest.postgresql.org is no longer fit for purpose Melanie Plageman <[email protected]>
2024-05-16 21:35 ` Re: commitfest.postgresql.org is no longer fit for purpose Peter Eisentraut <[email protected]>
2024-05-16 21:46 ` Re: commitfest.postgresql.org is no longer fit for purpose Tom Lane <[email protected]>
2024-05-17 02:26 ` Re: commitfest.postgresql.org is no longer fit for purpose Robert Haas <[email protected]>
2024-05-17 07:32 ` Re: commitfest.postgresql.org is no longer fit for purpose Heikki Linnakangas <[email protected]>
@ 2024-05-17 08:15 ` Jelte Fennema-Nio <[email protected]>
2 siblings, 0 replies; 21+ messages in thread
From: Jelte Fennema-Nio @ 2024-05-17 08:15 UTC (permalink / raw)
To: Heikki Linnakangas <[email protected]>; +Cc: Robert Haas <[email protected]>; Tom Lane <[email protected]>; Peter Eisentraut <[email protected]>; Melanie Plageman <[email protected]>; pgsql-hackers
On Fri, 17 May 2024 at 06:58, Peter Eisentraut <[email protected]> wrote:
> Yeah, some fine-tuning might be required. But I would be wary of
> over-designing too many new states at this point. I think the key idea
> is that there ought to be a state that communicates "needs attention
> from someone other than author, reviewer, or committer".
+1 on adding a new state like this. I think it would make sense for
the author to request additional input, but I think it would need two
states, something like "Request for new reviewer" and "Request for new
committer". Just like authors disappear sometimes after a driveby
patch submission, it's fairly common too imho for reviewers or
committers to disappear after a driveby review. Having a way for an
author to mark a patch as such would be helpful, both to the author,
and to reviewers/committers looking to do help some patch out.
On Fri, 17 May 2024 at 09:33, Heikki Linnakangas <[email protected]> wrote:
> Things like "cfbot says
> this has bitrotted" or "Will review this after other patch this depends
> on". On the mailing list, notes like that are both noisy and easily lost
> in the threads. But as a little free-form text box on the commitfest, it
> would be handy.
+many on the free form text box
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: commitfest.postgresql.org is no longer fit for purpose
2024-05-16 18:30 commitfest.postgresql.org is no longer fit for purpose Robert Haas <[email protected]>
2024-05-16 20:46 ` Re: commitfest.postgresql.org is no longer fit for purpose Melanie Plageman <[email protected]>
2024-05-16 21:35 ` Re: commitfest.postgresql.org is no longer fit for purpose Peter Eisentraut <[email protected]>
2024-05-16 21:46 ` Re: commitfest.postgresql.org is no longer fit for purpose Tom Lane <[email protected]>
2024-05-17 02:26 ` Re: commitfest.postgresql.org is no longer fit for purpose Robert Haas <[email protected]>
2024-05-17 07:32 ` Re: commitfest.postgresql.org is no longer fit for purpose Heikki Linnakangas <[email protected]>
@ 2024-05-17 08:47 ` Daniel Gustafsson <[email protected]>
2 siblings, 0 replies; 21+ messages in thread
From: Daniel Gustafsson @ 2024-05-17 08:47 UTC (permalink / raw)
To: Heikki Linnakangas <[email protected]>; +Cc: Robert Haas <[email protected]>; Tom Lane <[email protected]>; Peter Eisentraut <[email protected]>; Melanie Plageman <[email protected]>; pgsql-hackers
> On 17 May 2024, at 09:32, Heikki Linnakangas <[email protected]> wrote:
> On the mailing list, notes like that are both noisy and easily lost in the threads. But as a little free-form text box on the commitfest, it would be handy.
On a similar note, I have in the past suggested adding a free-form textfield to
the patch submission form for the author to give a short summary of what the
patch does/adds/requires etc. While the thread contains all of this, it's
likely quite overwhelming for many in general and new contributors in
particular. A short note, on purpose limited to ~500 chars or so to not allow
mailinglist post copy/paste, could be helpful there I think (I've certainly
wanted one, many times over, especially when doing CFM).
> One risk is that if we start to rely too much on that, or on the other fields in the commitfest app for that matter, we de-value the mailing list archives. I'm not too worried about it, the idea is that the summary box just summarizes what's already been said on the mailing list, or is transient information like "I'll get to this tomorrow" that's not interesting to archive.
One way to ensure we capture detail could be if the system would send an
automated email to the thread summarizing the entry when it's marked as
"committed"?
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: commitfest.postgresql.org is no longer fit for purpose
2024-05-16 18:30 commitfest.postgresql.org is no longer fit for purpose Robert Haas <[email protected]>
2024-05-16 20:46 ` Re: commitfest.postgresql.org is no longer fit for purpose Melanie Plageman <[email protected]>
2024-05-16 21:35 ` Re: commitfest.postgresql.org is no longer fit for purpose Peter Eisentraut <[email protected]>
2024-05-16 21:46 ` Re: commitfest.postgresql.org is no longer fit for purpose Tom Lane <[email protected]>
2024-05-17 02:26 ` Re: commitfest.postgresql.org is no longer fit for purpose Robert Haas <[email protected]>
2024-05-17 07:32 ` Re: commitfest.postgresql.org is no longer fit for purpose Heikki Linnakangas <[email protected]>
@ 2024-05-17 09:19 ` Chris Travers <[email protected]>
2 siblings, 0 replies; 21+ messages in thread
From: Chris Travers @ 2024-05-17 09:19 UTC (permalink / raw)
To: Heikki Linnakangas <[email protected]>; +Cc: Robert Haas <[email protected]>; Tom Lane <[email protected]>; Peter Eisentraut <[email protected]>; Melanie Plageman <[email protected]>; pgsql-hackers
I think there are actually a number of factors that make this much harder.
On Fri, May 17, 2024 at 2:33 PM Heikki Linnakangas <[email protected]> wrote:
>
> On 17/05/2024 05:26, Robert Haas wrote:
> > For bonus points, suppose we make it so that when you click the link,
> > it takes you to a box where you can type in a text comment that will
> > display in the app, explaining why your patch needs review: "Robert
> > says the wire protocol changes in this patch are wrong, but I think
> > he's full of hot garbage and want a second opinion!" (a purely
> > hypothetical example, I'm sure) If you put in a comment like this when
> > you register your patch for the CommitFest, it gets a sparkly gold
> > border and floats to the top of the list, or we mail you a Kit-Kat
> > bar, or something. I don't know.
>
> Dunno about having to click a link or sparkly gold borders, but +1 on
> having a free-form text box for notes like that. Things like "cfbot says
> this has bitrotted" or "Will review this after other patch this depends
> on". On the mailing list, notes like that are both noisy and easily lost
> in the threads. But as a little free-form text box on the commitfest, it
> would be handy.
>
> One risk is that if we start to rely too much on that, or on the other
> fields in the commitfest app for that matter, we de-value the mailing
> list archives. I'm not too worried about it, the idea is that the
> summary box just summarizes what's already been said on the mailing
> list, or is transient information like "I'll get to this tomorrow"
> that's not interesting to archive.
>
> > The point is - we need a much better signal to noise ratio here. I bet
> > the number of patches in the CommitFest that actually need review is
> > something like 25% of the total. The rest are things that are just
> > parked there by a committer, or that the author doesn't care about
> > right now, or that are already being actively discussed, or where
> > there's not a clear way forward. We could create new statuses for all
> > of those states - "Parked", "In Hibernation," "Under Discussion," and
> > "Unclear" - but I think that's missing the point. What we really want
> > is to not see that stuff in the first place. It's a CommitFest, not
> > once-upon-a-time-I-wrote-a-patch-Fest.
Yeah this is a problem.
I think in cases here something is in hibernation or unclear it really
should be "returned with feedback." There's really nothing stopping
someone from learning from the experience and resubmitting an improved
version.
I think under discussion is also rather unclear. The current statuses
already cover this sort of thing (waiting for author and waiting for
review). Maybe we could improve the categories here but it is
important to note whether the author or a reviewer is expected to take
the next step.
If the author doesn't respond within a period of time (let's say 30
days), I think we can just say "returned with feedback."
Since you can already attach older threads to a commitfest entry, it
seems to me that we ought to be more aggressive with "returned with
feedback" and note that this doesn't necessarily mean "rejected" which
is a separate status which we rarely use.
>
> Yeah, I'm also skeptical of adding new categories or statuses to the
> commitfest.
>
> I sometimes add patches to the commitfest that are not ready to be
> committed, because I want review on the general idea or approach, before
> polishing the patch to final state. That's also a fine use of the
> commitfest app. The expected workflow is to get some review on the
> patch, and then move it back to Waiting on Author.
>
> --
> Heikki Linnakangas
> Neon (https://neon.tech)
>
>
>
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: commitfest.postgresql.org is no longer fit for purpose
2024-05-16 18:30 commitfest.postgresql.org is no longer fit for purpose Robert Haas <[email protected]>
2024-05-16 20:46 ` Re: commitfest.postgresql.org is no longer fit for purpose Melanie Plageman <[email protected]>
2024-05-16 21:35 ` Re: commitfest.postgresql.org is no longer fit for purpose Peter Eisentraut <[email protected]>
2024-05-16 21:46 ` Re: commitfest.postgresql.org is no longer fit for purpose Tom Lane <[email protected]>
2024-05-17 02:26 ` Re: commitfest.postgresql.org is no longer fit for purpose Robert Haas <[email protected]>
@ 2024-05-17 12:19 ` Joe Conway <[email protected]>
1 sibling, 0 replies; 21+ messages in thread
From: Joe Conway @ 2024-05-17 12:19 UTC (permalink / raw)
To: Robert Haas <[email protected]>; Tom Lane <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Melanie Plageman <[email protected]>; pgsql-hackers
On 5/16/24 22:26, Robert Haas wrote:
> For example, imagine that the CommitFest is FORCIBLY empty
> until a week before it starts. You can still register patches in the
> system generally, but that just means they get CI runs, not that
> they're scheduled to be reviewed. A week before the CommitFest,
> everyone who has a patch registered in the system that still applies
> gets an email saying "click here if you think this patch should be
> reviewed in the upcoming CommitFest -- if you don't care about the
> patch any more or it needs more work before other people review it,
> don't click here". Then, the CommitFest ends up containing only the
> things where the patch author clicked there during that week.
100% agree. This is in line with what I suggested on an adjacent part of
the thread.
> The point is - we need a much better signal to noise ratio here. I bet
> the number of patches in the CommitFest that actually need review is
> something like 25% of the total. The rest are things that are just
> parked there by a committer, or that the author doesn't care about
> right now, or that are already being actively discussed, or where
> there's not a clear way forward.
I think there is another case that no one talks about, but I'm sure
exists, and that I am not the only one guilty of thinking this way.
Namely, the week before commitfest I don't actually know if I will have
the time during that month, but I will make sure my patch is in the
commitfest just in case I get a few clear days to work on it. Because if
it isn't there, I can't take advantage of those "found" hours.
> We could create new statuses for all of those states - "Parked", "In
> Hibernation," "Under Discussion," and "Unclear" - but I think that's
> missing the point. What we really want is to not see that stuff in
> the first place. It's a CommitFest, not
> once-upon-a-time-I-wrote-a-patch-Fest.
+1
--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: commitfest.postgresql.org is no longer fit for purpose
2024-05-16 18:30 commitfest.postgresql.org is no longer fit for purpose Robert Haas <[email protected]>
@ 2024-05-19 09:37 ` Dmitry Dolgov <[email protected]>
2024-05-19 14:42 ` Re: commitfest.postgresql.org is no longer fit for purpose Joe Conway <[email protected]>
4 siblings, 1 reply; 21+ messages in thread
From: Dmitry Dolgov @ 2024-05-19 09:37 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: pgsql-hackers
> On Thu, May 16, 2024 at 02:30:03PM -0400, Robert Haas wrote:
>
> I wonder what ideas people have for improving this situation. I doubt
> that there's any easy answer that just makes the problem go away --
> keeping large groups of people organized is a tremendously difficult
> task under pretty much all circumstances, and the fact that, in this
> context, nobody's really the boss, makes it a whole lot harder. But I
> also feel like what we're doing right now can't possibly be the best
> that we can do.
There are lots of good takes on this in the thread. It also makes clear what's
at stake -- as Melanie pointed out with the patch about EXPLAIN for parallel
bitmap heap scan, we're loosing potential contributors for no reasons. But I'm
a bit concerned about what are the next steps: if memory serves, every couple
of years there is a discussion about everything what goes wrong with the review
process, commitfests, etc. Yet to my (admittedly limited) insight into the
community, not many things have changed due to those discussions. How do we
make sure this time it will be different?
It is indeed tremendously difficult to self organize, so maybe it's worth to
volunteer a group of people to work out details of one or two proposals,
answering the question "how to make it better?". As far as I understand, the
community already has a similar experience. Summarizing this thread,
there seems to be following dimensions to look at:
* What is the purpose of CF and how to align it better with the community
goals.
"CommitFest" here means both the CF tool and the process behind it. So far
the discussion was evolving around the state machine for each individual CF
item as well as the whole CF cycle. At the end of the day perhaps a list of
pairs (item, status) is not the best representation, probably more filters
have to be considered (e.g. implementing a workflow "give me all the items,
updated in the last month with the last reply being from the patch author").
* How to synchronize the mailing list with CF content.
The entropy of CF content grows over time, making it less efficient. For
especially old threads it's even more visible. How to reduce the entropy
without scaring new contributors away?
* How to deal with review scalability bottleneck.
An evergreen question. PostgreSQL is getting more popular and, as stated in
diverse research whitepapers, the amount of contribution grows as a power
law, where the number of reviewers grows at best sub-linearly (limited by the
velocity of knowledge sharing). I agree with Andrey on this, the only
way I see to handle this is to scale CF management efforts.
* What are the UX gaps of CF tool?
There seems to be some number of improvements that could make work with CF
tool more frictionless.
What I think wasn't discussed yet in details is the question of motivation.
Surely, it would be great to have a process that will introduce as less burden
as possible. But giving more motivation to follow the process / use the tool is
as important. What motivates folks to review patches, figure out status of a
complicated patch thread, maintain a list of open items, etc?
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: commitfest.postgresql.org is no longer fit for purpose
2024-05-16 18:30 commitfest.postgresql.org is no longer fit for purpose Robert Haas <[email protected]>
2024-05-19 09:37 ` Re: commitfest.postgresql.org is no longer fit for purpose Dmitry Dolgov <[email protected]>
@ 2024-05-19 14:42 ` Joe Conway <[email protected]>
0 siblings, 0 replies; 21+ messages in thread
From: Joe Conway @ 2024-05-19 14:42 UTC (permalink / raw)
To: Dmitry Dolgov <[email protected]>; Robert Haas <[email protected]>; +Cc: pgsql-hackers
On 5/19/24 05:37, Dmitry Dolgov wrote:
> * How to deal with review scalability bottleneck.
>
> An evergreen question. PostgreSQL is getting more popular and, as stated in
> diverse research whitepapers, the amount of contribution grows as a power
> law, where the number of reviewers grows at best sub-linearly (limited by the
> velocity of knowledge sharing). I agree with Andrey on this, the only
> way I see to handle this is to scale CF management efforts.
The number of items tracked are surely growing, but I am not sure I
would call it exponential -- see attached
--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
[image/png] commitfest-stat.png (39.9K, ../../[email protected]/2-commitfest-stat.png)
download | view image
^ permalink raw reply [nested|flat] 21+ messages in thread
end of thread, other threads:[~2024-05-19 14:42 UTC | newest]
Thread overview: 21+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-05-03 14:37 [PATCH v36 1/6] Introduce RelOptInfo->notnullattrs attribute 一挃 <[email protected]>
2024-05-16 18:30 commitfest.postgresql.org is no longer fit for purpose Robert Haas <[email protected]>
2024-05-16 19:13 ` Re: commitfest.postgresql.org is no longer fit for purpose David G. Johnston <[email protected]>
2024-05-16 20:50 ` Re: commitfest.postgresql.org is no longer fit for purpose Jacob Champion <[email protected]>
2024-05-16 19:25 ` Re: commitfest.postgresql.org is no longer fit for purpose Maciek Sakrejda <[email protected]>
2024-05-17 07:19 ` Re: commitfest.postgresql.org is no longer fit for purpose Yasir <[email protected]>
2024-05-16 19:30 ` Re: commitfest.postgresql.org is no longer fit for purpose Daniel Gustafsson <[email protected]>
2024-05-16 20:46 ` Re: commitfest.postgresql.org is no longer fit for purpose Melanie Plageman <[email protected]>
2024-05-16 20:54 ` Re: commitfest.postgresql.org is no longer fit for purpose Tom Lane <[email protected]>
2024-05-16 21:35 ` Re: commitfest.postgresql.org is no longer fit for purpose Peter Eisentraut <[email protected]>
2024-05-16 21:46 ` Re: commitfest.postgresql.org is no longer fit for purpose Tom Lane <[email protected]>
2024-05-16 22:03 ` Re: commitfest.postgresql.org is no longer fit for purpose Peter Eisentraut <[email protected]>
2024-05-17 08:58 ` Re: commitfest.postgresql.org is no longer fit for purpose Daniel Gustafsson <[email protected]>
2024-05-17 02:26 ` Re: commitfest.postgresql.org is no longer fit for purpose Robert Haas <[email protected]>
2024-05-17 07:32 ` Re: commitfest.postgresql.org is no longer fit for purpose Heikki Linnakangas <[email protected]>
2024-05-17 08:15 ` Re: commitfest.postgresql.org is no longer fit for purpose Jelte Fennema-Nio <[email protected]>
2024-05-17 08:47 ` Re: commitfest.postgresql.org is no longer fit for purpose Daniel Gustafsson <[email protected]>
2024-05-17 09:19 ` Re: commitfest.postgresql.org is no longer fit for purpose Chris Travers <[email protected]>
2024-05-17 12:19 ` Re: commitfest.postgresql.org is no longer fit for purpose Joe Conway <[email protected]>
2024-05-19 09:37 ` Re: commitfest.postgresql.org is no longer fit for purpose Dmitry Dolgov <[email protected]>
2024-05-19 14:42 ` Re: commitfest.postgresql.org is no longer fit for purpose Joe Conway <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox