[PATCH] remove deprecated v8.2 containment operators

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] remove deprecated v8.2 containment operators

Justin Pryzby
Forking this thread:
https://www.postgresql.org/message-id/fd93f1c5-7818-a02c-01e5-1075ac0d4def@...

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.

0001-remove-deprecated-v8.2-containment-operators.patch (30K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Peter Eisentraut-6
On 2020-10-27 04:25, Justin Pryzby wrote:
> Forking this thread:
> https://www.postgresql.org/message-id/fd93f1c5-7818-a02c-01e5-1075ac0d4def@...
>
> 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


Reply | Threaded
Open this post in threaded view
|

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

Magnus Hagander-2


On Tue, Oct 27, 2020 at 9:38 AM Peter Eisentraut <[hidden email]> wrote:
On 2020-10-27 04:25, Justin Pryzby wrote:
> Forking this thread:
> https://www.postgresql.org/message-id/fd93f1c5-7818-a02c-01e5-1075ac0d4def@...
>
> 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.

--
Reply | Threaded
Open this post in threaded view
|

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

Peter Eisentraut-7
In reply to this post by Justin Pryzby
On 2020-10-27 04:25, Justin Pryzby wrote:
> Forking this thread:
> https://www.postgresql.org/message-id/fd93f1c5-7818-a02c-01e5-1075ac0d4def@...
>
> 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.


Reply | Threaded
Open this post in threaded view
|

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

Tom Lane-2
Peter Eisentraut <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Peter Eisentraut-7
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.



Reply | Threaded
Open this post in threaded view
|

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

Magnus Hagander-2
In reply to this post by Tom Lane-2
On Thu, Nov 12, 2020 at 11:28 PM Tom Lane <[hidden email]> 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/


Reply | Threaded
Open this post in threaded view
|

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

Stephen Frost
Greetings,

* Magnus Hagander ([hidden email]) wrote:

> On Thu, Nov 12, 2020 at 11:28 PM Tom Lane <[hidden email]> 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 (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Tom Lane-2
In reply to this post by Peter Eisentraut-7
Peter Eisentraut <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

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

> Peter Eisentraut <[hidden email]> 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.


Reply | Threaded
Open this post in threaded view
|

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

Justin Pryzby
In reply to this post by Stephen Frost
On Fri, Nov 13, 2020 at 10:03:43AM -0500, Stephen Frost wrote:

> * Magnus Hagander ([hidden email]) wrote:
> > On Thu, Nov 12, 2020 at 11:28 PM Tom Lane <[hidden email]> 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