Thread: Re: [PATCH] remove deprecated v8.2 containment operators
On 2020-10-27 04:25, Justin Pryzby wrote: > Forking this thread: > https://www.postgresql.org/message-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
Attachment
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/