Re: Last Commitfest patches waiting review

We are down to 13 patch in Needs Review state. Many of these are
stragglers that no-one's really even looked at yet.

If we want to finish the commitfest, we'll soon have to decide what to
do to patches that no-one cares enough about to review them.

pg_receivexlog: addition of --create/--drop to create/drop repslots
---

Magnus has promised to review this, but hasn't had the time for weeks.
Does anyone else want to pick this up? I'm OK to wait for Magnus to
handle this - I expect it to be a quick review and commit - but we
should not hold up the commitfest for this.

Grouping Sets
---

There has been a lot of discussion, but no decisions. See
http://www.postgresql.org/message-id/87vbodhtvb.fsf@news-spur.riddles.org.uk.

Would a committer be interested to take responsibility of this? If not,
this will just linger indefinitely.

Sort support for text with strxfrm() poor man's keys
---

Peter: Are you waiting for Robert to review this? Robert, could you
review the latest patch, please? Peter: Could you try to get rid of the
extra SortSupport object that Robert didn't like?
(http://www.postgresql.org/message-id/CA+TgmobDe+YDFnHTS0GWpT54-er8BPT3vx8rPshD+98CTDo25g@mail.gmail.com).
I think it would speed up the process if you did that, instead of
waiting for Robert to find the time.

Compression of Full Page Writes
---

This has been a ridiculously long thread, with diversions into different
compression and CRC algorithms. Given that, the latest patch is
surprisingly small. I don't think this is going to get any better with
further discussion, and the patch looks OK at a quick glance, so I've
now marked this as "Ready for Committer".

hash join - dynamic bucket count
---

Kevin: Could you review the latest patch, please?

INNER JOIN removals
---

The latest patch is significantly different from what was originally
submitted for the commitfest, so I wouldn't feel bad just bumping this
to the next one. I'll do that unless someone picks this up soon.

Tom: I know you're busy with the more urgent jsonb patch.. Do you think
you would find the time to review this anytime soon? Anyone else?

event triggers: more DROP info
---

Adam: Are you planning to do more review on this? Alvaro: do you feel
comfortable proceeding with the review you got so far?

Selectivity estimation for inet operators
---

I think there's a bug in the estimation of semi-joins, see
http://www.postgresql.org/message-id/5423DC8D.3080206@vmware.com. But I
think we could split this patch into two, and commit the non-join
selectivity estimator first, as that seems OK. If no-one else steps up
to the plate, I can do that..

add latency limit to pgbench throttling (--rate)
---

Rukh: Are you planning to review the latest patch version?

Escaping from blocked send() by pg_terminate_backend().
---

I've had a look, but I'd like to have a second opinion on this.

Fix local_preload_libraries to work as expected.
---

Peter: Could you move this forward, please?

pg_dump refactor to remove global variables
---

Peter: Can you review the latest patch, please?

Fix xpath() to return namespace definitions (fixes the issue with nested
or repeated xpath())
---

No-one's volunteered to review this. I don't understand XML enough to
review this, and Abhijit who was earlier signed up as reviewer said the
same thing.

Peter: Could you take a look at this too? You've dabbled into XML stuff
before..

- Heikki

On 2014-09-27 11:18:26 +0300, Heikki Linnakangas wrote:
> pg_receivexlog: addition of --create/--drop to create/drop repslots
> ---
>
> Magnus has promised to review this, but hasn't had the time for weeks. Does
> anyone else want to pick this up? I'm OK to wait for Magnus to handle this -
> I expect it to be a quick review and commit - but we should not hold up the
> commitfest for this.

I'll take that one, sometime next week, if Magnus hasn't gotten to it by then.

> Compression of Full Page Writes
> ---
>
> This has been a ridiculously long thread, with diversions into different
> compression and CRC algorithms. Given that, the latest patch is surprisingly
> small. I don't think this is going to get any better with further
> discussion, and the patch looks OK at a quick glance, so I've now marked
> this as "Ready for Committer".

Did you like the patch? On a quick pass I wasn't very enthusiastic about
it in its current state.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 09/27/2014 11:31 AM, Andres Freund wrote:
> On 2014-09-27 11:18:26 +0300, Heikki Linnakangas wrote:
>> pg_receivexlog: addition of --create/--drop to create/drop repslots
>> ---
>>
>> Magnus has promised to review this, but hasn't had the time for weeks. Does
>> anyone else want to pick this up? I'm OK to wait for Magnus to handle this -
>> I expect it to be a quick review and commit - but we should not hold up the
>> commitfest for this.
>
> I'll take that one, sometime next week, if Magnus hasn't gotten to it by then.

Thanks!

>> Compression of Full Page Writes
>> ---
>>
>> This has been a ridiculously long thread, with diversions into different
>> compression and CRC algorithms. Given that, the latest patch is surprisingly
>> small. I don't think this is going to get any better with further
>> discussion, and the patch looks OK at a quick glance, so I've now marked
>> this as "Ready for Committer".
>
> Did you like the patch? On a quick pass I wasn't very enthusiastic about
> it in its current state.

Now that I look at it closer, there's some ugly things there like
putting the xl_compress field to XLogRecord. I guess I should post to
the thread with more detailed comments.

- Heikki

Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
> [ unreviewed patches ]

> Grouping Sets

> There has been a lot of discussion, but no decisions. See
> http://www.postgresql.org/message-id/87vbodhtvb.fsf@news-spur.riddles.org.uk.

> Would a committer be interested to take responsibility of this? If not,
> this will just linger indefinitely.

I should and will take this, but not in this commitfest: I've been just
horribly busy lately with both work and personal issues. If we can punt
it to the next fest, I will promise to work on it then.

> INNER JOIN removals

> The latest patch is significantly different from what was originally
> submitted for the commitfest, so I wouldn't feel bad just bumping this
> to the next one. I'll do that unless someone picks this up soon.
> Tom: I know you're busy with the more urgent jsonb patch.. Do you think
> you would find the time to review this anytime soon? Anyone else?

Same deal here.

> Selectivity estimation for inet operators

> I think there's a bug in the estimation of semi-joins, see
> http://www.postgresql.org/message-id/5423DC8D.3080206@vmware.com. But I
> think we could split this patch into two, and commit the non-join
> selectivity estimator first, as that seems OK. If no-one else steps up
> to the plate, I can do that..

And I'd like to look this one over too ...

> Escaping from blocked send() by pg_terminate_backend().

> I've had a look, but I'd like to have a second opinion on this.

I concur with your opinion that this is scary as heck. We need multiple
pairs of eyeballs if we're going to do anything in this area. In the long
run though, I think pushing functionality into signal handlers is exactly
backwards; we ought to be trying to go the other way.

regards, tom lane

On 2014-09-27 10:12:03 -0400, Tom Lane wrote:
> > Escaping from blocked send() by pg_terminate_backend().
>
> > I've had a look, but I'd like to have a second opinion on this.
>
> I concur with your opinion that this is scary as heck. We need multiple
> pairs of eyeballs if we're going to do anything in this area. In the long
> run though, I think pushing functionality into signal handlers is exactly
> backwards; we ought to be trying to go the other way.

I very much agree with that concern. The interactions are just
complicated.

I'm now refreshing my work to do all waiting in client communication via
latches. While far from trivial, I'm pretty sure that's the better
course in the long run.

I guess I'll post it in the other thread.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sat, Sep 27, 2014 at 1:18 AM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> Sort support for text with strxfrm() poor man's keys
> ---
>
> Peter: Are you waiting for Robert to review this? Robert, could you review
> the latest patch, please? Peter: Could you try to get rid of the extra
> SortSupport object that Robert didn't like?
> (http://www.postgresql.org/message-id/CA+TgmobDe+YDFnHTS0GWpT54-er8BPT3vx8rPshD+98CTDo25g@mail.gmail.com).
> I think it would speed up the process if you did that, instead of waiting
> for Robert to find the time.

I am not waiting on Robert to spend the time, FWIW. The question that
resolving if we should not have an extra sortsupport object is
blocking on is the need to have a consistent sorttuple.datum1
representation for the benefit of having comparetup_heap() know that
it's either always abbreviated keys or always pointers to text. My
view is that it's not worth going back to fix up datum1 to always be a
pointer to text when we abort abbreviation - I think we should just
forget about datum1 on the rare occasion that happens (due to the
costs involved, as well as the complexity implied).

I think that it will be necessary for me to rigorously prove that
view, as with the "memcmp() == 0" thing. So I'm looking at that.

--
Peter Geoghegan

On 9/27/14, 4:18 AM, Heikki Linnakangas wrote:
> add latency limit to pgbench throttling (--rate)
> ---
> Rukh: Are you planning to review the latest patch version?

Rukh is free to jump onto this ASAP, but if it's still there next week I
already had my eye on picking that up and taking it out for a spin. I
already updated my pgbench-tools set to incorporate the rate limit for
9.4, and I use that part all the time now. I think I understand
Fabien's entire game plan for this now, and I want the remainder of the
set to land in 9.5.

--
Greg Smith greg(dot)smith(at)crunchydatasolutions(dot)com
Chief PostgreSQL Evangelist - http://crunchydatasolutions.com/

On 09/27/2014 05:12 PM, Tom Lane wrote:
> Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
>> [ unreviewed patches ]
>
>> Grouping Sets
>
>> There has been a lot of discussion, but no decisions. See
>> http://www.postgresql.org/message-id/87vbodhtvb.fsf@news-spur.riddles.org.uk.
>
>> Would a committer be interested to take responsibility of this? If not,
>> this will just linger indefinitely.
>
> I should and will take this, but not in this commitfest: I've been just
> horribly busy lately with both work and personal issues. If we can punt
> it to the next fest, I will promise to work on it then.

Ok, thanks! I moved this to next commitfest and put your name on it as
reviewer.

>> INNER JOIN removals
>
>> The latest patch is significantly different from what was originally
>> submitted for the commitfest, so I wouldn't feel bad just bumping this
>> to the next one. I'll do that unless someone picks this up soon.
>> Tom: I know you're busy with the more urgent jsonb patch.. Do you think
>> you would find the time to review this anytime soon? Anyone else?
>
> Same deal here.

I marked this as "Returned with feedback", as the trigger-queue issue
seems like a show-stopper.

>> Selectivity estimation for inet operators
>
>> I think there's a bug in the estimation of semi-joins, see
>> http://www.postgresql.org/message-id/5423DC8D.3080206@vmware.com. But I
>> think we could split this patch into two, and commit the non-join
>> selectivity estimator first, as that seems OK. If no-one else steps up
>> to the plate, I can do that..
>
> And I'd like to look this one over too ...

Ok, punted this to next commitfest too.

- Heikki

Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> wrote:

> hash join - dynamic bucket count
> ---
>
> Kevin: Could you review the latest patch, please?

Will post a code review later today. Benchmarks might take a bit
longer...

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Thanks to everyone's who's reviewed a patch so far. One last crunch,
and we'll be through.

We have 7 patches left in Needs Review state:

pg_receivexlog: addition of --create/--drop to create/drop repslots

Waiting for Magnus. Amit promised to take a look if Magnus continues
to be busy.

Sort support for text with strxfrm() poor man's keys

Robert? What do you think of Peter's latest patch?

add latency limit to pgbench throttling (--rate)

I'm waiting for Greg Smith to have a look at the latest patch.

Escaping from blocked send() by pg_terminate_backend().

Andres posted a patch series using a completely different approach. I
guess this should be just marked as returned with feedback, if we're
going to pursue the different approach instead.

Fix local_preload_libraries to work as expected.
pg_dump refactor to remove global variables

Peter: Ping? Will you have the time to review these?

Fix xpath() to return namespace definitions (fixes the issue with nested
or repeated xpath())

Peter, you've done some XML stuff before; could you have a look at
this too?

- Heikki

On Fri, Oct 3, 2014 at 6:14 PM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> Thanks to everyone's who's reviewed a patch so far. One last crunch, and
> we'll be through.
>
> We have 7 patches left in Needs Review state:
>
> pg_receivexlog: addition of --create/--drop to create/drop repslots
>
> Waiting for Magnus. Amit promised to take a look if Magnus continues
> to be busy.

Andres did, not Amit.

And thanks Andres! :) And sorry aobut that one - no way I'll get to it
until at the earliest next week but almost certainly not until the
week after :( So I really appreciate the pickup.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On 2014-10-03 19:14:14 +0300, Heikki Linnakangas wrote:
> Thanks to everyone's who's reviewed a patch so far. One last crunch, and
> we'll be through.
>
> We have 7 patches left in Needs Review state:
>
> pg_receivexlog: addition of --create/--drop to create/drop repslots
>
> Waiting for Magnus. Amit promised to take a look if Magnus continues
> to be busy.

I've committed half of it, and will commit the second half soon. I guess
you meant Andres instead of Amit?

> Escaping from blocked send() by pg_terminate_backend().
>
> Andres posted a patch series using a completely different approach. I
> guess this should be just marked as returned with feedback, if we're
> going to pursue the different approach instead.

Sounds fair to me. I'll comment with details on the corresponding thread.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Oct 3, 2014 at 12:14 PM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> Sort support for text with strxfrm() poor man's keys
>
> Robert? What do you think of Peter's latest patch?

I haven't had time to look at it yet. Can we move it to the next
CommitFest? I spent a lot of time on this one, but I can't keep doing
that forever, because, you know, other work.

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

On Mon, Oct 6, 2014 at 7:57 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I haven't had time to look at it yet. Can we move it to the next
> CommitFest? I spent a lot of time on this one, but I can't keep doing
> that forever, because, you know, other work.

Are you suggesting that it would be useful to have input from another
person? Because if you are, then I agree that ideally that would be
possible.

--
Peter Geoghegan

On Mon, Oct 6, 2014 at 1:58 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Mon, Oct 6, 2014 at 7:57 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> I haven't had time to look at it yet. Can we move it to the next
>> CommitFest? I spent a lot of time on this one, but I can't keep doing
>> that forever, because, you know, other work.
>
> Are you suggesting that it would be useful to have input from another
> person? Because if you are, then I agree that ideally that would be
> possible.

Well, really, I was just suggesting that I can spend more time on the
patch, but not immediately.

But if someone else can spend time on the patch, that's good too.

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

On Mon, Oct 6, 2014 at 11:27 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Well, really, I was just suggesting that I can spend more time on the
> patch, but not immediately.

We haven't really talked about the idea of the HyperLogLog-based abort
mechanism - the actual cost model - even though I thought we'd have
discussed that extensively by now. Maybe I'm reading too much into
that, but my guess is that that's because it's a particularly hard
thing to have an opinion on. I think that It might not be a bad idea
to have another opinion on that in particular.

We've historically resisted this kind of special case optimization,
and yet the optimization is likely to be very effective in many
representative cases. Plus, some aspects of the cost model are a bit
fuzzy, in a way that things in the executor ordinarily are not, and so
I can understand how this would present any reviewer with difficulty.

By the way, the original description of this approach to accelerating
sorts I saw was here, in this 2001 paper:

http://lgis.informatik.uni-kl.de/archiv/wwwdvs.informatik.uni-kl.de/courses/DBSREAL/SS2001/Vorlesungsunterlagen/Implementing_Sorting.pdf

(Under "2.4 Cache-optimized techniques")
--
Peter Geoghegan

2014-10-03 23:14 GMT+07:00 Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>:

> Fix xpath() to return namespace definitions (fixes the issue with nested
> or repeated xpath())
>
> Peter, you've done some XML stuff before; could you have a look at this
> too?
>

I am the author of the patch. I've sent Peter off-the-list review request
email as you had suggested before, but he didn't respond.

How can i help to ease the review? The last patch is re-implementation, as
per Tom Lane's findings about xmlNodeCopy's behavior if there's not enough
memory. It turns out that the reimplementation is not as simple as before
(because reimplement some of xmlNodeCopy code must be reimplemented here).

Reviewing the patch myself, i've found some code formatting problems. Will
fix and post in the patch's thread.

Regards,
--
Ali Akbar

On Mon, Oct 6, 2014 at 11:53 AM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Mon, Oct 6, 2014 at 11:27 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> Well, really, I was just suggesting that I can spend more time on the
>> patch, but not immediately.
>
> We haven't really talked about the idea of the HyperLogLog-based abort
> mechanism - the actual cost model - even though I thought we'd have
> discussed that extensively by now.

My concern is that if we only get it committed in the last commitfest,
we may run out of time to make sortsupport work for B-Tree index
builds. That's where the sortsupport for text stuff will be really
useful.

--
Peter Geoghegan

On 10/09/2014 09:59 PM, Peter Geoghegan wrote:
> My concern is that if we only get it committed in the last commitfest,
> we may run out of time to make sortsupport work for B-Tree index
> builds. That's where the sortsupport for text stuff will be really
> useful.

B-tree index build uses tuplesort.c. What's missing?

- Heikki

On Thu, Oct 9, 2014 at 12:14 PM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> B-tree index build uses tuplesort.c. What's missing?

I don't think that all that much is missing. Tuplesort expects to work
with an index scankey when sorting B-Tree tuples. There needs to be
something like a reverse lookup of the sortsupport function. It looks
like a historical oversight, that would take time to fix, but wouldn't
be particularly challenging. You'd need to pick out the operators from
the scankey, so you'd have something like what tuplesort_begin_heap()
starts off with with tuplesort_begin_index_btree().

copytup_index() would then later need to be modifed to make
abbreviation occur there too, but that's no big deal.

--
Peter Geoghegan

On 10/09/2014 10:25 PM, Peter Geoghegan wrote:
> On Thu, Oct 9, 2014 at 12:14 PM, Heikki Linnakangas
> <hlinnakangas(at)vmware(dot)com> wrote:
>> B-tree index build uses tuplesort.c. What's missing?
>
> I don't think that all that much is missing. Tuplesort expects to work
> with an index scankey when sorting B-Tree tuples. There needs to be
> something like a reverse lookup of the sortsupport function. It looks
> like a historical oversight, that would take time to fix, but wouldn't
> be particularly challenging. You'd need to pick out the operators from
> the scankey, so you'd have something like what tuplesort_begin_heap()
> starts off with with tuplesort_begin_index_btree().

Oh, I didn't realize we don't do that already! I'm surprised, I would've
expected index build to have been the first thing we'd use the
SortSupport stuff in.

Yeah, that seems worth doing, independently of the this patch. Can you
write a separate patch to use SortSupport for B-tree index builds,
please? Eliminating the FunctionCallInfoData overhead should shave off
some some cycles from every index build.

- Heikki

On Thu, Oct 9, 2014 at 12:51 PM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> Oh, I didn't realize we don't do that already! I'm surprised, I would've
> expected index build to have been the first thing we'd use the SortSupport
> stuff in.

The thing is that the most compelling numbers for sortsupport (plus
the related improvements to tuplesort itself) were from the "onlyKey"
optimization - the numbers are only so-so when you look at
multi-attribute sorts, because we specialize qsort() for both of those
two cases (i.e. one specialization, qsort_ssup(), only looks at
datum1, while qsort_tuple() looks at everything else, through which
comparetup_heap() and comparetup_datum() are called). But with the
B-Tree comparator, we're only ever going to be able to use
qsort_tuple() which is roughly equivalent to the so-so multi-attribute
case for heap tuples (because we need to detect duplicate violations,
and a few other things - no choice there).

It kind of makes sense that we didn't push ourselves to get B-Tree
support until now. But with sortsupport for text accelerating sorts by
perhaps as much as 10 times in sympathetic (though realistic) cases,
it would be crazy to have that without B-Tree support (abbreviated
keys always force us to use the qsort_tuple() specialization, because
the comparator tie-breaker logic must live in places like
comparetup_heap()).

> Yeah, that seems worth doing, independently of the this patch.

As I mentioned, that might be less true than you'd think.

> Can you write
> a separate patch to use SortSupport for B-tree index builds, please?
> Eliminating the FunctionCallInfoData overhead should shave off some some
> cycles from every index build.

I'll look into it. Hopefully an effort to actually implement it will
show that I was right, and there isn't much to it.

--
Peter Geoghegan

On Thu, Oct 9, 2014 at 1:13 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
>> Can you write
>> a separate patch to use SortSupport for B-tree index builds, please?
>> Eliminating the FunctionCallInfoData overhead should shave off some some
>> cycles from every index build.
>
> I'll look into it. Hopefully an effort to actually implement it will
> show that I was right, and there isn't much to it.

I was able to get this working pretty quickly. All regression tests
pass. I'm not going to post a patch just yet, because I still need to
make the cluster case work, and I'll probably want to refactor my
approach to performing catalog lookups a bit, but the important point
is that my original suspicion that this isn't very difficult or
invasive has been confirmed.

I should have something to post before too long.

--
Peter Geoghegan