Обсуждение: Commitfest 2021-11 Patch Triage - Part 1

We have amassed quite a lot of patches in the CF app, and while Jaime did a
very good job closing patches in the last CF there is still a lot to go
through.  I've attempted a brief per-patch triage here to see where we are with
these.  Reading every email in the threads for these patches is a herculean
task, so I may very well have misunderstood things.  If so I apologize and
please do jump and correct me.  If not, then please also jump in and comment
below (or on the relevant threads) to try and get closure.

While I don't have the expertise to replicate Andres' famous patch triage [0]
from a few years back, below is at least a try.  These are the patches with 7
or more commitfests under their belts (the record holder having 18), the next
installment will be 6 CF's and down limited by when the Shiraz runs out.


1608: schema variables, LET command
===================================
After 18 CF's and two very long threads it seems to be nearing completion
judging by Tomas' review.  There is an ask in that review for a second pass
over the docs by a native speaker, any takers?


1741: Index Skip Scan
=====================
An often requested feature which has proven hard to reach consensus on an
implementation for.  The thread(s) have stalled since May, is there any hope of
taking this further?  Where do we go from here with this patch?


1712: Remove self join on a unique column
=========================================
This has moved from "don't write bad queries" to "maybe we should do something
about this".  It seems like there is concensus that it can be worth paying the
added planner cost for this, especially if gated by a GUC to keep it out of
installations where it doesn't apply.  The regression on large join queries
hasn't been benchmarked it seems (or I missed it), but the patch seems to have
matured and be quite ready.  Any takers on getting it across the finish line?


2161: standby recovery fails when re-replaying due to missing directory which
was removed in previous replay.
=============================================================================
Tom and Robert seem to be in agreement that parts of this patchset are good,
and that some parts are not.  The thread has stalled and the patch no longer
apply, so unless someone feels like picking up the good pieces this seems like
a contender to close for now.


2138: Incremental Materialized View Maintenance
===============================================
There seems to be concensus on the thread that this is a feature that we want,
and after initial design discussions there seems to be no disagreements with
the approach taken.  The patch was marked ready for committer almost a year
ago, but have since been needs review (which seems correct).  The size of the
patchset and the length of the thread make it hard to gauge just far away it
is, maybe the author or a review can summarize the current state and outline
what is left for it to be committable.


2215: pg_upgrade fails with non-standard ACL
============================================
This thread has been stalled since March with open questions from Noah, but the
patch seems to be in pretty good shape.  Do you have any thoughts on the state
of this patch Noah? Seems like a pretty good fix to get in.


2218: Implement INSERT SET syntax
=================================
The author has kept this patch updated, and has seemingly updated according to
the review comments.  Tom: do you have any opinions on whether the updated
version addresses your concerns wrt the SELECT rewrite?


2048: SQL:2011 application time
===============================
The patch no longer applies, but there is a semi-active review discussion
ongoing.  It doesn't seem like the patch is nearing completion, but the feature
would no doubt be welcome by many. Is this realistic for 15?


2316: WITH SYSTEM VERSIONING Temporal Tables
============================================
This patch no longer applies, and judging by the thread there is a pretty major
re-design ongoing.  I think we should close this entry and open a new one when
there is a new patch rather than push it along.  What are your thoughts Simon?


2096: psql - add SHOW_ALL_RESULTS option
========================================
Peter posted an updated version of Fabiens patch about a month ago (which at
this point no longer applies) fixing a few issues, but also point at old review
comments still unadressed.  Since this was pushed, but had to be reverted, I
assume there is a desire for the feature but it seems to need more work still.


1651: GROUP BY optimization
===========================
This is IMO a desired optimization, and after all the heavy lifting by Tomas
the patch seems to be in pretty good shape.


2377: pg_ls_* functions for showing metadata and recurse (pg_ls_tmpdir to show
shared filesets)
==============================================================================
The question of what to do with lstat() on Windows is still left unanswered,
but the patchset has been split to up to be able to avoid it.  Stephen and Tom,
having done prior reviews do you have any thoughts on this?


2349: global temporary table
============================
GTT has been up for discussion numerous times in tbe past, and I can't judge
whether this proposal has a better chance than previous ones.  I do note the
patch has a number of crashes reported lately, and no reviews from senior
contributors in a while, making it seem unlikely to be committed in this CF.
Since the patch is very big, can it be teased apart and committed separately
for easier review?


2433: Erase the distinctClause if the result is unique by definition
====================================================================
(parts of) The approach taken in this patch has been objected against in favor
of work that Tom has proposed.  Until that work materialize this patch is
blocked, and thus I think we are better of closing it and re-opening it when it
gets unstuck.  Unless Tom has plans to hack on this shortly?


2482: jsonpath syntax extensions
================================
This thread has stalled, with no reviews so far a good year and half after
being.  Rather than continue to push this along, should we instead close it due
to lack of interest?  It's pretty big, can it be broken up into smaller
features for easier review?


2490: Make message at end-of-recovery less scary
================================================
This thread stalled, but has had recent interest.  The patch no longer applies
so while the patch has support it will be marked as returned with feedback
during this CF unless revived.


2573: pg_dump - read data for some options from external file
=============================================================
This patch has been heavily debated, but I think we've landed in a compromise
that has been agreed upon.  The gist of the debate has been around the file
format and it's extensibility, the final version proposing an extensible yet
simple format which can be referenced from a larger configfile (rather than
making the configfile now).  Having done a lot of review myself I think the
patch is ready with one exception: IMO the parsing code isn't maintainable
enough to be committed (if another committer disagree with that, feel free to
jump in).  I have on my TODO to implement it as a Bison parser to see what that
would look like.


2553: should INSERT SELECT use a BulkInsertState?
==================================================
The conclusion on the thread seems to be that there is little support for it
due to the risk of regressions, and that closing the patch might be the best
course of action.  If anyone is in favor of going ahead with it, now is the
time to speak up.


2518: Corruption during WAL replay
==================================
Both Robert and Tom have given positive remarks about this, and recent review
concerns raised turned out to have been handled.  Robert, Tom: any thoughts on
the latest posted version?


2601: Fast COPY FROM command for the foreign tables
===================================================
This approach taken in this patch has stabilized and the benchmarks posted are
very promising.  It seems pretty uncontroversial to add COPY to the FDW API SQL
spec wise.  Amit, Justin and Takayuki-san has done a lot of review and the
patch is marked Ready for Committer.  Any committer interested in taking this
on?  Tomas?


2602: ALTER SYSTEM READ { ONLY | WRITE }
========================================
AFAIK Robert is actively working on this and have committed parts of it
already, so there isn't much to add.  I'm personally quite excited about this
feature so I'm looking forward to (hopefully) seeing it go in.  Is the full
feature likely to make 15?


2627: More scalable multixacts buffers and locking
==================================================
Skimming the thread makes it seem like this patch is still undergoing some
fairly big changes.  Thomas and Andrey, how far along is this from RFC?


2716: fix spinlock contention in LogwrtResult
=============================================
This addresses a bottleneck which definitely seems like one we want to fix, I
don't have a hard time imagining it impacting other production usecases then
the reported one.  The goalposts moved early in the thread, and while there has
been work on it the patch has stalled on Andres' review which raised some
concerns.  Are you still pursuing this Alvaro?


2684: enhancing plpgsql API for debugging and tracing
======================================================
This hasn't seen much review, but what has been performed has been positive.
It does add some weight to plpgsql by passing the NS around, but maybe the
effect is negligable.  Given the size and complexity of the test module to use
this, it's perhaps not obvious to see who the target audience is.  Maybe a
better option would be to polish the test module and propose it for contrib/
along with the patch, and write the tests on using said module?


2710: Fix behavior of geo_ops when NaN is involved
==================================================
Two thirds of this bugfix patchset have been committed already, with the thread
stalling on the remaining bit.  Tom, Georgios: any thoughts on the last patch?


2694: Automatic HASH and LIST partition creation
================================================
The subject of automatic partition creation is pretty hot, so getting something
done in this area would be good.  This proposal has gotten criticism over the
syntax chosen, and the thread has effectively stalled after Robert's review in
July.  It seems like this patch should be returned with feedback, and any new
attempt at this should start by just discussing the syntax and only once agreed
upon hack on the code.


Thanks for reading all this way, I hope it did something to help progress.

--
Daniel Gustafsson        https://vmware.com/

[0]
https://www.postgresql.org/message-id/flat/20180302075242.yfqkcgzbrmjysboa%40alap3.anarazel.de#b5015aab4ba3a95b67636faef74a1d51

Thanks for taking care of this.

> 2433: Erase the distinctClause if the result is unique by definition
> ====================================================================
> (parts of) The approach taken in this patch has been objected against in favor
> of work that Tom has proposed.

Actually no exactly.  The 90% of the code is about UniqueKey and UniqueKey
needs a notnulls representation of Var during join,  Tom is objecting
that part only.


> Until that work materialize this patch is
> blocked, and thus I think we are better of closing it and re-opening it when it
> gets unstuck.  Unless Tom has plans to hack on this shortly?

Both Tom and David  have provided many insights on this topic.  I'd like to hear
the decision from Tom and David.

-- 
Best Regards
Andy Fan (https://www.aliyun.com/)

> 1741: Index Skip Scan
> =====================
> An often requested feature which has proven hard to reach consensus on an
> implementation for.  The thread(s) have stalled since May,

This statement is not accurate.  Peter started a new thread in [1] for this
topic last month and then we had a discussion. Thanks for taking
care of this.

[1] https://www.postgresql.org/message-id/flat/CAH2-WzmUscvoxVkokHxP%3DuPTDjSi0tJkFpUPD-CeA35dvn-CMw%40mail.gmail.com

is there any hope of
> taking this further?  Where do we go from here with this patch?
>

-- 
Best Regards
Andy Fan (https://www.aliyun.com/)

On 11/5/21 22:15, Daniel Gustafsson wrote:
> ...
> 1651: GROUP BY optimization
> ===========================
> This is IMO a desired optimization, and after all the heavy lifting by Tomas
> the patch seems to be in pretty good shape.
> 

I agree it's desirable. To move the patch forward, I need some feedback 
on the costing questions, mentioned in my last review, and the overall 
approach to planning (considering multiple group by variants).

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sat, Nov 6, 2021 at 6:16 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> 2601: Fast COPY FROM command for the foreign tables
> ===================================================
> This approach taken in this patch has stabilized and the benchmarks posted are
> very promising.  It seems pretty uncontroversial to add COPY to the FDW API SQL
> spec wise.  Amit, Justin and Takayuki-san has done a lot of review and the
> patch is marked Ready for Committer.  Any committer interested in taking this
> on?  Tomas?

I’d like to work on this unless Tomas (or anyone else) want.

Best regards,
Etsuro Fujita

> On 7 Nov 2021, at 10:26, Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>
> On Sat, Nov 6, 2021 at 6:16 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>> 2601: Fast COPY FROM command for the foreign tables
>> ===================================================
>> This approach taken in this patch has stabilized and the benchmarks posted are
>> very promising.  It seems pretty uncontroversial to add COPY to the FDW API SQL
>> spec wise.  Amit, Justin and Takayuki-san has done a lot of review and the
>> patch is marked Ready for Committer.  Any committer interested in taking this
>> on?  Tomas?
>
> I’d like to work on this unless Tomas (or anyone else) want.

Great, thanks for picking it up!

--
Daniel Gustafsson        https://vmware.com/

> On 6 Nov 2021, at 17:20, Tomas Vondra <tomas.vondra@enterprisedb.com> wrote:
>
> On 11/5/21 22:15, Daniel Gustafsson wrote:
>> ...
>> 1651: GROUP BY optimization
>> ===========================
>> This is IMO a desired optimization, and after all the heavy lifting by Tomas
>> the patch seems to be in pretty good shape.
>
> I agree it's desirable.  To move the patch forward, I need some feedback on the
> costing questions,


Tom or David perhaps have some thoughts on that?

--
Daniel Gustafsson        https://vmware.com/

> On 6 Nov 2021, at 02:12, Andy Fan <zhihui.fan1213@gmail.com> wrote:
>
>> 1741: Index Skip Scan
>> =====================
>> An often requested feature which has proven hard to reach consensus on an
>> implementation for.  The thread(s) have stalled since May,
>
> This statement is not accurate.  Peter started a new thread in [1] for this
> topic last month and then we had a discussion. Thanks for taking
> care of this.

Aha, I had missed that.  Thanks to Dimitry for attaching this thread to the CF
entry to make that clearer going forward.

--
Daniel Gustafsson        https://vmware.com/

On 2021-Nov-05, Daniel Gustafsson wrote:

> 2716: fix spinlock contention in LogwrtResult
> =============================================
> This addresses a bottleneck which definitely seems like one we want to fix, I
> don't have a hard time imagining it impacting other production usecases then
> the reported one.  The goalposts moved early in the thread, and while there has
> been work on it the patch has stalled on Andres' review which raised some
> concerns.  Are you still pursuing this Alvaro?

I am, but not in the immediate future.  I am very absorbed by trying to
make MERGE work again for resubmission, so I haven't had time to invest
in these other patches.  I hope I'll be able to return with a new
version of this for the January commitfest.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"On the other flipper, one wrong move and we're Fatal Exceptions"
(T.U.X.: Term Unit X  - http://www.thelinuxreview.com/TUX/)

Thanks for the summary.

At Fri, 5 Nov 2021 22:15:49 +0100, Daniel Gustafsson <daniel@yesql.se> wrote in 
> 2490: Make message at end-of-recovery less scary
> ================================================
> This thread stalled, but has had recent interest.  The patch no longer applies
> so while the patch has support it will be marked as returned with feedback
> during this CF unless revived.

I'll soon repost a rebased version.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, Nov 05, 2021 at 10:15:49PM +0100, Daniel Gustafsson wrote:
> 2161: standby recovery fails when re-replaying due to missing directory which
> was removed in previous replay.
> =============================================================================
> Tom and Robert seem to be in agreement that parts of this patchset are good,
> and that some parts are not.  The thread has stalled and the patch no longer
> apply, so unless someone feels like picking up the good pieces this seems like
> a contender to close for now.

That's in my area, so I have signed up as reviewer.

> 2518: Corruption during WAL replay
> ==================================
> Both Robert and Tom have given positive remarks about this, and recent review
> concerns raised turned out to have been handled.  Robert, Tom: any thoughts on
> the latest posted version?

The latest comments are from a couple of weeks ago.  I'll take a look.

> 2602: ALTER SYSTEM READ { ONLY | WRITE }
> ========================================
> AFAIK Robert is actively working on this and have committed parts of it
> already, so there isn't much to add.  I'm personally quite excited about this
> feature so I'm looking forward to (hopefully) seeing it go in.

So am I.
--
Michael

At Mon, 8 Nov 2021 14:43:43 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Fri, Nov 05, 2021 at 10:15:49PM +0100, Daniel Gustafsson wrote:
> > 2161: standby recovery fails when re-replaying due to missing directory which
> > was removed in previous replay.
> > =============================================================================
> > Tom and Robert seem to be in agreement that parts of this patchset are good,
> > and that some parts are not.  The thread has stalled and the patch no longer
> > apply, so unless someone feels like picking up the good pieces this seems like
> > a contender to close for now.
> 
> That's in my area, so I have signed up as reviewer.

I noticed I'm one of the author^^; I'll also look into the comments
and try to address them.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Nov 08, 2021 at 03:07:43PM +0900, Kyotaro Horiguchi wrote:
> I noticed I'm one of the author^^; I'll also look into the comments
> and try to address them.

Cool, thanks.  I'll come back to this thread from this point, then.
--
Michael

On Fri, 5 Nov 2021 22:15:49 +0100
Daniel Gustafsson <daniel@yesql.se> wrote:

> 2138: Incremental Materialized View Maintenance
> ===============================================
> There seems to be concensus on the thread that this is a feature that we want,
> and after initial design discussions there seems to be no disagreements with
> the approach taken.  The patch was marked ready for committer almost a year
> ago, but have since been needs review (which seems correct).  The size of the
> patchset and the length of the thread make it hard to gauge just far away it
> is, maybe the author or a review can summarize the current state and outline
> what is left for it to be committable.

Thanks for taking care of this.

I've responded to it in the following thread, and described the recent discussions,
current status, summary of IVM  feature and design, and past discussions.

https://www.postgresql.org/message-id/20211129144826.c9d42c1f61495c6983d8b6b1%40sraoss.co.jp

Regards,
Yugo Nagata

-- 
Yugo NAGATA <nagata@sraoss.co.jp>

On Mon, 29 Nov 2021 09:03:06 -0300
Marcos Pegoraro <marcos@f10.com.br> wrote:

> >
> > > 2138: Incremental Materialized View Maintenance
> >
> > I've responded to it in the following thread, and described the recent
> > discussions,
> > current status, summary of IVM  feature and design, and past discussions.
> >
> 
> IVM is a wonderful feature, but some features were omitted because of
> its complexity, so maybe IVM will spend more time to completely solve what
> it wants to do.
> I did not understand, and didn´t find on docs, if a Materialized View is a
> table, why I cannot change a specific record ?
> Because if I have a way to create and refresh the entire view and update a
> single record it would give me all power of IVM is trying to.

I think the reason why we can't update a materialized view directly is because
it is basically a "view" and it should not contains any data irrelevant to its
definition and underlying tables. If we would have a feature to update a
materialized view direcly,  maybe, it should behave as updatable-view as well
as normal (virtual) views, although I am not sure....


-- 
Yugo NAGATA <nagata@sraoss.co.jp>

> On 6 Nov 2021, at 17:20, Tomas Vondra <tomas.vondra@enterprisedb.com> wrote:
>
> On 11/5/21 22:15, Daniel Gustafsson wrote:
>> ...
>> 1651: GROUP BY optimization
>> ===========================
>> This is IMO a desired optimization, and after all the heavy lifting by Tomas
>> the patch seems to be in pretty good shape.
>
> I agree it's desirable. To move the patch forward, I need some feedback on the costing questions, mentioned in my
lastreview, and the overall approach to planning (considering multiple group by variants). 

I'm moving this to the next CF with the hope that more feedback is received.
If you think "Ready for Committer" is a more suitable status, please do update
it as I'm leaving it in Needs Review for now not having the full context.

--
Daniel Gustafsson        https://vmware.com/

On 12/2/21 12:06, Daniel Gustafsson wrote:
>> On 6 Nov 2021, at 17:20, Tomas Vondra <tomas.vondra@enterprisedb.com> wrote:
>>
>> On 11/5/21 22:15, Daniel Gustafsson wrote:
>>> ...
>>> 1651: GROUP BY optimization
>>> ===========================
>>> This is IMO a desired optimization, and after all the heavy lifting by Tomas
>>> the patch seems to be in pretty good shape.
>>
>> I agree it's desirable. To move the patch forward, I need some feedback on the costing questions, mentioned in my
lastreview, and the overall approach to planning (considering multiple group by variants).
 
> 
> I'm moving this to the next CF with the hope that more feedback is received.
> If you think "Ready for Committer" is a more suitable status, please do update
> it as I'm leaving it in Needs Review for now not having the full context.

I don't think anyone reviewed the costing, so I think "needs review" is 
appropriate.

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, 1 Dec 2021 09:14:31 -0300
Marcos Pegoraro <marcos@f10.com.br> wrote:

> >
> > I think the reason why we can't update a materialized view directly is
> > because
> > it is basically a "view" and it should not contains any data irrelevant to
> > its
> > definition and underlying tables. If we would have a feature to update a
> > materialized view direcly,  maybe, it should behave as updatable-view as
> > well
> > as normal (virtual) views, although I am not sure....
> >
> 
> Well, I didn´t find any place where is detailed why those tables are not
> updatable.
> And would be fine to be updated through triggers or cron jobs until IVM is
> available.
> CheckValidRowMarkRel just gives an exception "cannot lock rows in
> materialized view ...", but why ?
> What are the differences between Materialized Views and tables ?

It would be that materialized views are related to its definition query
and expected to have contents that is consistent with it,  at least at some
point in time, I think.

In order to allow triggers to update materialized views, we'd need to
make OpenMatViewIncrementalMaintenance and CloseMatViewIncrementalMaintenance
public since there are static functions in matview.c. However, there is a
concern that it will make the contents of a materialized view completely
unreliable [1]. Therefore, if we do it, we would need privilege management
in some way.

[1]
https://www.postgresql.org/message-id/flat/CACjxUsP8J6bA4RKxbmwujTVMwMZrgR3AZ7yP5F2XkB-f9w7K7Q%40mail.gmail.com#efbee336d7651ce39bc5ff9574f92002

Regards,
Yugo Nagata
-- 
Yugo NAGATA <nagata@sraoss.co.jp>