Thread: [Commitfest 2022-07] Patch Triage: Needs Review, Part 1

[Commitfest 2022-07] Patch Triage: Needs Review, Part 1

From
Jacob Champion
Date:
Hi,

Next up is the large list of Needs Review. This part 1 should include
entries as old or older than seven commitfests running.

My heuristics for classifying these continue to evolve as I go, and
there's a lot to read, so please let me know if I've made any mistakes.

= Stalled Patches, Recommend Return =

These are stalled and I recommend outright that we return them. We don't
have a separate status for "needs more interest" (working on a patch) so
I'd just RwF, with a note explaining that what is actually needed to
continue isn't more code work but more coalition building.

- Implement INSERT SET syntax
  https://commitfest.postgresql.org/38/2218/

A recent author rebase this CF, but unfortunately I think the the real
issue is just a lack of review interest. It's been suggested for return
for a few CFs now.

- Fix up partitionwise join on how equi-join conditions between the
partition keys are identified
  https://commitfest.postgresql.org/38/2266/

It looks like this one was Returned with Feedback but did not actually
have feedback, which may have caused confusion. (Solid motivation for a
new close status.) I don't think there's been any review since 2020.

- New default role allowing to change per-role/database settings
  https://commitfest.postgresql.org/38/2918/

Stalled on review in January, and needs a rebase.

= Stalled Patches, Need Help =

These are stalled but seem to have interest. They need help to either
get them out of the rut, or else be Returned so that the author can try
a different approach instead of perma-rebasing. I plan to move them to
the next CF unless someone speaks up to say otherwise.

- Show shared filesets in pg_ls_tmpdir (pg_ls_* functions for showing
metadata and recurse)
  https://commitfest.postgresql.org/38/2377/

From a quick skim it looks like there was a flurry of initial positive
feedback followed by a stall and then some design whiplash. This thread
needs help to avoid burnout, I think.

- Make message at end-of-recovery less scary
  https://commitfest.postgresql.org/38/2490/

This got marked RfC twice, fell back out, and has been stuck in a rebase
loop.

- Fix behavior of geo_ops when NaN is involved
  https://commitfest.postgresql.org/38/2710/

Stuck in a half-committed state, which is tricky. Could maybe use a
reframing or recap (or a new thread?).

- Add extra statistics to explain for Nested Loop
  https://commitfest.postgresql.org/38/2765/

I think the author is hoping for help with testing and performance
characterization.

- CREATE INDEX CONCURRENTLY on partitioned table
  https://commitfest.postgresql.org/38/2815/

This had an author switch since last CF, so I think it'd be
inappropriate to close it out this time around, but it needs assistance.

- New Table Access Methods for Multi and Single Inserts
  https://commitfest.postgresql.org/38/2871/

Although there was a brief flicker in March, I think this one has
stalled out and is just about ready to be returned.

- Fix pg_rewind race condition just after promotion
  https://commitfest.postgresql.org/38/2864/

Seems like an important fix, but it's silent? Does it need to be
promoted to an Open Issue?

- pg_stat_statements and "IN" conditions
  https://commitfest.postgresql.org/38/2837/

Some good, recent interest. Last review in March.

- Function to log backtrace of postgres processes
  https://commitfest.postgresql.org/38/2863/

This is just starting to stall; I think it needs some assistance.

- Allow batched insert during cross-partition updates
  https://commitfest.postgresql.org/38/2992/

Was RfC (twice), then dropped out, now it's stuck rebasing. Last
substantial review in 2021.

= Active Patches =

The following are actively being worked and I expect to move them to
next CF:

- session variables, LET command
- Remove self join on a unique column
- Incremental Materialized View Maintenance
- More scalable multixacts buffers and locking
- Fast COPY FROM command for the foreign tables
- Extended statistics / estimate Var op Var clauses

Thanks,
--Jacob



Re: [Commitfest 2022-07] Patch Triage: Needs Review, Part 1

From
Tom Lane
Date:
Jacob Champion <jchampion@timescale.com> writes:
> Next up is the large list of Needs Review. This part 1 should include
> entries as old or older than seven commitfests running.

I'm just commenting on a couple that I've been involved with.

> = Stalled Patches, Recommend Return =

> - Fix up partitionwise join on how equi-join conditions between the
> partition keys are identified
>   https://commitfest.postgresql.org/38/2266/
> It looks like this one was Returned with Feedback but did not actually
> have feedback, which may have caused confusion. (Solid motivation for a
> new close status.) I don't think there's been any review since 2020.

Yeah, there was an earlier discussion of this same patch in some
previous CF-closing thread, IIRC, but I can't find that right now.
I think it basically is stuck behind the outer-join-variables work
I'm pursuing at https://commitfest.postgresql.org/39/3755/ ... and
when/if that lands, the present patch probably won't be anywhere
near what we want anyway.  +1 for RWF.

> = Stalled Patches, Need Help =

> - Fix behavior of geo_ops when NaN is involved
>   https://commitfest.postgresql.org/38/2710/

> Stuck in a half-committed state, which is tricky. Could maybe use a
> reframing or recap (or a new thread?).

We fixed a couple of easy cases but then realized that the hard cases
are hard.  I don't have much faith that the current patch is going to
lead to anything committable, and it doesn't look like anyone has the
appetite to put in a lot of work on the topic.  I'd vote for RWF.

            regards, tom lane



Re: [Commitfest 2022-07] Patch Triage: Needs Review, Part 1

From
Julien Rouhaud
Date:
Hi,

On Thu, Jul 28, 2022 at 02:28:23PM -0700, Jacob Champion wrote:
>
> = Stalled Patches, Need Help =
> [...]
> - Add extra statistics to explain for Nested Loop
>   https://commitfest.postgresql.org/38/2765/
>
> I think the author is hoping for help with testing and performance
> characterization.

As I mentioned in [1], this patch breaks the current assumption that
INSTRUMENT_ALL will lead to statement-level metrics that are generally useful.
According to the benchmark, the proposed patch would add a 1.5% overhead for
pg_stat_statements or any other similar extension that relies on INSTRUMENT_ALL
for no additional information, and I don't think it's acceptable.

I'm still not sure of what is the best way to fix that, but clearly something
has to be done, ideally without requiring every single pg_stat_statements-like
extension to be modified.

> The following are actively being worked and I expect to move them to
> next CF:
>
> - session variables, LET command

Yes please.



Re: [Commitfest 2022-07] Patch Triage: Needs Review, Part 1

From
Jacob Champion
Date:
On Thu, Jul 28, 2022 at 2:51 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > - Fix behavior of geo_ops when NaN is involved
> >   https://commitfest.postgresql.org/38/2710/
>
> > Stuck in a half-committed state, which is tricky. Could maybe use a
> > reframing or recap (or a new thread?).
>
> We fixed a couple of easy cases but then realized that the hard cases
> are hard.  I don't have much faith that the current patch is going to
> lead to anything committable, and it doesn't look like anyone has the
> appetite to put in a lot of work on the topic.  I'd vote for RWF.

Barring any competing votes, that's what I'll do, then. Thanks!

--Jacob



Re: [Commitfest 2022-07] Patch Triage: Needs Review, Part 1

From
Jacob Champion
Date:
Hi Julien,

On Thu, Jul 28, 2022 at 11:38 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > - Add extra statistics to explain for Nested Loop
> >   https://commitfest.postgresql.org/38/2765/
> >
> > [...]
>
> As I mentioned in [1], this patch breaks the current assumption that
> INSTRUMENT_ALL will lead to statement-level metrics that are generally useful.
> According to the benchmark, the proposed patch would add a 1.5% overhead for
> pg_stat_statements or any other similar extension that relies on INSTRUMENT_ALL
> for no additional information, and I don't think it's acceptable.

(I'm missing the [1] link.) From skimming the end of the thread, it
looks like Ekaterina responded to that concern and was hoping for
feedback. If you still think it doesn't go far enough, would you mind
dropping a note in the thread? Then we can mark WoA and go from there.

Thanks!
--Jacob



Re: [Commitfest 2022-07] Patch Triage: Needs Review, Part 1

From
Julien Rouhaud
Date:
Hi Jacob,

On Fri, Jul 29, 2022 at 10:08:08AM -0700, Jacob Champion wrote:
> 
> On Thu, Jul 28, 2022 at 11:38 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > > - Add extra statistics to explain for Nested Loop
> > >   https://commitfest.postgresql.org/38/2765/
> > >
> > > [...]
> >
> > As I mentioned in [1], this patch breaks the current assumption that
> > INSTRUMENT_ALL will lead to statement-level metrics that are generally useful.
> > According to the benchmark, the proposed patch would add a 1.5% overhead for
> > pg_stat_statements or any other similar extension that relies on INSTRUMENT_ALL
> > for no additional information, and I don't think it's acceptable.
> 
> (I'm missing the [1] link.)

Ah sorry I forgot to include it, here it's:
https://www.postgresql.org/message-id/20220307050830.zahd57wbvezu2d6r%40jrouhaud.

>From skimming the end of the thread, it
> looks like Ekaterina responded to that concern and was hoping for
> feedback. If you still think it doesn't go far enough, would you mind
> dropping a note in the thread? Then we can mark WoA and go from there.

I think that the problem still exist, unfortunately the benchmark done only
tests various EXPLAIN commands and not normal query execution with pgss
enabled.  I will double check and reply on the thread tomorrow!