Removing faulty hyperLogLog merge function

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] /messages/by-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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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