Thread: Removing faulty hyperLogLog merge function
The function hyperLogLogMerge() is faulty [1]. It has no current callers, though. I propose that we rip it out, as in the attached patch. [1] http://www.postgresql.org/message-id/CAM3SWZT-i6R9JU5YXa8MJUou2_r3LfGJZpQ9tYa1BYxfkj0=cQ@mail.gmail.com -- Peter Geoghegan
Attachment
On Mon, Apr 25, 2016 at 10:39 PM, Peter Geoghegan <pg@heroku.com> wrote: > The function hyperLogLogMerge() is faulty [1]. It has no current > callers, though. I propose that we rip it out, as in the attached > patch. > > [1] http://www.postgresql.org/message-id/CAM3SWZT-i6R9JU5YXa8MJUou2_r3LfGJZpQ9tYa1BYxfkj0=cQ@mail.gmail.com I'm not prepared to commit this over the objection offered by Tomas Vondra on that thread. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Apr 26, 2016 at 4:41 PM, Robert Haas <robertmhaas@gmail.com> wrote: > I'm not prepared to commit this over the objection offered by Tomas > Vondra on that thread. You don't want to remove buggy code that is currently unused simply because it might be useful to have that functionality in the future? -- Peter Geoghegan
On Tue, Apr 26, 2016 at 7:46 PM, Peter Geoghegan <pg@heroku.com> wrote: > On Tue, Apr 26, 2016 at 4:41 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> I'm not prepared to commit this over the objection offered by Tomas >> Vondra on that thread. > > You don't want to remove buggy code that is currently unused simply > because it might be useful to have that functionality in the future? No, I don't want to remove code that somebody thinks we should fix instead of removing on your say-so. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Apr 26, 2016 at 4:47 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> You don't want to remove buggy code that is currently unused simply >> because it might be useful to have that functionality in the future? > > No, I don't want to remove code that somebody thinks we should fix > instead of removing on your say-so. I don't think that that's an efficient use of anyone's time. If somebody proposes a patch with functionality that needs to merge HLL states, then they can add an implementation at that time. -- Peter Geoghegan
On Tue, Apr 26, 2016 at 7:54 PM, Peter Geoghegan <pg@heroku.com> wrote: > On Tue, Apr 26, 2016 at 4:47 PM, Robert Haas <robertmhaas@gmail.com> wrote: >>> You don't want to remove buggy code that is currently unused simply >>> because it might be useful to have that functionality in the future? >> >> No, I don't want to remove code that somebody thinks we should fix >> instead of removing on your say-so. > > I don't think that that's an efficient use of anyone's time. If > somebody proposes a patch with functionality that needs to merge HLL > states, then they can add an implementation at that time. Peter, if there is consensus on this patch, I will commit it. If there isn't, I won't. If you don't like that, sorry. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> I'm not prepared to commit this over the objection offered by Tomas
> Vondra on that thread.
FWIW, I agree with Peter that we should remove this code. We know that it
is buggy. Leaving it there constitutes an "attractive nuisance" --- that
is, I'm afraid that someone will submit a patch that depends on that
function, and that we might forget that the function is broken and commit
said patch.
Tomas' objection would be reasonable if a fix was simple, but so far as
I can tell from the thread, it's not. In particular, Peter doesn't trust
the upstream patch in question. But whether or not you trust it, doing
nothing is not a sane choice. The reasonable alternatives are to remove
the merge function or sync the upstream patch.
regards, tom lane
On Tue, Apr 26, 2016 at 9:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> I'm not prepared to commit this over the objection offered by Tomas >> Vondra on that thread. > > FWIW, I agree with Peter that we should remove this code. We know that it > is buggy. Leaving it there constitutes an "attractive nuisance" --- that > is, I'm afraid that someone will submit a patch that depends on that > function, and that we might forget that the function is broken and commit > said patch. > > Tomas' objection would be reasonable if a fix was simple, but so far as > I can tell from the thread, it's not. In particular, Peter doesn't trust > the upstream patch in question. But whether or not you trust it, doing > nothing is not a sane choice. The reasonable alternatives are to remove > the merge function or sync the upstream patch. Now I agree with that. And now we do not have a 1-1 tie on which alternative to prefer, which is a good start towards a consensus. Any other views? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 04/26/2016 07:23 PM, Robert Haas wrote: > On Tue, Apr 26, 2016 at 9:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Robert Haas <robertmhaas@gmail.com> writes: >>> I'm not prepared to commit this over the objection offered by Tomas >>> Vondra on that thread. >> >> FWIW, I agree with Peter that we should remove this code. We know that it >> is buggy. Leaving it there constitutes an "attractive nuisance" --- that >> is, I'm afraid that someone will submit a patch that depends on that >> function, and that we might forget that the function is broken and commit >> said patch. >> >> Tomas' objection would be reasonable if a fix was simple, but so far as >> I can tell from the thread, it's not. In particular, Peter doesn't trust >> the upstream patch in question. But whether or not you trust it, doing >> nothing is not a sane choice. The reasonable alternatives are to remove >> the merge function or sync the upstream patch. > > Now I agree with that. And now we do not have a 1-1 tie on which > alternative to prefer, which is a good start towards a consensus. Any > other views? I haven't followed this issue all that closely, but to me it seems pretty clear. If the function is brand new to 9.6, buggy, and not even used anywhere, I cannot imagine why we would leave it in the tree. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
On Wed, Apr 27, 2016 at 12:08 PM, Joe Conway <mail@joeconway.com> wrote: > On 04/26/2016 07:23 PM, Robert Haas wrote: >> On Tue, Apr 26, 2016 at 9:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Robert Haas <robertmhaas@gmail.com> writes: >>>> I'm not prepared to commit this over the objection offered by Tomas >>>> Vondra on that thread. >>> >>> FWIW, I agree with Peter that we should remove this code. We know that it >>> is buggy. Leaving it there constitutes an "attractive nuisance" --- that >>> is, I'm afraid that someone will submit a patch that depends on that >>> function, and that we might forget that the function is broken and commit >>> said patch. >>> >>> Tomas' objection would be reasonable if a fix was simple, but so far as >>> I can tell from the thread, it's not. In particular, Peter doesn't trust >>> the upstream patch in question. But whether or not you trust it, doing >>> nothing is not a sane choice. The reasonable alternatives are to remove >>> the merge function or sync the upstream patch. >> >> Now I agree with that. And now we do not have a 1-1 tie on which >> alternative to prefer, which is a good start towards a consensus. Any >> other views? > > I haven't followed this issue all that closely, but to me it seems > pretty clear. If the function is brand new to 9.6, buggy, and not even > used anywhere, I cannot imagine why we would leave it in the tree. +1. We should definitely not encourage its use for 3rd-part plugins. -- Michael
On Wed, Apr 27, 2016 at 1:37 AM, Michael Paquier <michael.paquier@gmail.com> wrote: >> I haven't followed this issue all that closely, but to me it seems >> pretty clear. If the function is brand new to 9.6, buggy, and not even >> used anywhere, I cannot imagine why we would leave it in the tree. > > +1. We should definitely not encourage its use for 3rd-part plugins. So, now there's clearly a consensus. Committed. :-) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company