Re: [PATCH] remove deprecated v8.2 containment operators

[Tom Lane]

Justin Pryzby  writes:
> On Thu, Mar 04, 2021 at 08:58:39PM -0500, Tom Lane wrote:
>> I'm confused by why this patch is only dropping the operators'
>> opclass-membership links.  Don't we want to actually DROP OPERATOR
>> too?

> Okay

Pushed.  Since hstore already had a new-in-v14 edition, I just added
the commands to hstore--1.7--1.8.sql rather than make another update
script.  (Also, you forgot to drop ~ in that one?)

> Also , I think it's unrelated to this patch, but shouldn't these be removed ?

Right, done.

regards, tom lane

Re: [PATCH] remove deprecated v8.2 containment operators

[Justin Pryzby]

On Thu, Mar 04, 2021 at 08:58:39PM -0500, Tom Lane wrote:
> Justin Pryzby  writes:
> > [ 0001-remove-deprecated-v8.2-containment-operators.patch ]
> 
> I'm confused by why this patch is only dropping the operators'
> opclass-membership links.  Don't we want to actually DROP OPERATOR
> too?

Okay

Also , I think it's unrelated to this patch, but shouldn't these be removed ?
See: b0b7be613, c15898c1d

+++ b/doc/src/sgml/brin.sgml

- Operator Strategy 7, 13, 16, 24, 25
+ Operator Strategy 7, 16, 24, 25

- Operator Strategy 8, 14, 26, 27
+ Operator Strategy 8, 26, 27


> Also, the patch seems to be trying to resurrect hstore--1.0--1.1.sql,

Not sure why or how I had that.

-- 
Justin
>From d9cfb33c1a87a8404fa949613500acd1021254cd Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Sat, 11 Apr 2020 22:57:06 -0500
Subject: [PATCH] remove deprecated v8.2 containment operators

See also:
ba920e1c9182eac55d5f1327ab0d29b721154277 684ad6a92fcc33adebdab65c4e7d72a68ba05408
3165426e54df02a6199c0a216546e5095e875a0a 2f70fdb0644c32c4154236c2b5c241bec92eac5e 591d282e8d3e0448ec1339c6b066e10953f040a2
---
 contrib/cube/Makefile   |  2 +-
 contrib/cube/cube--1.4--1.5.sql |  8 
 contrib/cube/cube.control   |  2 +-
 contrib/hstore/Makefile |  1 +
 contrib/hstore/hstore--1.8--1.9.sql |  7 +++
 contrib/hstore/hstore.control   |  2 +-
 contrib/intarray/Makefile   |  2 +-
 contrib/intarray/intarray--1.4--1.5.sql | 10 ++
 contrib/intarray/intarray.control   |  2 +-
 contrib/seg/Makefile|  2 +-
 contrib/seg/seg--1.3--1.4.sql   |  8 
 contrib/seg/seg.control |  2 +-
 doc/src/sgml/cube.sgml  |  8 
 doc/src/sgml/hstore.sgml| 10 --
 doc/src/sgml/intarray.sgml  |  8 
 doc/src/sgml/seg.sgml   |  8 
 16 files changed, 41 insertions(+), 41 deletions(-)
 create mode 100644 contrib/cube/cube--1.4--1.5.sql
 create mode 100644 contrib/hstore/hstore--1.8--1.9.sql
 create mode 100644 contrib/intarray/intarray--1.4--1.5.sql
 create mode 100644 contrib/seg/seg--1.3--1.4.sql

diff --git a/contrib/cube/Makefile b/contrib/cube/Makefile
index 54f609db17..cf195506c7 100644
--- a/contrib/cube/Makefile
+++ b/contrib/cube/Makefile
@@ -7,7 +7,7 @@ OBJS = \
 	cubeparse.o
 
 EXTENSION = cube
-DATA = cube--1.2.sql cube--1.2--1.3.sql cube--1.3--1.4.sql \
+DATA = cube--1.2.sql cube--1.2--1.3.sql cube--1.3--1.4.sql cube--1.4--1.5.sql \
 	cube--1.1--1.2.sql cube--1.0--1.1.sql
 PGFILEDESC = "cube - multidimensional cube data type"
 
diff --git a/contrib/cube/cube--1.4--1.5.sql b/contrib/cube/cube--1.4--1.5.sql
new file mode 100644
index 00..54492e5d18
--- /dev/null
+++ b/contrib/cube/cube--1.4--1.5.sql
@@ -0,0 +1,8 @@
+/* contrib/cube/cube--1.4--1.5.sql */
+
+-- complain if script is sourced in psql, rather than via ALTER EXTENSION
+\echo Use "ALTER EXTENSION cube UPDATE TO '1.5'" to load this file. \quit
+
+-- Remove @ and ~
+DROP OPERATOR @ (cube, cube);
+DROP OPERATOR ~ (cube, cube);
diff --git a/contrib/cube/cube.control b/contrib/cube/cube.control
index 3e238fc937..50427ec117 100644
--- a/contrib/cube/cube.control
+++ b/contrib/cube/cube.control
@@ -1,6 +1,6 @@
 # cube extension
 comment = 'data type for multidimensional cubes'
-default_version = '1.4'
+default_version = '1.5'
 module_pathname = '$libdir/cube'
 relocatable = true
 trusted = true
diff --git a/contrib/hstore/Makefile b/contrib/hstore/Makefile
index c4e339b57c..97b228b65f 100644
--- a/contrib/hstore/Makefile
+++ b/contrib/hstore/Makefile
@@ -12,6 +12,7 @@ OBJS = \
 
 EXTENSION = hstore
 DATA = hstore--1.4.sql \
+	hstore--1.8--1.9.sql \
 	hstore--1.7--1.8.sql \
 	hstore--1.6--1.7.sql \
 	hstore--1.5--1.6.sql \
diff --git a/contrib/hstore/hstore--1.8--1.9.sql b/contrib/hstore/hstore--1.8--1.9.sql
new file mode 100644
index 00..7cd3467c55
--- /dev/null
+++ b/contrib/hstore/hstore--1.8--1.9.sql
@@ -0,0 +1,7 @@
+/* contrib/hstore/hstore--1.8--1.9.sql */
+
+-- complain if script is sourced in psql, rather than via ALTER EXTENSION
+\echo Use "ALTER EXTENSION hstore UPDATE TO '1.9'" to load this file. \quit
+
+-- Remove @
+DROP OPERATOR @ (hstore, hstore);
diff --git a/contrib/hstore/hstore.control b/contrib/hstore/hstore.control
index 89e3c746c4..b73c28aa4d 100644
--- a/contrib/hstore/hstore.control
+++ b/contrib/hstore/hstore.control
@@ -1,6 +1,6 @@
 # hstore extension
 comment = 'data type for storing sets of (key, value) pairs'
-default_version = '1.8'
+default_version = '1.9'
 module_pathname = '$libdir/hstore'
 relocatable = true
 trusted = true
diff --git a/contrib/intarray/Makefile b/contrib/intarray/Makefile
index 01faa36b10..3817c1669a 100644
--- a/contrib/intarray/Makefile
+++ b/contrib/intarray/Makefile
@@ -12,7 +12,7 @@ OBJS = \
 	_intbig_gist.o
 
 EXTENSION = intar

Re: [PATCH] remove deprecated v8.2 containment operators

[Tom Lane]

Justin Pryzby  writes:
> [ 0001-remove-deprecated-v8.2-containment-operators.patch ]

I'm confused by why this patch is only dropping the operators'
opclass-membership links.  Don't we want to actually DROP OPERATOR
too?

Also, the patch seems to be trying to resurrect hstore--1.0--1.1.sql,
which I do not see the point of.  It was removed because no modern
server will even think it's valid syntax, and that situation has
not changed.

regards, tom lane

Re: [PATCH] remove deprecated v8.2 containment operators

[Justin Pryzby]

On Mon, Nov 30, 2020 at 02:09:10PM -0500, Tom Lane wrote:
> Justin Pryzby  writes:
> > I think this is waiting on me to provide a patch for the contrib/ modules 
> > with
> > update script removing the SQL operators, and documentating their 
> > deprecation.
> 
> Right.  We can remove the SQL operators, but not (yet) the C code support.
> 
> I'm not sure that the docs change would do more than remove any existing
> mentions of the operators.

I've finally returned to this.  RFC.

-- 
Justin
>From 98a870449bdf3670e2e190d8bdfccd59dae42452 Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Sat, 11 Apr 2020 22:57:06 -0500
Subject: [PATCH] remove deprecated v8.2 containment operators

See also:
ba920e1c9182eac55d5f1327ab0d29b721154277 684ad6a92fcc33adebdab65c4e7d72a68ba05408
3165426e54df02a6199c0a216546e5095e875a0a 2f70fdb0644c32c4154236c2b5c241bec92eac5e 591d282e8d3e0448ec1339c6b066e10953f040a2
---
 contrib/cube/Makefile   |  2 +-
 contrib/cube/cube--1.4--1.5.sql | 10 ++
 contrib/cube/cube.control   |  2 +-
 contrib/hstore/Makefile |  1 +
 contrib/hstore/hstore--1.0--1.1.sql |  7 +++
 contrib/hstore/hstore--1.8--1.9.sql |  8 
 contrib/hstore/hstore.control   |  2 +-
 contrib/intarray/Makefile   |  2 +-
 contrib/intarray/intarray--1.4--1.5.sql | 18 ++
 contrib/intarray/intarray.control   |  2 +-
 contrib/seg/Makefile|  2 +-
 contrib/seg/seg--1.3--1.4.sql   | 11 +++
 contrib/seg/seg.control |  2 +-
 doc/src/sgml/cube.sgml  |  8 
 doc/src/sgml/hstore.sgml| 10 --
 doc/src/sgml/intarray.sgml  |  8 
 doc/src/sgml/seg.sgml   |  8 
 17 files changed, 62 insertions(+), 41 deletions(-)
 create mode 100644 contrib/cube/cube--1.4--1.5.sql
 create mode 100644 contrib/hstore/hstore--1.0--1.1.sql
 create mode 100644 contrib/hstore/hstore--1.8--1.9.sql
 create mode 100644 contrib/intarray/intarray--1.4--1.5.sql
 create mode 100644 contrib/seg/seg--1.3--1.4.sql

diff --git a/contrib/cube/Makefile b/contrib/cube/Makefile
index 54f609db17..cf195506c7 100644
--- a/contrib/cube/Makefile
+++ b/contrib/cube/Makefile
@@ -7,7 +7,7 @@ OBJS = \
 	cubeparse.o
 
 EXTENSION = cube
-DATA = cube--1.2.sql cube--1.2--1.3.sql cube--1.3--1.4.sql \
+DATA = cube--1.2.sql cube--1.2--1.3.sql cube--1.3--1.4.sql cube--1.4--1.5.sql \
 	cube--1.1--1.2.sql cube--1.0--1.1.sql
 PGFILEDESC = "cube - multidimensional cube data type"
 
diff --git a/contrib/cube/cube--1.4--1.5.sql b/contrib/cube/cube--1.4--1.5.sql
new file mode 100644
index 00..07a5f0755d
--- /dev/null
+++ b/contrib/cube/cube--1.4--1.5.sql
@@ -0,0 +1,10 @@
+/* contrib/cube/cube--1.4--1.5.sql */
+
+-- complain if script is sourced in psql, rather than via ALTER EXTENSION
+\echo Use "ALTER EXTENSION cube UPDATE TO '1.5'" to load this file. \quit
+
+-- Remove @ and ~
+ALTER OPERATOR FAMILY gist_cube_ops USING gist
+DROP OPERATOR 13 (cube, cube);
+ALTER OPERATOR FAMILY gist_cube_ops USING gist
+DROP OPERATOR 14 (cube, cube);
diff --git a/contrib/cube/cube.control b/contrib/cube/cube.control
index 3e238fc937..50427ec117 100644
--- a/contrib/cube/cube.control
+++ b/contrib/cube/cube.control
@@ -1,6 +1,6 @@
 # cube extension
 comment = 'data type for multidimensional cubes'
-default_version = '1.4'
+default_version = '1.5'
 module_pathname = '$libdir/cube'
 relocatable = true
 trusted = true
diff --git a/contrib/hstore/Makefile b/contrib/hstore/Makefile
index c4e339b57c..97b228b65f 100644
--- a/contrib/hstore/Makefile
+++ b/contrib/hstore/Makefile
@@ -12,6 +12,7 @@ OBJS = \
 
 EXTENSION = hstore
 DATA = hstore--1.4.sql \
+	hstore--1.8--1.9.sql \
 	hstore--1.7--1.8.sql \
 	hstore--1.6--1.7.sql \
 	hstore--1.5--1.6.sql \
diff --git a/contrib/hstore/hstore--1.0--1.1.sql b/contrib/hstore/hstore--1.0--1.1.sql
new file mode 100644
index 00..4e32a575c5
--- /dev/null
+++ b/contrib/hstore/hstore--1.0--1.1.sql
@@ -0,0 +1,7 @@
+/* contrib/hstore/hstore--1.0--1.1.sql */
+
+-- complain if script is sourced in psql, rather than via ALTER EXTENSION
+\echo Use "ALTER EXTENSION hstore UPDATE TO '1.1'" to load this file. \quit
+
+ALTER EXTENSION hstore DROP OPERATOR => (text, text);
+DROP OPERATOR => (text, text);
diff --git a/contrib/hstore/hstore--1.8--1.9.sql b/contrib/hstore/hstore--1.8--1.9.sql
new file mode 100644
index 00..2220fee444
--- /dev/null
+++ b/contrib/hstore/hstore--1.8--1.9.sql
@@ -0,0 +1,8 @@
+/* contrib/hstore/hstore--1.8--1.9.sql */
+
+-- complain if script is sourced in psql, rather than via ALTER EXTENSION
+\echo Use "ALTER EXTENSION hstore UPDATE TO '1.9'" to load this file. \quit
+
+-- Remove @
+ALTER OPERATOR FAMILY gist_hstore_ops USING gist
+DROP OPERATOR 13 (hstore, hstore);
diff --git a/contrib/hstore/hstore.contr

Re: [PATCH] remove deprecated v8.2 containment operators

[Masahiko Sawada]

On Thu, Jan 28, 2021 at 9:50 PM Masahiko Sawada  wrote:
>
> On Tue, Dec 1, 2020 at 4:09 AM Tom Lane  wrote:
> >
> > Justin Pryzby  writes:
> > > I think this is waiting on me to provide a patch for the contrib/ modules 
> > > with
> > > update script removing the SQL operators, and documentating their 
> > > deprecation.
> >
> > Right.  We can remove the SQL operators, but not (yet) the C code support.
> >
> > I'm not sure that the docs change would do more than remove any existing
> > mentions of the operators.
> >
>
> Status update for a commitfest entry.
>
> Almost 2 months passed since the last update. Are you planning to work
> on this, Justin? If not, I'm planning to set it "Returned with
> Feedback" barring objectinos.
>

This patch has been awaiting updates for more than one month. As such,
we have moved it to "Returned with Feedback" and removed it from the
reviewing queue. Depending on timing, this may be reversable, so let
us know if there are extenuating circumstances. In any case, you are
welcome to address the feedback you have received, and resubmit the
patch to the next CommitFest.

Thank you for contributing to PostgreSQL.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

Re: [PATCH] remove deprecated v8.2 containment operators

[Masahiko Sawada]

On Tue, Dec 1, 2020 at 4:09 AM Tom Lane  wrote:
>
> Justin Pryzby  writes:
> > I think this is waiting on me to provide a patch for the contrib/ modules 
> > with
> > update script removing the SQL operators, and documentating their 
> > deprecation.
>
> Right.  We can remove the SQL operators, but not (yet) the C code support.
>
> I'm not sure that the docs change would do more than remove any existing
> mentions of the operators.
>

Status update for a commitfest entry.

Almost 2 months passed since the last update. Are you planning to work
on this, Justin? If not, I'm planning to set it "Returned with
Feedback" barring objectinos.

Regards,


--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

Re: [PATCH] remove deprecated v8.2 containment operators

[Tom Lane]

Justin Pryzby  writes:
> I think this is waiting on me to provide a patch for the contrib/ modules with
> update script removing the SQL operators, and documentating their deprecation.

Right.  We can remove the SQL operators, but not (yet) the C code support.

I'm not sure that the docs change would do more than remove any existing
mentions of the operators.

regards, tom lane

Re: [PATCH] remove deprecated v8.2 containment operators

[Justin Pryzby]

On Mon, Nov 30, 2020 at 09:51:12PM +0300, Anastasia Lubennikova wrote:
> On 16.11.2020 23:55, Justin Pryzby wrote:
> > On Fri, Nov 13, 2020 at 10:03:43AM -0500, Stephen Frost wrote:
> > > * Magnus Hagander (mag...@hagander.net) wrote:
> > > > On Thu, Nov 12, 2020 at 11:28 PM Tom Lane  wrote:
> > > > > > The changes to the contrib modules appear to be incomplete in some 
> > > > > > ways.
> > > > > >In cube, hstore, and seg, there are no changes to the extension
> > > > > > scripts to remove the operators.  All you're doing is changing the C
> > > > > > code to no longer recognize the strategy, but that doesn't explain 
> > > > > > what
> > > > > > will happen if the operator is still used.  In intarray, by 
> > > > > > contrast,
> > > > > > you're editing an existing extension script, but that should be 
> > > > > > done by
> > > > > > an upgrade script instead.
> > > > > In the contrib modules, I'm afraid what you gotta do is remove the
> > > > > SQL operator definitions but leave the opclass code support in place.
> > > > > That's because there's no guarantee that users will update the 
> > > > > extension's
> > > > > SQL version immediately, so a v14 build of the .so might still be used
> > > > > with the old SQL definitions.  It's not clear how much window we need
> > > > > give for people to do that update, but I don't think "zero" is an
> > > > > acceptable answer.
> > > > Based on my experience from the field, the answer is "never".
> > > > 
> > > > As in, most people have no idea they are even *supposed* to do such an
> > > > upgrade, so they don't do it. Until we solve that problem, I think
> > > > we're basically stuck with keeping them "forever". (and even if/when
> > > > we do, "zero" is probably not going to cut it, no)
> > > Yeah, this is a serious problem and one that we should figure out a way
> > > to fix or at least improve on- maybe by having pg_upgrade say something
> > > about extensions that could/should be upgraded..?
> > I think what's needed are:
> > 
> > 1) a way to *warn* users about deprecation.  CREATE EXTENSION might give an
> > elog(WARNING), but it's probably not enough.  It only happens once, and if 
> > it's
> > in pg_restore/pg_upgrade, it be wrapped by vendor upgrade scripts.  It 
> > needs to
> > be more like first function call in every session.  Or more likely, put it 
> > in
> > documentation for 10 years.
> > 
> > 2) a way to *enforce* it, like making CREATE EXTENSION fail when run 
> > against an
> > excessively old server, including by pg_restore/pg_upgrade (which ought to 
> > also
> > handle it in --check).
> > 
> > Are there any contrib for which (1) is done and we're anywhere near doing 
> > (2) ?
> 
> Status update for a commitfest entry.
> 
> The commitfest is nearing the end and this thread is "Waiting on Author". As

I think this is waiting on me to provide a patch for the contrib/ modules with
update script removing the SQL operators, and documentating their deprecation.

Is that right ?

-- 
Justin

Re: [PATCH] remove deprecated v8.2 containment operators

[Tom Lane]

Anastasia Lubennikova  writes:
> Status update for a commitfest entry.

> The commitfest is nearing the end and this thread is "Waiting on 
> Author". As far as I see we don't have a patch here and discussion is a 
> bit stuck.
> So, I am planning to return it with feedback. Any objections?

AFAICS, the status is

(1) core-code changes are committed;

(2) proposed edits to contrib modules need significant rework;

(3) there was also a bit of discussion about inventing a mechanism
to prod users to update out-of-date extensions.

Now, (3) is far outside the scope of this patch, and I do not
think it should block finishing (2).  We need a new patch for (2),
but there's no real doubt as to what it should contain -- Justin
just needs to turn the crank.

You could either move this to the next CF in state WoA, or
close it RWF.  But the patch did make progress in this CF,
so I'd tend to lean to the former.

regards, tom lane

Re: [PATCH] remove deprecated v8.2 containment operators

[Anastasia Lubennikova]

On 16.11.2020 23:55, Justin Pryzby wrote:

On Fri, Nov 13, 2020 at 10:03:43AM -0500, Stephen Frost wrote:

* Magnus Hagander (mag...@hagander.net) wrote:

On Thu, Nov 12, 2020 at 11:28 PM Tom Lane  wrote:

The changes to the contrib modules appear to be incomplete in some ways.
   In cube, hstore, and seg, there are no changes to the extension
scripts to remove the operators.  All you're doing is changing the C
code to no longer recognize the strategy, but that doesn't explain what
will happen if the operator is still used.  In intarray, by contrast,
you're editing an existing extension script, but that should be done by
an upgrade script instead.

In the contrib modules, I'm afraid what you gotta do is remove the
SQL operator definitions but leave the opclass code support in place.
That's because there's no guarantee that users will update the extension's
SQL version immediately, so a v14 build of the .so might still be used
with the old SQL definitions.  It's not clear how much window we need
give for people to do that update, but I don't think "zero" is an
acceptable answer.

Based on my experience from the field, the answer is "never".

As in, most people have no idea they are even *supposed* to do such an
upgrade, so they don't do it. Until we solve that problem, I think
we're basically stuck with keeping them "forever". (and even if/when
we do, "zero" is probably not going to cut it, no)

Yeah, this is a serious problem and one that we should figure out a way
to fix or at least improve on- maybe by having pg_upgrade say something
about extensions that could/should be upgraded..?

I think what's needed are:

1) a way to *warn* users about deprecation.  CREATE EXTENSION might give an
elog(WARNING), but it's probably not enough.  It only happens once, and if it's
in pg_restore/pg_upgrade, it be wrapped by vendor upgrade scripts.  It needs to
be more like first function call in every session.  Or more likely, put it in
documentation for 10 years.

2) a way to *enforce* it, like making CREATE EXTENSION fail when run against an
excessively old server, including by pg_restore/pg_upgrade (which ought to also
handle it in --check).

Are there any contrib for which (1) is done and we're anywhere near doing (2) ?



Status update for a commitfest entry.

The commitfest is nearing the end and this thread is "Waiting on 
Author". As far as I see we don't have a patch here and discussion is a 
bit stuck.

So, I am planning to return it with feedback. Any objections?

--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [PATCH] remove deprecated v8.2 containment operators

[Justin Pryzby]

On Fri, Nov 13, 2020 at 10:03:43AM -0500, Stephen Frost wrote:
> * Magnus Hagander (mag...@hagander.net) wrote:
> > On Thu, Nov 12, 2020 at 11:28 PM Tom Lane  wrote:
> > > > The changes to the contrib modules appear to be incomplete in some ways.
> > > >   In cube, hstore, and seg, there are no changes to the extension
> > > > scripts to remove the operators.  All you're doing is changing the C
> > > > code to no longer recognize the strategy, but that doesn't explain what
> > > > will happen if the operator is still used.  In intarray, by contrast,
> > > > you're editing an existing extension script, but that should be done by
> > > > an upgrade script instead.
> > >
> > > In the contrib modules, I'm afraid what you gotta do is remove the
> > > SQL operator definitions but leave the opclass code support in place.
> > > That's because there's no guarantee that users will update the extension's
> > > SQL version immediately, so a v14 build of the .so might still be used
> > > with the old SQL definitions.  It's not clear how much window we need
> > > give for people to do that update, but I don't think "zero" is an
> > > acceptable answer.
> > 
> > Based on my experience from the field, the answer is "never".
> > 
> > As in, most people have no idea they are even *supposed* to do such an
> > upgrade, so they don't do it. Until we solve that problem, I think
> > we're basically stuck with keeping them "forever". (and even if/when
> > we do, "zero" is probably not going to cut it, no)
> 
> Yeah, this is a serious problem and one that we should figure out a way
> to fix or at least improve on- maybe by having pg_upgrade say something
> about extensions that could/should be upgraded..?

I think what's needed are:

1) a way to *warn* users about deprecation.  CREATE EXTENSION might give an
elog(WARNING), but it's probably not enough.  It only happens once, and if it's
in pg_restore/pg_upgrade, it be wrapped by vendor upgrade scripts.  It needs to
be more like first function call in every session.  Or more likely, put it in
documentation for 10 years.

2) a way to *enforce* it, like making CREATE EXTENSION fail when run against an
excessively old server, including by pg_restore/pg_upgrade (which ought to also
handle it in --check).

Are there any contrib for which (1) is done and we're anywhere near doing (2) ?

-- 
Justin

Re: [PATCH] remove deprecated v8.2 containment operators

[Peter Eisentraut]

On 2020-11-13 16:57, Tom Lane wrote:

Peter Eisentraut  writes:

On 2020-11-12 23:28, Tom Lane wrote:

I'm on board with pulling these now --- 8.2 to v14 is plenty of
deprecation notice.  However, the patch seems incomplete in that
the code support for these is still there -- look for
RTOldContainedByStrategyNumber and RTOldContainsStrategyNumber.
Admittedly, there's not much to be removed except some case labels,
but it still seems like we oughta do that to avoid future confusion.



Yeah, the stuff in gistproc.c should be removed now.  But I wonder what
the mentions in brin_inclusion.c are and whether or how they should be
removed.


I think those probably got cargo-culted in there at some point.
They're visibly dead code, because there are no pg_amop entries
for BRIN opclasses with amopstrategy 13 or 14.


I have committed fixes that remove the unused strategy numbers from both 
of these code areas.

Re: [PATCH] remove deprecated v8.2 containment operators

[Tom Lane]

Peter Eisentraut  writes:
> On 2020-11-12 23:28, Tom Lane wrote:
>> I'm on board with pulling these now --- 8.2 to v14 is plenty of
>> deprecation notice.  However, the patch seems incomplete in that
>> the code support for these is still there -- look for
>> RTOldContainedByStrategyNumber and RTOldContainsStrategyNumber.
>> Admittedly, there's not much to be removed except some case labels,
>> but it still seems like we oughta do that to avoid future confusion.

> Yeah, the stuff in gistproc.c should be removed now.  But I wonder what 
> the mentions in brin_inclusion.c are and whether or how they should be 
> removed.

I think those probably got cargo-culted in there at some point.
They're visibly dead code, because there are no pg_amop entries
for BRIN opclasses with amopstrategy 13 or 14.

This comment that you removed in 2f70fdb06 is suspicious:

# we could, but choose not to, supply entries for strategies 13 and 14

I'm guessing that somebody was vacillating about whether it'd be
a feature to support these old operator names in BRIN, and
eventually didn't, but forgot to remove the code support.

regards, tom lane

Re: [PATCH] remove deprecated v8.2 containment operators

[Stephen Frost]

Greetings,

* Magnus Hagander (mag...@hagander.net) wrote:
> On Thu, Nov 12, 2020 at 11:28 PM Tom Lane  wrote:
> > > The changes to the contrib modules appear to be incomplete in some ways.
> > >   In cube, hstore, and seg, there are no changes to the extension
> > > scripts to remove the operators.  All you're doing is changing the C
> > > code to no longer recognize the strategy, but that doesn't explain what
> > > will happen if the operator is still used.  In intarray, by contrast,
> > > you're editing an existing extension script, but that should be done by
> > > an upgrade script instead.
> >
> > In the contrib modules, I'm afraid what you gotta do is remove the
> > SQL operator definitions but leave the opclass code support in place.
> > That's because there's no guarantee that users will update the extension's
> > SQL version immediately, so a v14 build of the .so might still be used
> > with the old SQL definitions.  It's not clear how much window we need
> > give for people to do that update, but I don't think "zero" is an
> > acceptable answer.
> 
> Based on my experience from the field, the answer is "never".
> 
> As in, most people have no idea they are even *supposed* to do such an
> upgrade, so they don't do it. Until we solve that problem, I think
> we're basically stuck with keeping them "forever". (and even if/when
> we do, "zero" is probably not going to cut it, no)

Yeah, this is a serious problem and one that we should figure out a way
to fix or at least improve on- maybe by having pg_upgrade say something
about extensions that could/should be upgraded..?

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: [PATCH] remove deprecated v8.2 containment operators

[Magnus Hagander]

On Thu, Nov 12, 2020 at 11:28 PM Tom Lane  wrote:
>
> > The changes to the contrib modules appear to be incomplete in some ways.
> >   In cube, hstore, and seg, there are no changes to the extension
> > scripts to remove the operators.  All you're doing is changing the C
> > code to no longer recognize the strategy, but that doesn't explain what
> > will happen if the operator is still used.  In intarray, by contrast,
> > you're editing an existing extension script, but that should be done by
> > an upgrade script instead.
>
> In the contrib modules, I'm afraid what you gotta do is remove the
> SQL operator definitions but leave the opclass code support in place.
> That's because there's no guarantee that users will update the extension's
> SQL version immediately, so a v14 build of the .so might still be used
> with the old SQL definitions.  It's not clear how much window we need
> give for people to do that update, but I don't think "zero" is an
> acceptable answer.

Based on my experience from the field, the answer is "never".

As in, most people have no idea they are even *supposed* to do such an
upgrade, so they don't do it. Until we solve that problem, I think
we're basically stuck with keeping them "forever". (and even if/when
we do, "zero" is probably not going to cut it, no)

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

Re: [PATCH] remove deprecated v8.2 containment operators

[Peter Eisentraut]

On 2020-11-12 23:28, Tom Lane wrote:

I'm on board with pulling these now --- 8.2 to v14 is plenty of
deprecation notice.  However, the patch seems incomplete in that
the code support for these is still there -- look for
RTOldContainedByStrategyNumber and RTOldContainsStrategyNumber.
Admittedly, there's not much to be removed except some case labels,
but it still seems like we oughta do that to avoid future confusion.


Yeah, the stuff in gistproc.c should be removed now.  But I wonder what 
the mentions in brin_inclusion.c are and whether or how they should be 
removed.

Re: [PATCH] remove deprecated v8.2 containment operators

[Tom Lane]

Peter Eisentraut  writes:
> On 2020-10-27 04:25, Justin Pryzby wrote:
>> They have been deprecated for a Long Time, so finally remove them for v14.
>> Four fewer exclamation marks makes the documentation less exciting, which is 
>> a
>> good thing.

> I have committed the parts that remove the built-in geometry operators 
> and the related regression test changes.

I'm on board with pulling these now --- 8.2 to v14 is plenty of
deprecation notice.  However, the patch seems incomplete in that
the code support for these is still there -- look for
RTOldContainedByStrategyNumber and RTOldContainsStrategyNumber.
Admittedly, there's not much to be removed except some case labels,
but it still seems like we oughta do that to avoid future confusion.

> The changes to the contrib modules appear to be incomplete in some ways. 
>   In cube, hstore, and seg, there are no changes to the extension 
> scripts to remove the operators.  All you're doing is changing the C 
> code to no longer recognize the strategy, but that doesn't explain what 
> will happen if the operator is still used.  In intarray, by contrast, 
> you're editing an existing extension script, but that should be done by 
> an upgrade script instead.

In the contrib modules, I'm afraid what you gotta do is remove the
SQL operator definitions but leave the opclass code support in place.
That's because there's no guarantee that users will update the extension's
SQL version immediately, so a v14 build of the .so might still be used
with the old SQL definitions.  It's not clear how much window we need
give for people to do that update, but I don't think "zero" is an
acceptable answer.

(The core code doesn't have to concern itself with such scenarios,
since we require the initial catalog contents to match the backend
major version.  Hence it is okay to remove the code support now in
the in-core opclasses.)

regards, tom lane

Re: [PATCH] remove deprecated v8.2 containment operators

[Peter Eisentraut]

On 2020-10-27 04:25, Justin Pryzby wrote:

Forking this thread:
https://www.postgresql.org/message-id/fd93f1c5-7818-a02c-01e5-1075ac0d4...@iki.fi

They have been deprecated for a Long Time, so finally remove them for v14.
Four fewer exclamation marks makes the documentation less exciting, which is a
good thing.


I have committed the parts that remove the built-in geometry operators 
and the related regression test changes.


The changes to the contrib modules appear to be incomplete in some ways. 
 In cube, hstore, and seg, there are no changes to the extension 
scripts to remove the operators.  All you're doing is changing the C 
code to no longer recognize the strategy, but that doesn't explain what 
will happen if the operator is still used.  In intarray, by contrast, 
you're editing an existing extension script, but that should be done by 
an upgrade script instead.

Re: [PATCH] remove deprecated v8.2 containment operators

[Magnus Hagander]

On Tue, Oct 27, 2020 at 9:38 AM Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 2020-10-27 04:25, Justin Pryzby wrote:
> > Forking this thread:
> >
> https://www.postgresql.org/message-id/fd93f1c5-7818-a02c-01e5-1075ac0d4...@iki.fi
> >
> > They have been deprecated for a Long Time, so finally remove them for
> v14.
> > Four fewer exclamation marks makes the documentation less exciting,
> which is a
> > good thing.
>
> I don't know the reason or context why they were deprecated, but I agree
> that the timeline for removing them now is good.
>

IIRC it was to align things so that "containment" had the same operator for
all different kinds of datatypes?

But whether that memory is right nor not it was indeed a long time ago,
so +1 that it's definitely time to get rid of them.

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

Re: [PATCH] remove deprecated v8.2 containment operators

[Peter Eisentraut]

On 2020-10-27 04:25, Justin Pryzby wrote:

Forking this thread:
https://www.postgresql.org/message-id/fd93f1c5-7818-a02c-01e5-1075ac0d4...@iki.fi

They have been deprecated for a Long Time, so finally remove them for v14.
Four fewer exclamation marks makes the documentation less exciting, which is a
good thing.


I don't know the reason or context why they were deprecated, but I agree 
that the timeline for removing them now is good.


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

[PATCH] remove deprecated v8.2 containment operators

[Justin Pryzby]

Forking this thread:
https://www.postgresql.org/message-id/fd93f1c5-7818-a02c-01e5-1075ac0d4...@iki.fi

They have been deprecated for a Long Time, so finally remove them for v14.
Four fewer exclamation marks makes the documentation less exciting, which is a
good thing.
>From 7868fee24f92fb5150735f1f9507cfe9a6ab212c Mon Sep 17 00:00:00 2001
From: Justin Pryzby 
Date: Sat, 11 Apr 2020 22:57:06 -0500
Subject: [PATCH] remove deprecated v8.2 containment operators

See also:
ba920e1c9182eac55d5f1327ab0d29b721154277
684ad6a92fcc33adebdab65c4e7d72a68ba05408
---
 contrib/cube/cube.c|  4 --
 contrib/hstore/hstore.h|  1 -
 contrib/hstore/hstore_gist.c   |  3 +-
 contrib/intarray/_int_gin.c|  4 --
 contrib/intarray/_int_gist.c   |  2 -
 contrib/intarray/_intbig_gist.c|  2 -
 contrib/intarray/intarray--1.2.sql |  6 --
 contrib/intarray/intarray--1.3--1.4.sql|  7 ---
 contrib/seg/seg.c  |  4 --
 doc/src/sgml/cube.sgml |  8 ---
 doc/src/sgml/func.sgml |  9 ---
 doc/src/sgml/hstore.sgml   | 10 
 doc/src/sgml/intarray.sgml |  8 ---
 doc/src/sgml/seg.sgml  |  8 ---
 src/backend/access/brin/brin_inclusion.c   |  2 -
 src/backend/access/gist/gistproc.c |  4 --
 src/include/access/stratnum.h  |  2 -
 src/include/catalog/pg_amop.dat| 16 --
 src/include/catalog/pg_operator.dat| 65 --
 src/test/regress/expected/create_am.out| 12 ++--
 src/test/regress/expected/create_index.out | 24 
 src/test/regress/expected/opr_sanity.out   |  7 +--
 src/test/regress/sql/create_am.sql |  6 +-
 src/test/regress/sql/create_index.sql  | 12 ++--
 24 files changed, 28 insertions(+), 198 deletions(-)

diff --git a/contrib/cube/cube.c b/contrib/cube/cube.c
index 6f810b26c5..57b89d91b8 100644
--- a/contrib/cube/cube.c
+++ b/contrib/cube/cube.c
@@ -634,11 +634,9 @@ g_cube_leaf_consistent(NDBOX *key,
 			retval = (cube_cmp_v0(key, query) == 0);
 			break;
 		case RTContainsStrategyNumber:
-		case RTOldContainsStrategyNumber:
 			retval = cube_contains_v0(key, query);
 			break;
 		case RTContainedByStrategyNumber:
-		case RTOldContainedByStrategyNumber:
 			retval = cube_contains_v0(query, key);
 			break;
 		default:
@@ -661,11 +659,9 @@ g_cube_internal_consistent(NDBOX *key,
 			break;
 		case RTSameStrategyNumber:
 		case RTContainsStrategyNumber:
-		case RTOldContainsStrategyNumber:
 			retval = (bool) cube_contains_v0(key, query);
 			break;
 		case RTContainedByStrategyNumber:
-		case RTOldContainedByStrategyNumber:
 			retval = (bool) cube_overlap_v0(key, query);
 			break;
 		default:
diff --git a/contrib/hstore/hstore.h b/contrib/hstore/hstore.h
index bf4a565ed9..8713a22dcc 100644
--- a/contrib/hstore/hstore.h
+++ b/contrib/hstore/hstore.h
@@ -181,7 +181,6 @@ extern Pairs *hstoreArrayToPairs(ArrayType *a, int *npairs);
 #define HStoreExistsStrategyNumber		9
 #define HStoreExistsAnyStrategyNumber	10
 #define HStoreExistsAllStrategyNumber	11
-#define HStoreOldContainsStrategyNumber 13	/* backwards compatibility */
 
 /*
  * defining HSTORE_POLLUTE_NAMESPACE=0 will prevent use of old function names;
diff --git a/contrib/hstore/hstore_gist.c b/contrib/hstore/hstore_gist.c
index 102c9cea72..10c9906fa5 100644
--- a/contrib/hstore/hstore_gist.c
+++ b/contrib/hstore/hstore_gist.c
@@ -517,8 +517,7 @@ ghstore_consistent(PG_FUNCTION_ARGS)
 
 	sign = GETSIGN(entry);
 
-	if (strategy == HStoreContainsStrategyNumber ||
-		strategy == HStoreOldContainsStrategyNumber)
+	if (strategy == HStoreContainsStrategyNumber)
 	{
 		HStore	   *query = PG_GETARG_HSTORE_P(1);
 		HEntry	   *qe = ARRPTR(query);
diff --git a/contrib/intarray/_int_gin.c b/contrib/intarray/_int_gin.c
index b7958d8eca..72cfbc2fd7 100644
--- a/contrib/intarray/_int_gin.c
+++ b/contrib/intarray/_int_gin.c
@@ -78,7 +78,6 @@ ginint4_queryextract(PG_FUNCTION_ARGS)
 *searchMode = GIN_SEARCH_MODE_DEFAULT;
 break;
 			case RTContainedByStrategyNumber:
-			case RTOldContainedByStrategyNumber:
 /* empty set is contained in everything */
 *searchMode = GIN_SEARCH_MODE_INCLUDE_EMPTY;
 break;
@@ -89,7 +88,6 @@ ginint4_queryextract(PG_FUNCTION_ARGS)
 	*searchMode = GIN_SEARCH_MODE_INCLUDE_EMPTY;
 break;
 			case RTContainsStrategyNumber:
-			case RTOldContainsStrategyNumber:
 if (*nentries > 0)
 	*searchMode = GIN_SEARCH_MODE_DEFAULT;
 else			/* everything contains the empty set */
@@ -127,7 +125,6 @@ ginint4_consistent(PG_FUNCTION_ARGS)
 			res = true;
 			break;
 		case RTContainedByStrategyNumber:
-		case RTOldContainedByStrategyNumber:
 			/* we will need recheck */
 			*recheck = true;
 			/* at least one element in check[] is true, so result = true */
@@ -148,7 +145,6 @@ ginint4_consistent(PG_FUNCTION_ARGS)
 			}
 			break;