Re: [HACKERS] Patches that don't apply or don't compile: 2017-09-12

[Thomas Munro]

On Fri, Sep 15, 2017 at 8:13 AM, Robert Haas  wrote:
> On Wed, Sep 13, 2017 at 5:18 AM, Alvaro Herrera  
> wrote:
>> Thinking further, I think changing patch status automatically may never
>> be a good idea; there's always going to be some amount of common sense
>> applied beforehand (such as if a conflict is just an Oid catalog
>> collision, or a typo fix in a recent commit, etc).  Such conflicts will
>> always raise errors with a tool, but would never cause a (decent) human
>> being from changing the patch status to WoA.
>
> Well it would perhaps be fine if sending an updated patch bounced it
> right back to Needs Review.  But if things are only being auto-flipped
> in one direction that's going to get tedious.
>
> Or one could imagine having the CF app show the CI status alongside
> the existing status column.

FWIW that last thing is the idea that I've been discussing with
Magnus.  That web page I made wouldn't exist, and the "apply" and
"build" badges would appear directly on the CF app and you'd be able
to sort and filter by them etc.  The main submission status would
still be managed by humans and the new apply and build statuses would
be managed by faithful robot servants.

How exactly that would work, I don't know.  One idea is that a future
cfbot, rewritten in better Python and adopted into the pginfra
universe, pokes CF via an HTTPS endpoint so that the CF app finishes
up with the information in its database.

-- 
Thomas Munro
http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patches that don't apply or don't compile: 2017-09-12

[Robert Haas]

On Wed, Sep 13, 2017 at 5:18 AM, Alvaro Herrera  wrote:
> Thinking further, I think changing patch status automatically may never
> be a good idea; there's always going to be some amount of common sense
> applied beforehand (such as if a conflict is just an Oid catalog
> collision, or a typo fix in a recent commit, etc).  Such conflicts will
> always raise errors with a tool, but would never cause a (decent) human
> being from changing the patch status to WoA.

Well it would perhaps be fine if sending an updated patch bounced it
right back to Needs Review.  But if things are only being auto-flipped
in one direction that's going to get tedious.

Or one could imagine having the CF app show the CI status alongside
the existing status column.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patches that don't apply or don't compile: 2017-09-12

[Aleksander Alekseev]

Hi Martin,

> > === Build Failed: 7 ===
> > Title: Fix the optimization to skip WAL-logging on table created in same 
> > transaction
> > Author: Martijn van Oosterhout 
> > URL: https://commitfest.postgresql.org/14/528/
> 
> I'm not the author of this patch, and the page doesn't say so.
> Something wrong with your script?

It's a bug. The authors were determined by the senders of the first
email in the corresponding thread. This is because I needed email list
to CC the authors but the commitfest application doesn't show emails.

Sorry for the disturbance.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Patches that don't apply or don't compile: 2017-09-12

[Tomas Vondra]

Hi Aleksander,

On 09/13/2017 11:49 AM, Aleksander Alekseev wrote:
> Hi Tomas,
> 
> I appreciate your feedback, although it doesn't seem to be completely
> fair. Particularly:
> 
>> You gave everyone about 4 hours to object
> 
> This is not quite accurate since my proposal was sent 2017-09-11
> 09:41:32 and this thread started - 2017-09-12 14:14:55.
> 

Understood. I didn't really consider the first message to be a proposal
with a deadline, as it starts with "here's a crazy idea" and it's not
immediately clear that you intend to proceed with it immediately, and
that you expect people to object.

The message I referenced is a much clearer on this.

>> You just changed the status of 10-15% open patches. I'd expect
>> things like this to be consulted with the CF manager, yet I don't see
>> any comments from Daniel.
> 
> Agree, that was clearly a mistake, I had to add Daniel to CC. Sorry I
> didn't do that. I've returned all affected patches back to "Needs
> Review". On the bright side while doing this I've noticed that many
> patches were already updated by their authors.
> 

Yeah.

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patches that don't apply or don't compile: 2017-09-12

[Daniel Gustafsson]

> On 13 Sep 2017, at 11:49, Aleksander Alekseev  
> wrote:
> 
> Hi Tomas,
> 
> I appreciate your feedback, although it doesn't seem to be completely
> fair.

I would like to stress one thing (and I am speaking only for myself here), this
has been feedback and not criticism.  Your (and everyone involved in this)
initiative is great and automation will no doubt make the CF process better.
We just need to finetune it a little to make it work for, as well as with, the
community.

cheers ./daniel

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patches that don't apply or don't compile: 2017-09-12

[Aleksander Alekseev]

Hi Tomas,

I appreciate your feedback, although it doesn't seem to be completely
fair. Particularly:

> You gave everyone about 4 hours to object

This is not quite accurate since my proposal was sent 2017-09-11
09:41:32 and this thread started - 2017-09-12 14:14:55.

> You just changed the status of 10-15% open patches. I'd expect
> things like this to be consulted with the CF manager, yet I don't see
> any comments from Daniel.

Agree, that was clearly a mistake, I had to add Daniel to CC. Sorry I
didn't do that. I've returned all affected patches back to "Needs
Review". On the bright side while doing this I've noticed that many
patches were already updated by their authors.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Patches that don't apply or don't compile: 2017-09-12

[Alvaro Herrera]

Aleksander Alekseev wrote:

> Agree, especially regarding build logs. All of this currently is only an
> experiment. For some reason I got a weird feeling that at this time it
> will be not quite successful one. If there will be too many false
> positives I'll just return the patches back to "Needs Review" as it was
> before. But I strongly believe that after a few iterations we will find
> a solution that will be good enough and this slight inconveniences will
> worth it in a long run.

Thinking further, I think changing patch status automatically may never
be a good idea; there's always going to be some amount of common sense
applied beforehand (such as if a conflict is just an Oid catalog
collision, or a typo fix in a recent commit, etc).  Such conflicts will
always raise errors with a tool, but would never cause a (decent) human
being from changing the patch status to WoA.

On the other hand, sending an email to the patch author (possibly CCing
the mailing list, incl. In-Reply-To headers as appropriate) notifying
them that there is a conflict would be very useful.  I think it would be
perfectly reasonable to automate that.  Include exactly what went wrong,
and of course the date where the problem was detected (in the email
headers) so that reviewers can see "this patch's author received a
notice and didn't act on it for two weeks" and take a decision to set it
WoA.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patches that don't apply or don't compile: 2017-09-12

[Kyotaro HORIGUCHI]

Hello, aside from the discussion on the policy of usage of
automation CI, it seems having trouble applying patches.

https://travis-ci.org/postgresql-cfbot/postgresql/builds/27450
>1363  heapam.c:2502:18: error: ‘HEAP_INSERT_SKIP_WAL’ undeclared (first use in 
>this function)
>1364  if (!(options & HEAP_INSERT_SKIP_WAL) && RelationNeedsWAL(relation))

These lines shows that the patch is applied halfway.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patches that don't apply or don't compile: 2017-09-12

[Kyotaro HORIGUCHI]

At Wed, 13 Sep 2017 08:13:08 +0900, Michael Paquier  
wrote in 

Re: [HACKERS] Patches that don't apply or don't compile: 2017-09-12

[Michael Paquier]

On Wed, Sep 13, 2017 at 7:39 AM, Daniel Gustafsson  wrote:
>> On 12 Sep 2017, at 23:54, Tomas Vondra  wrote:
>> With all due respect, it's hard not to see this as a disruption of the
>> current CF. I agree automating the patch processing is a worthwhile
>> goal, but we're not there yet and it seems somewhat premature.
>>
>> Let me explain why I think so:
>>
>> (1) You just changed the status of 10-15% open patches. I'd expect
>> things like this to be consulted with the CF manager, yet I don't see
>> any comments from Daniel. Considering he's been at the Oslo PUG meetup
>> today, I doubt he was watching hackers very closely.
>
> Correct, I’ve been travelling and running a meetup today so had missed this on
> -hackers.

FWIW, I tend to think that the status of a patch ought to be changed
by either a direct lookup at the patch itself or the author depending
on how the discussion goes on, not an automatic processing. Or at
least have more delay to allow people to object as some patches can be
applied, but do not apply automatically because of naming issues.
There are as well people sending test patches to allow Postgres to
fail on purpose, for example see the replication slot issue not able
to retain a past segment because the beginning of a record was not
tracked correctly on the receiver-side. This can make the recovery
tests fail, but we want them to fail to reproduce easily the wanted
failure.

>> (2) You gave everyone about 4 hours to object, ending 3PM UTC, which
>> excludes about one whole hemisphere where it's either too early or too
>> late for people to respond. I'd say waiting for >24 hours would be more
>> appropriate.
>
> Agreed.

Definitely. Any batch updates have to involve the CFM authorization at
least, in this case Daniel.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patches that don't apply or don't compile: 2017-09-12

[Daniel Gustafsson]

> On 12 Sep 2017, at 23:54, Tomas Vondra  wrote:
> 
> On 09/12/2017 04:14 PM, Aleksander Alekseev wrote:
>> Hello, hackers!
>> 
>> Thanks to the work of Thomas Munro now we have a CI for the patches on
>> the commitfest [1]. Naturally there is still room for improvement, but
>> in any case it's much, much better than nothing.
>> 
>> After a short discussion [2] we agreed (or at least no one objected)
>> to determine the patches that don't apply / don't compile / don't
>> pass regression tests and have "Needs Review" status, change the
>> status of these patches to "Waiting on Author" and write a report
>> (this one) with a CC to the authors. As all we know, we are short on
>> reviewers and this action will save them a lot of time. Here [3] you
>> can find a script I've been using to find such patches.
>> 
>> I rechecked the list manually and did my best to exclude the patches 
>> that were updated recently or that depend on other patches. However 
>> there still a chance that your patch got to the list by a mistake.
>> In this case please let me know.>
> 
> With all due respect, it's hard not to see this as a disruption of the
> current CF. I agree automating the patch processing is a worthwhile
> goal, but we're not there yet and it seems somewhat premature.
> 
> Let me explain why I think so:
> 
> (1) You just changed the status of 10-15% open patches. I'd expect
> things like this to be consulted with the CF manager, yet I don't see
> any comments from Daniel. Considering he's been at the Oslo PUG meetup
> today, I doubt he was watching hackers very closely.

Correct, I’ve been travelling and running a meetup today so had missed this on
-hackers.

> (2) You gave everyone about 4 hours to object, ending 3PM UTC, which
> excludes about one whole hemisphere where it's either too early or too
> late for people to respond. I'd say waiting for >24 hours would be more
> appropriate.

Agreed.

> I object to changing the patch status merely based on the script output.
> It's a nice goal, but we need to do the legwork first, otherwise it'll
> be just annoying and disrupting.

I too fear that automating the state change will move patches away from “Needs
review” in too many cases unless there is manual inspection step.  Colliding on
Oids in pg_proc comes to mind as a case where the patch won’t build, but the
reviewer can trivially fix that locally and keep reviewing.

> I suggest we inspect the reported patches manually, investigate whether
> the failures are legitimate or not, and eliminate as many false
> positives as possible. Once we are happy with the accuracy, we can
> enable it again.

This seems to summarize the sentiment in the other thread, this is absolutely a
step in the right direction, we just need to tweak it with human knowledge
before it can be made fully automatic to avoid false positives.  The last thing
we want is for the community to consider CF status changes/updates to be crying
wolf, there are few enough reviewers as there is.

cheers ./daniel

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patches that don't apply or don't compile: 2017-09-12

[Tomas Vondra]

On 09/12/2017 04:14 PM, Aleksander Alekseev wrote:
> Hello, hackers!
> 
> Thanks to the work of Thomas Munro now we have a CI for the patches on
> the commitfest [1]. Naturally there is still room for improvement, but
> in any case it's much, much better than nothing.
> 
> After a short discussion [2] we agreed (or at least no one objected)
> to determine the patches that don't apply / don't compile / don't
> pass regression tests and have "Needs Review" status, change the
> status of these patches to "Waiting on Author" and write a report
> (this one) with a CC to the authors. As all we know, we are short on
> reviewers and this action will save them a lot of time. Here [3] you
> can find a script I've been using to find such patches.
> 
> I rechecked the list manually and did my best to exclude the patches 
> that were updated recently or that depend on other patches. However 
> there still a chance that your patch got to the list by a mistake.
> In this case please let me know.>

With all due respect, it's hard not to see this as a disruption of the
current CF. I agree automating the patch processing is a worthwhile
goal, but we're not there yet and it seems somewhat premature.

Let me explain why I think so:

(1) You just changed the status of 10-15% open patches. I'd expect
things like this to be consulted with the CF manager, yet I don't see
any comments from Daniel. Considering he's been at the Oslo PUG meetup
today, I doubt he was watching hackers very closely.

(2) You gave everyone about 4 hours to object, ending 3PM UTC, which
excludes about one whole hemisphere where it's either too early or too
late for people to respond. I'd say waiting for >24 hours would be more
appropriate.

(3) The claim that "on one objected" is somewhat misleading, I guess. I
myself objected to automating this yesterday, and AFAICS Thomas Munro
shares the opinion that we're not ready for automating it.

(4) You say you rechecked the list manually - can you elaborate what you
checked? Per reports from others, some patches seem to "not apply"
merely because "git apply" is quite strict. Have you actually tried
applying / compiling the patches yourself?

(5) I doubt "does not apply" is actually sufficient to move the patch to
"waiting on author". For example my statistics patch was failing to
apply merely due to 821fb8cdbf lightly touching the SGML docs, changing
"type" to "kind" on a few places. Does that mean the patch can't get any
reviews until the author fixes it? Hardly. But after switching it to
"waiting on author" that's exactly what's going to happen, as people are
mostly ignoring patches in that state.

(6) It's generally a good idea to send a message the patch thread
whenever the status is changed, otherwise the patch authors may not
notice the change for a long time. I don't see any such messages,
certainly not in "my" patch thread.

I object to changing the patch status merely based on the script output.
It's a nice goal, but we need to do the legwork first, otherwise it'll
be just annoying and disrupting.

I suggest we inspect the reported patches manually, investigate whether
the failures are legitimate or not, and eliminate as many false
positives as possible. Once we are happy with the accuracy, we can
enable it again.


kind regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patches that don't apply or don't compile: 2017-09-12

[Thomas Munro]

On Wed, Sep 13, 2017 at 2:55 AM, Andreas Karlsson  wrote:
> On 09/12/2017 04:14 PM, Aleksander Alekseev wrote:
>>
>> Title: Foreign Key Arrays
>> Author: Mark Rofail
>> URL:https://commitfest.postgresql.org/14/1252/
>
>
> I am currently reviewing this one and it applies, compiles, and passes the
> test suite. It could be the compilation warnings which makes the system
> think it failed, but I could not find the log of the failed build.

I guess you didn't run "make check-world", because it crashes in the
contrib regression tests:

https://travis-ci.org/postgresql-cfbot/postgresql/builds/274732512

Sorry that the build logs are a bit hard to find at the moment.
Starting from http://commitfest.cputube.org/ if you click the
"build|failing" badge you'll land at
https://travis-ci.org/postgresql-cfbot/postgresql/branches and then
you have to locate the right branch, in this case commitfest/14/1252,
and then click the latest build link which (in this case) looks like
"# 4603 failed".  Eventually I'll have time to figure out how to make
the "build|failing" badge take you there directly...   Eventually I'll
also teach it how to dump a backtrace out of gdb the tests leave a
smouldering core.

-- 
Thomas Munro
http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patches that don't apply or don't compile: 2017-09-12

[Aleksander Alekseev]

Hi Andreas,

On 09/12/2017 04:14 PM, Aleksander Alekseev wrote:
> > Title: Foreign Key Arrays
> > Author: Mark Rofail
> > URL:https://commitfest.postgresql.org/14/1252/
> 
> I am currently reviewing this one and it applies, compiles, and passes the
> test suite. It could be the compilation warnings which makes the system
> think it failed, but I could not find the log of the failed build.

Thanks for your feedback. I'm changing the status of this patch back to
"Needs Review" and adding it to the exclude list until we will figure
out what went wrong.

> We want to be welcoming to new contributors so until we have a reliable CI
> server which can provide easy to read build logs I am against changing the
> status of any patches solely based on the result of the CI server. I think
> it should be used as a complimentary tool until the community deems it to be
> good enough.

Agree, especially regarding build logs. All of this currently is only an
experiment. For some reason I got a weird feeling that at this time it
will be not quite successful one. If there will be too many false
positives I'll just return the patches back to "Needs Review" as it was
before. But I strongly believe that after a few iterations we will find
a solution that will be good enough and this slight inconveniences will
worth it in a long run.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Patches that don't apply or don't compile: 2017-09-12

[Andreas Karlsson]

On 09/12/2017 04:14 PM, Aleksander Alekseev wrote:

Title: Foreign Key Arrays
Author: Mark Rofail
URL:https://commitfest.postgresql.org/14/1252/


I am currently reviewing this one and it applies, compiles, and passes 
the test suite. It could be the compilation warnings which makes the 
system think it failed, but I could not find the log of the failed build.


We want to be welcoming to new contributors so until we have a reliable 
CI server which can provide easy to read build logs I am against 
changing the status of any patches solely based on the result of the CI 
server. I think it should be used as a complimentary tool until the 
community deems it to be good enough.


Andreas


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers