Re: bug in citext's upgrade script for parallel aggregates

Seems there's a small error in the upgrade script for citext for 1.1
to 1.2 which will cause min(citext) not to be parallel enabled.

max(citext)'s combinefunc is first set incorrectly, but then updated
to the correct value. I assume it was meant to set the combine
function for min(citext) instead.

Fix attached. I've assumed that because we're still in beta that we
can get away with this fix rather than making a 1.3 version to fix the
issue.

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

On 06/24/2016 01:31 PM, David Rowley wrote:
> Seems there's a small error in the upgrade script for citext for 1.1
> to 1.2 which will cause min(citext) not to be parallel enabled.
>
> max(citext)'s combinefunc is first set incorrectly, but then updated
> to the correct value. I assume it was meant to set the combine
> function for min(citext) instead.
>
> Fix attached. I've assumed that because we're still in beta that we
> can get away with this fix rather than making a 1.3 version to fix the
> issue.

Yes, this is indeed a bug.

Andreas

On Sat, Jun 25, 2016 at 3:44 AM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> On 06/24/2016 01:31 PM, David Rowley wrote:
>> Seems there's a small error in the upgrade script for citext for 1.1
>> to 1.2 which will cause min(citext) not to be parallel enabled.
>>
>> max(citext)'s combinefunc is first set incorrectly, but then updated
>> to the correct value. I assume it was meant to set the combine
>> function for min(citext) instead.
>>
>> Fix attached. I've assumed that because we're still in beta that we
>> can get away with this fix rather than making a 1.3 version to fix the
>> issue.
>
> Yes, this is indeed a bug.

Since we've already released beta2, I think we need to do a whole new
extension version. We treated beta1 as a sufficiently-significant
event to mandate a version bump, so we should do the same here.

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

On 30 June 2016 at 03:49, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Sat, Jun 25, 2016 at 3:44 AM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
>> On 06/24/2016 01:31 PM, David Rowley wrote:
>>> Seems there's a small error in the upgrade script for citext for 1.1
>>> to 1.2 which will cause min(citext) not to be parallel enabled.
>>>
>>> max(citext)'s combinefunc is first set incorrectly, but then updated
>>> to the correct value. I assume it was meant to set the combine
>>> function for min(citext) instead.
>>>
>>> Fix attached. I've assumed that because we're still in beta that we
>>> can get away with this fix rather than making a 1.3 version to fix the
>>> issue.
>>
>> Yes, this is indeed a bug.
>
> Since we've already released beta2, I think we need to do a whole new
> extension version. We treated beta1 as a sufficiently-significant
> event to mandate a version bump, so we should do the same here.

Ok, good point. Patch attached.

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

On 07/09/2016 05:42 AM, David Rowley wrote:
> On 30 June 2016 at 03:49, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Sat, Jun 25, 2016 at 3:44 AM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
>>> On 06/24/2016 01:31 PM, David Rowley wrote:
>>>> Seems there's a small error in the upgrade script for citext for 1.1
>>>> to 1.2 which will cause min(citext) not to be parallel enabled.
>>>>
>>>> max(citext)'s combinefunc is first set incorrectly, but then updated
>>>> to the correct value. I assume it was meant to set the combine
>>>> function for min(citext) instead.
>>>>
>>>> Fix attached. I've assumed that because we're still in beta that we
>>>> can get away with this fix rather than making a 1.3 version to fix the
>>>> issue.
>>>
>>> Yes, this is indeed a bug.
>>
>> Since we've already released beta2, I think we need to do a whole new
>> extension version. We treated beta1 as a sufficiently-significant
>> event to mandate a version bump, so we should do the same here.
>
> Ok, good point. Patch attached.

Thanks!

I tested the patch and it looks good.

Andreas

On 14 July 2016 at 02:00, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> On 07/09/2016 05:42 AM, David Rowley wrote:
>> On 30 June 2016 at 03:49, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> Since we've already released beta2, I think we need to do a whole new
>>> extension version. We treated beta1 as a sufficiently-significant
>>> event to mandate a version bump, so we should do the same here.
>>
>>
>> Ok, good point. Patch attached.
>
>
> Thanks!
>
> I tested the patch and it looks good.

Thanks for looking.

I've added this to the open items list so that it does not get forgotten about.

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

On Thu, Jul 14, 2016 at 02:00:59AM +0200, Andreas Karlsson wrote:
> On 07/09/2016 05:42 AM, David Rowley wrote:
> >On 30 June 2016 at 03:49, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> >>On Sat, Jun 25, 2016 at 3:44 AM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> >>>On 06/24/2016 01:31 PM, David Rowley wrote:
> >>>>Seems there's a small error in the upgrade script for citext for 1.1
> >>>>to 1.2 which will cause min(citext) not to be parallel enabled.
> >>>>
> >>>>max(citext)'s combinefunc is first set incorrectly, but then updated
> >>>>to the correct value. I assume it was meant to set the combine
> >>>>function for min(citext) instead.
> >>>>
> >>>>Fix attached. I've assumed that because we're still in beta that we
> >>>>can get away with this fix rather than making a 1.3 version to fix the
> >>>>issue.
> >>>
> >>>Yes, this is indeed a bug.
> >>
> >>Since we've already released beta2, I think we need to do a whole new
> >>extension version. We treated beta1 as a sufficiently-significant
> >>event to mandate a version bump, so we should do the same here.
> >
> >Ok, good point. Patch attached.
>
> Thanks!
>
> I tested the patch and it looks good.

[Action required within 72 hours. This is a generic notification.]

The above-described topic is currently a PostgreSQL 9.6 open item. Robert,
since you committed the patch believed to have created it, you own this open
item. If some other commit is more relevant or if this does not belong as a
9.6 open item, please let us know. Otherwise, please observe the policy on
open item ownership[1] and send a status update within 72 hours of this
message. Include a date for your subsequent status update. Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping 9.6rc1. Consequently, I will appreciate your
efforts toward speedy resolution. Thanks.

[1] http://www.postgresql.org/message-id/20160527025039.GA447393@tornado.leadboat.com

On Tue, Jul 26, 2016 at 3:48 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Thu, Jul 14, 2016 at 02:00:59AM +0200, Andreas Karlsson wrote:
>> On 07/09/2016 05:42 AM, David Rowley wrote:
>> >On 30 June 2016 at 03:49, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> >>On Sat, Jun 25, 2016 at 3:44 AM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
>> >>>On 06/24/2016 01:31 PM, David Rowley wrote:
>> >>>>Seems there's a small error in the upgrade script for citext for 1.1
>> >>>>to 1.2 which will cause min(citext) not to be parallel enabled.
>> >>>>
>> >>>>max(citext)'s combinefunc is first set incorrectly, but then updated
>> >>>>to the correct value. I assume it was meant to set the combine
>> >>>>function for min(citext) instead.
>> >>>>
>> >>>>Fix attached. I've assumed that because we're still in beta that we
>> >>>>can get away with this fix rather than making a 1.3 version to fix the
>> >>>>issue.
>> >>>
>> >>>Yes, this is indeed a bug.
>> >>
>> >>Since we've already released beta2, I think we need to do a whole new
>> >>extension version. We treated beta1 as a sufficiently-significant
>> >>event to mandate a version bump, so we should do the same here.
>> >
>> >Ok, good point. Patch attached.
>>
>> Thanks!
>>
>> I tested the patch and it looks good.
>
> [Action required within 72 hours. This is a generic notification.]
>
> The above-described topic is currently a PostgreSQL 9.6 open item. Robert,
> since you committed the patch believed to have created it, you own this open
> item. If some other commit is more relevant or if this does not belong as a
> 9.6 open item, please let us know. Otherwise, please observe the policy on
> open item ownership[1] and send a status update within 72 hours of this
> message. Include a date for your subsequent status update. Testers may
> discover new open items at any time, and I want to plan to get them all fixed
> well in advance of shipping 9.6rc1. Consequently, I will appreciate your
> efforts toward speedy resolution. Thanks.

Committed the patch.

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