[PATCH] remove deprecated v8.2 containment operators

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

Forking this thread:
/messages/by-id/fd93f1c5-7818-a02c-01e5-1075ac0d4def@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

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

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

Forking this thread:

/messages/by-id/fd93f1c5-7818-a02c-01e5-1075ac0d4def@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/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

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

Forking this thread:
/messages/by-id/fd93f1c5-7818-a02c-01e5-1075ac0d4def@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.

Peter Eisentraut <peter.eisentraut@enterprisedb.com> 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

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.

On Thu, Nov 12, 2020 at 11:28 PM Tom Lane <tgl@sss.pgh.pa.us> 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/

Greetings,

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

On Thu, Nov 12, 2020 at 11:28 PM Tom Lane <tgl@sss.pgh.pa.us> 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

Peter Eisentraut <peter.eisentraut@enterprisedb.com> 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

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

Peter Eisentraut <peter.eisentraut@enterprisedb.com> 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.

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

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

On Thu, Nov 12, 2020 at 11:28 PM Tom Lane <tgl@sss.pgh.pa.us> 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

On 16.11.2020 23:55, Justin Pryzby wrote:

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

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

On Thu, Nov 12, 2020 at 11:28 PM Tom Lane <tgl@sss.pgh.pa.us> 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

Anastasia Lubennikova <a.lubennikova@postgrespro.ru> 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

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 (magnus@hagander.net) wrote:

On Thu, Nov 12, 2020 at 11:28 PM Tom Lane <tgl@sss.pgh.pa.us> 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

Justin Pryzby <pryzby@telsasoft.com> 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

On Tue, Dec 1, 2020 at 4:09 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Justin Pryzby <pryzby@telsasoft.com> 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/

On Thu, Jan 28, 2021 at 9:50 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Dec 1, 2020 at 4:09 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Justin Pryzby <pryzby@telsasoft.com> 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/

On Mon, Nov 30, 2020 at 02:09:10PM -0500, Tom Lane wrote:

Justin Pryzby <pryzby@telsasoft.com> 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

Justin Pryzby <pryzby@telsasoft.com> 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

On Thu, Mar 04, 2021 at 08:58:39PM -0500, Tom Lane wrote:

Justin Pryzby <pryzby@telsasoft.com> 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

-     <entry>Operator Strategy 7, 13, 16, 24, 25</entry>
+     <entry>Operator Strategy 7, 16, 24, 25</entry>

-     <entry>Operator Strategy 8, 14, 26, 27</entry>
+     <entry>Operator Strategy 8, 26, 27</entry>

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

Not sure why or how I had that.

--
Justin