Thread: Commitfest remaining "Needs Review" items

Hello Hackers,

There are a few "Needs Review" items remaining in the July commitfest. 
Reviewers, please take action - you are holding up the commitfest.

In addition to these items, there are a bunch of items in "Ready for 
Committer" state. Committers: please help with those.

> * extends pgbench expressions with functions

Robert signed up as reviewer for this a long time ago. Ping, do you 
still plan to review this?

> * self-defined policy for sepgsql regression test

Stephen: would you have the time to handle this, please?

> * Allocate restart_lsn at the time of creating physical replication slot

Andres, can you have a look at this, please?

> * Parallel Seq scan

Jeff, Haribabu, you're signed up as reviewers. How's it going?

> * Join pushdown support for foreign tables

Ashutosh, KaiGai, Etsuro: You signed up as reviewers in spring, but 
there hasn't been any activity during this commitfest. What's the status 
of this patch? Do you plan to review this now?

> * replace pg_stat_activity.waiting with something more descriptive

This is not quite ready yet, but has had its fair share of review, so 
I'm going to move it to the next commitfest. Feel free to continue the 
discussion and review, though; I just don't want drag the commitfest 
because for this.

> * Unique Joins

Still needs to be reviewed. Any volunteers?

> * checkpoint continuous flushing

Per discussion on that thread, let's just do the sorting part now. Needs 
some cleanup, but it's almost there.

- Heikki

On 08/10/2015 10:46 AM, Atri Sharma wrote:
>>
>> * Unique Joins
>>
>> Still needs to be reviewed. Any volunteers?
>
> Can take this one up, if its within my limits.

Thanks! I've added you as reviewer to that.

- Heikki

On 2015/08/10 17:10, Ashutosh Bapat wrote:
> On Mon, Aug 10, 2015 at 1:04 PM, Heikki Linnakangas <hlinnaka@iki.fi
> <mailto:hlinnaka@iki.fi>> wrote:
>         * Join pushdown support for foreign tables

>     Ashutosh, KaiGai, Etsuro: You signed up as reviewers in spring, but
>     there hasn't been any activity during this commitfest. What's the
>     status of this patch? Do you plan to review this now?

> There are three patches involved here. I have reviewed one of them. I am
> planning to look at them within few days.

Sorry for not having taken any action.

I think we should address the issue dicussed in [1] first because I 
think that would affect the design of join pushdown API in the core. 
I'll submit a patch for that within a few days.  I'd like to review this 
in detail after addressing that, if it's OK.

Best regards,
Etsuro Fujita

[1] http://www.postgresql.org/message-id/558A18B3.9050201@lab.ntt.co.jp

>> * extends pgbench expressions with functions
>
> Robert signed up as reviewer for this a long time ago. Ping, do you still 
> plan to review this?

I seem to recall that I nominated Robert when adding the patch, because
this is an extension of his expression patch. So the "sign up" is really
just a hint.

>> * checkpoint continuous flushing
>
> Per discussion on that thread, let's just do the sorting part now. Needs some 
> cleanup, but it's almost there.

Given the performance improvements (yes some tps, but also latency), I 
think that the full patch deserves consideration.

-- 
Fabien.

Heikki,

* Heikki Linnakangas (hlinnaka@iki.fi) wrote:
> >* self-defined policy for sepgsql regression test
>
> Stephen: would you have the time to handle this, please?

We'll definitely get this addressed soon.  Apologies for taking so long
to get to it, was in Brazil last week and over the weekend and then had
a bit of civic duty today (Jury duty).
Thanks!
    Stephen

On Mon, Aug 10, 2015 at 4:34 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> Hello Hackers,
>
> There are a few "Needs Review" items remaining in the July commitfest.
> Reviewers, please take action - you are holding up the commitfest.
>
> In addition to these items, there are a bunch of items in "Ready for
> Committer" state. Committers: please help with those.

At this stage, there are currently 26 patches in need of actions for
the current CF:
- 12 patches are waiting on author:
-- Let pg_ctl check the result of a postmaster config reload: returned
with feedback? Heikki has provided input on this patch but the patch
has not been updated by the author since last month.
-- Allow specifying multiple hosts in libpq connect options: returned
with feedback? A review has been sent but no new versions have been
submitted.
-- multivariate statistics: bump to next CF?
-- merging pgbench logs: returned with feedback or bump? Fabien has
concerns about performance regarding fprintf when merging the logs.
Fabien, Tomas, thoughts?
-- pgbench - per-transaction and aggregated logs: returned with
feedback or bump to next CF? Fabien, Tomas, thoughts?
-- Add sample rate to auto_explain: returned with feedback because of
lack of activity?
-- Backpatch Resource Owner reassign locks cache for the sake of
pg_upgrade: a committer may want to look at that for a backpatch in <=
9.2.
-- Compensate for full_page_writes when spreading checkpoint I/O: Bump
to next CF? Heikki, are you planning to continue working on that?
-- Improving replay of XLOG_BTREE_VACUUM records: returned with
feedback? There has been review input from 2 committers but no updated
versions.
-- compress method for spgist : returned with feedback?
-- Support to COMMENT ON CURRENT DATABASE: returned with feedback? The
author mentioned that patch will be reworked but there has been no new
version around it seems.
-- Default Roles: Stephen, are you planning to work on that for next CF?

- 8 patches are in need of review:
-- extends pgbench expressions with functions: bump to next CF, v9 has
not been reviewed.
-- check existency of table for -t option (pg_dump) when pattern has
no wildcard: Bump to next CF?
-- self-defined policy for sepgsql regression test, returned with
feedback? The regressions are broken as mentioned at the end of the
thread.
-- Unique Joins: bump to next CF?
-- improving join estimates using FK: bump to next CF?
-- checkpoint continuous flushing, bump, work, performance tests and
review are all still going on.
-- Join pushdown support for foreign tables: err... This patch had no
activity for 4 months.
-- Reload SSL certificates on SIGHUP: returned with feedback? I think
that this patch needs more work to be in a commitable state.

- 6 patches marked as ready for committer:
-- numeric timestamp in log_line_prefix
-- plpgsql raise statement with context
-- postgres_fdw: Options to set fetch_size at the server and table level.
-- Improving test coverage of extensions with pg_dump
-- New functions in sslinfo module
-- CREATE EXTENSION ... CASCADE
For all of them, bump to next CF with the same status if they are not
committed at the end of the month?
Regards,
-- 
Michael

> -- merging pgbench logs: returned with feedback or bump? Fabien has
> concerns about performance regarding fprintf when merging the logs.
> Fabien, Tomas, thoughts?
> -- pgbench - per-transaction and aggregated logs: returned with
> feedback or bump to next CF? Fabien, Tomas, thoughts?

I think that both features are worthwhile so next CF would be better, but 
it really depends on Tomas.

The key issue was the implementation complexity and maintenance burden 
which was essentially driven by fork-based thread emulation compatibility, 
but it has gone away as the emulation has been taken out of pgbench and it 
is now possible to do a much simpler implementation of these features.

The performance issue is that if you have many threads which perform 
monstruous tps and try to log them then logging becomes a bottle neck, 
both the "printf" time and the eventual file locking... Well, that is 
life, it is well know that experimentators influence experiments they are 
looking at since Schrödinger, and moreover the --sampling-rate option is 
already here to alleviate this problem if needed, so I do not think that 
it is an issue to address by keeping the code complex.

-- 
Fabien.

On Tue, Aug 25, 2015 at 5:51 PM, Heikki Linnakangas wrote:
> On 08/25/2015 10:39 AM, Michael Paquier wrote:
>> - 12 patches are waiting on author:
>
> These can all be marked as Returned with good conscience, they've gotten at
> least some feedback.

Fine for me. I'll notify each thread individually and update the
status of each patch.

>> - 8 patches are in need of review:
>> [...]
>>
>> - 6 patches marked as ready for committer:
>> [...]
>> For all of them, bump to next CF with the same status if they are not
>> committed at the end of the month?
>
>
> Yeah, I don't think we can let this linger much longer. I hate to bump
> patches in a commitfest just because no-one's gotten around to reviewing
> them, because the point of commitfests is precisely to provide a checkpoint
> where every submitted patch gets at least some feedback. But I've run out of
> steam myself, and I don't see anyone else interested in any of these
> patches, so I don't think there's much else we can do :-(.

OK let's wait a bit then for those ones. There is still a bit of time.
-- 
Michael

On Tue, Aug 25, 2015 at 6:05 PM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>
>> -- merging pgbench logs: returned with feedback or bump? Fabien has
>> concerns about performance regarding fprintf when merging the logs.
>> Fabien, Tomas, thoughts?
>> -- pgbench - per-transaction and aggregated logs: returned with
>> feedback or bump to next CF? Fabien, Tomas, thoughts?
>
>
> I think that both features are worthwhile so next CF would be better, but it
> really depends on Tomas.

OK, so let's wait for input from Tomas for now.

> The key issue was the implementation complexity and maintenance burden which
> was essentially driven by fork-based thread emulation compatibility, but it
> has gone away as the emulation has been taken out of pgbench and it is now
> possible to do a much simpler implementation of these features.
>
> The performance issue is that if you have many threads which perform
> monstruous tps and try to log them then logging becomes a bottle neck, both
> the "printf" time and the eventual file locking... Well, that is life, it is
> well know that experimentators influence experiments they are looking at
> since Schrödinger, and moreover the --sampling-rate option is already here
> to alleviate this problem if needed, so I do not think that it is an issue
> to address by keeping the code complex.

Honestly, I don't like the idea of having a bottleneck at logging
level even if we can leverage it with a logging sample rate, that's a
recipe for making pgbench become a benchmark to measure its own
contention, while it should put the backend into pressure,
particularly when short transactions are used.
--
Michael

Hi,

On 08/25/2015 02:44 PM, Michael Paquier wrote:
> On Tue, Aug 25, 2015 at 6:05 PM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>>
>>> -- merging pgbench logs: returned with feedback or bump? Fabien has
>>> concerns about performance regarding fprintf when merging the logs.
>>> Fabien, Tomas, thoughts?
>>> -- pgbench - per-transaction and aggregated logs: returned with
>>> feedback or bump to next CF? Fabien, Tomas, thoughts?
>>
>>
>> I think that both features are worthwhile so next CF would be
>> better,but it really depends on Tomas.
>
> OK, so let's wait for input from Tomas for now.

Let's move them to the next CF.

>
>> The key issue was the implementation complexity and maintenance burden which
>> was essentially driven by fork-based thread emulation compatibility, but it
>> has gone away as the emulation has been taken out of pgbench and it is now
>> possible to do a much simpler implementation of these features.

To some extent, yes. It makes logging into a single file simpler, but 
the overhead it introduces is still an open question and it does not 
really simplify the other patch (writing both raw and aggregated logs).

>>
>> The performance issue is that if you have many threads which perform
>> monstruous tps and try to log them then logging becomes a bottle neck, both
>> the "printf" time and the eventual file locking... Well, that is life, it is
>> well know that experimentators influence experiments they are looking at
>> since Schrödinger, and moreover the --sampling-rate option is already here
>> to alleviate this problem if needed, so I do not think that it is an issue
>> to address by keeping the code complex.
>
> Honestly, I don't like the idea of having a bottleneck at logging
> level even if we can leverage it with a logging sample rate, that's a
> recipe for making pgbench become a benchmark to measure its own
> contention, while it should put the backend into pressure,
> particularly when short transactions are used.

I'd like to point out this overhead would not be a new thing - the 
locking is already there (at least with glibc) to a large degree. See:

http://www.gnu.org/software/libc/manual/html_node/Streams-and-Threads.html

So fprintf does locking, and that has overhead even when the lock is 
uncontended (e.g. when using one file per thread). And it has nothing to 
do with the thread emulation - that was mostly about code complexity, 
not about locking overhead.

The logging system was designed with a single log in mind, so it's not 
quite compatible with features like this - I think we may need to 
redesign it, and I think it nicely matches the producer/consumer 
pattern, about like this:

1) each thread (-j) is a producer
   - producing transaction details (un-formatted)   - possibly batches the data to minimize overhead

2) each log type is a separate consumer
   - may be a dedicated thread or just a function   - gets the raw transaction details (in batches)   - either just
writesthe data to a file (raw), aggregates them or     does something else about that (e.g prints progress)
 

Data is passed through queues (hopefully with low overhead thanks to the 
batching).

regards

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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 08/25/2015 12:39 AM, Michael Paquier wrote:
> -- self-defined policy for sepgsql regression test, returned with 
> feedback? The regressions are broken as mentioned at the end of
> the thread.

I am in the process of getting a VM setup with an appropriate SELinux
setup to look at this one. Will hopefully be able to provide more
feedback in a day or two.

Joe

- -- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJV3HdkAAoJEDfy90M199hlMggQAKfKGJrKzSkdEACoMy83KPAY
UBLPtoFr7ay6c8eS13ahlW6xehIeQoSp1COftYOIO0qFo4JN3+NOJCVGvmS+ooG+
HbGkP919urG10QB3REWp4J6gdnKjIhuAq2cQZQRFBSVPVWqKTBDGUL6RRWDkRwtq
ytWqOLnXb6FTidBihi1hBmV12z9z1FDujMntBMyzIrGpcCcnQ0KDeDXM7kkQeOU7
dMs2m1b2PijToI9UEtWI5lBldhFH/UUSVtBAKQ0BQtAJaLgs2BknIEuj4zwswzEx
IUG7Q+zD4RijkYePVw/gqeHW24g5rSvAC6lmvSpF5L7Cnr2bP1ZXedcInWcRGafT
8IhW1Sggmjd9ZP3EOO/9cEKG50cZpqbdDWdUZciAG0MTbAvOByMyMP49ckyO49T9
XLyUj8lCL1Hoocl203DAJouUr24HOOxd2IcfqYRa64B0jlWUwqi9j1E+DofBMJc+
wUOYrpsQ0/r0YOxNQppfiHX0NxIkp2RCI2dmmNHt2JjlqNME4YH1o2RHby7OwiBb
7kE5WUz7MTJrN7vg9Ua0DvJEBJ1YihOl7mjnlXgJ6ZoXih/l2Jq7LrrlohQlfHeZ
lpSqeVUTu7HkBm0zicwgGna9dzeXpnUrTrlvFH9Wsr7//9kmrfHi8Kt8lASKeQ4d
7YwaThj0oAp4sVsFW39Z
=WGIF
-----END PGP SIGNATURE-----

<div dir="ltr"><div class="gmail_extra"><br />On Tue, Aug 25, 2015 at 4:39 AM, Michael Paquier <<a
href="mailto:michael.paquier@gmail.com">michael.paquier@gmail.com</a>>wrote:<br />> <br />> [...]<br />>
<br/>> -- Support to COMMENT ON CURRENT DATABASE: returned with feedback? The<br />> author mentioned that patch
willbe reworked but there has been no new<br />> version around it seems.<br />><br /><br /></div><div
class="gmail_extra">Movedto the next commitfest.<br /></div><div class="gmail_extra"><br /></div><div
class="gmail_extra">Regards,<br/><br /></div><div class="gmail_extra">--<br />Fabrízio de Royes Mello<br
/>Consultoria/CoachingPostgreSQL<br />>> Timbira: <a
href="http://www.timbira.com.br">http://www.timbira.com.br</a><br/>>> Blog: <a
href="http://fabriziomello.github.io">http://fabriziomello.github.io</a><br/>>> Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/>>> Twitter: <a
href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a><br/>>> Github: <a
href="http://github.com/fabriziomello">http://github.com/fabriziomello</a></div></div>

Michael,

* Michael Paquier (michael.paquier@gmail.com) wrote:
> -- Default Roles: Stephen, are you planning to work on that for next CF?

Yup!
Thanks!
    Stephen

On 08/25/2015 10:39 AM, Michael Paquier wrote:
> On Mon, Aug 10, 2015 at 4:34 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> Hello Hackers,
>>
>> There are a few "Needs Review" items remaining in the July commitfest.
>> Reviewers, please take action - you are holding up the commitfest.
>>
>> In addition to these items, there are a bunch of items in "Ready for
>> Committer" state. Committers: please help with those.
>
> At this stage, there are currently 26 patches in need of actions for
> the current CF:

Thanks Michael!

> - 12 patches are waiting on author:

These can all be marked as Returned with good conscience, they've gotten 
at least some feedback.

> - 8 patches are in need of review:
> -- extends pgbench expressions with functions: bump to next CF, v9 has
> not been reviewed.
> -- check existency of table for -t option (pg_dump) when pattern has
> no wildcard: Bump to next CF?
> -- self-defined policy for sepgsql regression test, returned with
> feedback? The regressions are broken as mentioned at the end of the
> thread.
> -- Unique Joins: bump to next CF?
> -- improving join estimates using FK: bump to next CF?
> -- checkpoint continuous flushing, bump, work, performance tests and
> review are all still going on.
> -- Join pushdown support for foreign tables: err... This patch had no
> activity for 4 months.
> -- Reload SSL certificates on SIGHUP: returned with feedback? I think
> that this patch needs more work to be in a commitable state.
>
> - 6 patches marked as ready for committer:
> -- numeric timestamp in log_line_prefix
> -- plpgsql raise statement with context
> -- postgres_fdw: Options to set fetch_size at the server and table level.
> -- Improving test coverage of extensions with pg_dump
> -- New functions in sslinfo module
> -- CREATE EXTENSION ... CASCADE
>
> For all of them, bump to next CF with the same status if they are not
> committed at the end of the month?

Yeah, I don't think we can let this linger much longer. I hate to bump 
patches in a commitfest just because no-one's gotten around to reviewing 
them, because the point of commitfests is precisely to provide a 
checkpoint where every submitted patch gets at least some feedback. But 
I've run out of steam myself, and I don't see anyone else interested in 
any of these patches, so I don't think there's much else we can do :-(.

- Heikki

On Tue, Aug 25, 2015 at 11:28 PM, Stephen Frost <sfrost@snowman.net> wrote:
> Michael,
>
> * Michael Paquier (michael.paquier@gmail.com) wrote:
>> -- Default Roles: Stephen, are you planning to work on that for next CF?
>
> Yup!

OK. Fine for me. I have moved the patch to next CF, even if I
mentioned having marked it as returned with feedback on the related
thread. I was too hasty here.
-- 
Michael

On 08/25/2015 09:39 AM, Michael Paquier wrote:
> -- Reload SSL certificates on SIGHUP: returned with feedback? I think
> that this patch needs more work to be in a commitable state.

Maybe I am being dense here, but I do not feel like I have gotten any 
clear feedback which gives me a way forward with the patch. I do not 
really see what more I can do here other than resubmit it to the next CF 
which I feel would be poor etiquette by me.

If you have the time I to spare I would be very grateful if you could 
clarify what work you think needs to be done.

-- 
Andreas Karlsson

On Wed, Aug 26, 2015 at 10:30 AM, Andreas Karlsson <andreas@proxel.se> wrote:
> On 08/25/2015 09:39 AM, Michael Paquier wrote:
>>
>> -- Reload SSL certificates on SIGHUP: returned with feedback? I think
>> that this patch needs more work to be in a commitable state.
>
>
> Maybe I am being dense here, but I do not feel like I have gotten any clear
> feedback which gives me a way forward with the patch. I do not really see
> what more I can do here other than resubmit it to the next CF which I feel
> would be poor etiquette by me.
> If you have the time I to spare I would be very grateful if you could
> clarify what work you think needs to be done.

No problem. I have moved it to the next CF then.
-- 
Michael

On Tue, Aug 25, 2015 at 4:39 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Mon, Aug 10, 2015 at 4:34 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> Hello Hackers,
>>
>> There are a few "Needs Review" items remaining in the July commitfest.
>> Reviewers, please take action - you are holding up the commitfest.
>>
>> In addition to these items, there are a bunch of items in "Ready for
>> Committer" state. Committers: please help with those.
>
> At this stage, there are currently 26 patches in need of actions for
> the current CF:

And now we are down to 2, the following ones:
-- Backpatch Resource Owner reassign locks cache. Still not sure if
Andres is planning a backpatch to 9.2.
-- self-defined policy for sepgsql regression test. Joe is wrapping up that.
The rest has been updated in the CF app. If you have any complaints or
remarks, feel free to do so on the thread of the related patch or here
that's fine.
Regards,
-- 
Michael

On Mon, Aug 31, 2015 at 9:13 PM, Magnus Hagander wrote:
> Anyway - that CF is closed, but we seem to not have *any* CF open at this
> point. Should we not make 2015-11 open?

Yeah, switched.
-- 
Michael

On 2015-08-31 16:22:54 +0300, Alexander Korotkov wrote:
> Is it correct to switch 2015-09 commitfest to inprogress now?

Yea, isn't it only starting the 15th? Can we add an option to display
days in the CF app?

Greetings,

Andres Freund

On 08/31/2015 03:34 PM, Alexander Korotkov wrote:
> +1 for making these dates more explicit

Yeah, I think being explicit would make life easier for newcomers.

Andreas

On Mon, Aug 31, 2015 at 10:31 PM, Andres Freund <andres@anarazel.de> wrote:
> On 2015-08-31 16:22:54 +0300, Alexander Korotkov wrote:
>> Is it correct to switch 2015-09 commitfest to inprogress now?

No, this was not correct, it is expected to begin tomorrow.

> Yea, isn't it only starting the 15th?

If my memory does not fail me, the next CF is expected to begin on the
1st, not the 15th.
-- 
Michael