Thread: Retiring support for pre-7.3 FK constraint triggers
While looking at the tg_updatedcols patch I happened to notice that we still support pre-7.3 constraint triggers by converting them on the fly. AFAICT this requires a pre-7.3 dump to hit. This was added in late 2007 in a2899ebdc28080eab0f4bb0b8a5f30aa7bb31a89 due to a report from the field, but I doubt this codepath is excercised much today (if at all). Having code which is untested and not excercised by developers (or users, if my assumption holds), yet being reachable by SQL, runs the risk of introducing subtle bugs. Is there a usecase for keeping it, or can/should it be removed in 14? That would still leave a lot of supported versions to upgrade to in case there are users to need this. Unless there are immediate -1's, I'll park this in a CF for v14. cheers ./daniel
Attachment
On 2020-Mar-05, Daniel Gustafsson wrote: > While looking at the tg_updatedcols patch I happened to notice that we still > support pre-7.3 constraint triggers by converting them on the fly. AFAICT this > requires a pre-7.3 dump to hit. > > This was added in late 2007 in a2899ebdc28080eab0f4bb0b8a5f30aa7bb31a89 due to > a report from the field, but I doubt this codepath is excercised much today (if > at all). pg_dump's support for server versions prior to 8.0 was removed by commit 64f3524e2c8d (Oct 2016) so it seems fair to remove this too. If people need to upgrade from anything older than 7.3, they can do an intermediate jump. > Having code which is untested and not excercised by developers (or users, if my > assumption holds), yet being reachable by SQL, runs the risk of introducing > subtle bugs. Is there a usecase for keeping it, or can/should it be removed in > 14? That would still leave a lot of supported versions to upgrade to in case > there are users to need this. Unless there are immediate -1's, I'll park this > in a CF for v14. I know it's a late in the cycle for patches in commitfest, but why not consider this for pg13 nonetheless? It seems simple enough. Also, per https://coverage.postgresql.org/src/backend/commands/trigger.c.gcov.html this is the only large chunk of uncovered code in commands/trigger.c. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes: > On 2020-Mar-05, Daniel Gustafsson wrote: >> While looking at the tg_updatedcols patch I happened to notice that we still >> support pre-7.3 constraint triggers by converting them on the fly. AFAICT this >> requires a pre-7.3 dump to hit. > I know it's a late in the cycle for patches in commitfest, but why not > consider this for pg13 nonetheless? It seems simple enough. Also, per > https://coverage.postgresql.org/src/backend/commands/trigger.c.gcov.html > this is the only large chunk of uncovered code in commands/trigger.c. +1 --- I think this fits in well with my nearby proposal to remove OPAQUE, which is also only relevant for pre-7.3 dumps. Let's just nuke that stuff. regards, tom lane
On 3/5/20 9:42 AM, Tom Lane wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: >> On 2020-Mar-05, Daniel Gustafsson wrote: >>> While looking at the tg_updatedcols patch I happened to notice that we still >>> support pre-7.3 constraint triggers by converting them on the fly. AFAICT this >>> requires a pre-7.3 dump to hit. > >> I know it's a late in the cycle for patches in commitfest, but why not >> consider this for pg13 nonetheless? It seems simple enough. Also, per >> https://coverage.postgresql.org/src/backend/commands/trigger.c.gcov.html >> this is the only large chunk of uncovered code in commands/trigger.c. > > +1 --- I think this fits in well with my nearby proposal to remove > OPAQUE, which is also only relevant for pre-7.3 dumps. Let's just > nuke that stuff. +1. CF entry added: https://commitfest.postgresql.org/27/2506 Regards, -- -David david@pgmasters.net
> On 5 Mar 2020, at 15:42, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Alvaro Herrera <alvherre@2ndquadrant.com> writes: >>> On 2020-Mar-05, Daniel Gustafsson wrote: >>> While looking at the tg_updatedcols patch I happened to notice that we still >>> support pre-7.3 constraint triggers by converting them on the fly. AFAICT this >>> requires a pre-7.3 dump to hit. > >> I know it's a late in the cycle for patches in commitfest, but why not >> consider this for pg13 nonetheless? It seems simple enough. Also, per >> https://coverage.postgresql.org/src/backend/commands/trigger.c.gcov.html >> this is the only large chunk of uncovered code in commands/trigger.c. > > +1 --- I think this fits in well with my nearby proposal to remove > OPAQUE, which is also only relevant for pre-7.3 dumps. Let's just > nuke that stuff. Sounds good. I was opting for 14 to not violate the no new patches in an ongoing CF policy, but if there is concensus fromcommitters then +1 from me. cheers ./daniel
Daniel Gustafsson <daniel@yesql.se> writes:
> On 5 Mar 2020, at 15:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> +1 --- I think this fits in well with my nearby proposal to remove
>> OPAQUE, which is also only relevant for pre-7.3 dumps. Let's just
>> nuke that stuff.
> Sounds good. I was opting for 14 to not violate the no new patches in an ongoing CF policy, but if there is concensus
fromcommitters then +1 from me.
As long as we're thinking of zapping code that is long past its sell-by
date, I propose getting rid of this stanza in indexcmds.c, which
basically causes CREATE INDEX to ignore certain opclass specifications:
/*
* Release 7.0 removed network_ops, timespan_ops, and datetime_ops, so we
* ignore those opclass names so the default *_ops is used. This can be
* removed in some later release. bjm 2000/02/07
*
* Release 7.1 removes lztext_ops, so suppress that too for a while. tgl
* 2000/07/30
*
* Release 7.2 renames timestamp_ops to timestamptz_ops, so suppress that
* too for awhile. I'm starting to think we need a better approach. tgl
* 2000/10/01
*
* Release 8.0 removes bigbox_ops (which was dead code for a long while
* anyway). tgl 2003/11/11
*/
if (list_length(opclass) == 1)
{
char *claname = strVal(linitial(opclass));
if (strcmp(claname, "network_ops") == 0 ||
strcmp(claname, "timespan_ops") == 0 ||
strcmp(claname, "datetime_ops") == 0 ||
strcmp(claname, "lztext_ops") == 0 ||
strcmp(claname, "timestamp_ops") == 0 ||
strcmp(claname, "bigbox_ops") == 0)
opclass = NIL;
}
At some point, the risk that this causes problems for developers of
new opclasses must outweigh the value of silently upgrading old dumps.
I think if we're zapping other pre-7.3-compatibility hacks for that
purpose, this one could go too.
Elsewhere in indexcmds.c, there's this:
/*
* Hack to provide more-or-less-transparent updating of old RTREE
* indexes to GiST: if RTREE is requested and not found, use GIST.
*/
if (strcmp(accessMethodName, "rtree") == 0)
{
ereport(NOTICE,
(errmsg("substituting access method \"gist\" for obsolete method \"rtree\"")));
accessMethodName = "gist";
tuple = SearchSysCache1(AMNAME, PointerGetDatum(accessMethodName));
}
which dates to 8.2 (2a8d3d83e of 2005-11-07). This is less bad than the
other thing, since it won't affect the behavior of any command that
wouldn't otherwise just fail; but maybe its time has passed as well?
Although Alvaro's point comparing these behaviors to pg_dump's support
cutoff of 8.0 suggests that maybe we should leave this one for now.
regards, tom lane
On 05/03/2020 16:33, Tom Lane wrote:
> Elsewhere in indexcmds.c, there's this:
>
> /*
> * Hack to provide more-or-less-transparent updating of old RTREE
> * indexes to GiST: if RTREE is requested and not found, use GIST.
> */
> if (strcmp(accessMethodName, "rtree") == 0)
> {
> ereport(NOTICE,
> (errmsg("substituting access method \"gist\" for obsolete method \"rtree\"")));
> accessMethodName = "gist";
> tuple = SearchSysCache1(AMNAME, PointerGetDatum(accessMethodName));
> }
Aww, this one is in my list of gotcha trivia questions.
That's not a reason not to remove it, of course.
--
Vik Fearing
On 2020-Mar-05, Tom Lane wrote:
> As long as we're thinking of zapping code that is long past its sell-by
> date, I propose getting rid of this stanza in indexcmds.c, which
> basically causes CREATE INDEX to ignore certain opclass specifications:
I agree, this should be fine to remove.
> Elsewhere in indexcmds.c, there's this:
>
> /*
> * Hack to provide more-or-less-transparent updating of old RTREE
> * indexes to GiST: if RTREE is requested and not found, use GIST.
> */
> if (strcmp(accessMethodName, "rtree") == 0)
> {
> ereport(NOTICE,
> (errmsg("substituting access method \"gist\" for obsolete method \"rtree\"")));
> accessMethodName = "gist";
> tuple = SearchSysCache1(AMNAME, PointerGetDatum(accessMethodName));
> }
>
> which dates to 8.2 (2a8d3d83e of 2005-11-07). This is less bad than the
> other thing, since it won't affect the behavior of any command that
> wouldn't otherwise just fail; but maybe its time has passed as well?
> Although Alvaro's point comparing these behaviors to pg_dump's support
> cutoff of 8.0 suggests that maybe we should leave this one for now.
Yeah, dunno, 'rtree' is even immortalized in tests; commit f2e403803fe6
as recently as March 2019 was seen modifying that.
(Another curious factoid is that SQLite supports something that vaguely
looks rtreeish https://sqlite.org/rtree.html -- However, because it
doesn't use the same syntax Postgres uses, it's not a point against
removing our hack.)
I guess we can wait a couple years more on that one, since it's not
damaging anything anyway.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> On 5 Mar 2020, at 19:36, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > On 2020-Mar-05, Tom Lane wrote: > >> As long as we're thinking of zapping code that is long past its sell-by >> date, I propose getting rid of this stanza in indexcmds.c, which >> basically causes CREATE INDEX to ignore certain opclass specifications: > > I agree, this should be fine to remove. The attached patchset removes this stanza as well. When poking around here I realized that defGetStringList was also left unused. It was added with the logical decoding code but the single callsite has since been removed. As it's published in a header we might not want to remove it, but I figured I'd bring it up as were talking about removing code. cheers ./daniel
Attachment
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2020-Mar-05, Tom Lane wrote:
>> As long as we're thinking of zapping code that is long past its sell-by
>> date, I propose getting rid of this stanza in indexcmds.c, which
>> basically causes CREATE INDEX to ignore certain opclass specifications:
> I agree, this should be fine to remove.
Done.
>> which dates to 8.2 (2a8d3d83e of 2005-11-07). This is less bad than the
>> other thing, since it won't affect the behavior of any command that
>> wouldn't otherwise just fail; but maybe its time has passed as well?
>> Although Alvaro's point comparing these behaviors to pg_dump's support
>> cutoff of 8.0 suggests that maybe we should leave this one for now.
> Yeah, dunno, 'rtree' is even immortalized in tests; commit f2e403803fe6
> as recently as March 2019 was seen modifying that.
Hah, I didn't realize we actually had code coverage for that!
> I guess we can wait a couple years more on that one, since it's not
> damaging anything anyway.
Agreed, I left it be.
regards, tom lane
Daniel Gustafsson <daniel@yesql.se> writes:
> Having code which is untested and not excercised by developers (or users, if my
> assumption holds), yet being reachable by SQL, runs the risk of introducing
> subtle bugs. Is there a usecase for keeping it, or can/should it be removed in
> 14? That would still leave a lot of supported versions to upgrade to in case
> there are users to need this.
Pushed. Looking at the original commit, I noticed one now-obsolete
comment that should also be removed, so I did that.
regards, tom lane
Daniel Gustafsson <daniel@yesql.se> writes:
> When poking around here I realized that defGetStringList was also left unused.
> It was added with the logical decoding code but the single callsite has since
> been removed. As it's published in a header we might not want to remove it,
> but I figured I'd bring it up as were talking about removing code.
Hm. Kind of inclined to leave it, since somebody will probably need it
again someday.
regards, tom lane
> On 5 Mar 2020, at 21:52, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Daniel Gustafsson <daniel@yesql.se> writes: >> Having code which is untested and not excercised by developers (or users, if my >> assumption holds), yet being reachable by SQL, runs the risk of introducing >> subtle bugs. Is there a usecase for keeping it, or can/should it be removed in >> 14? That would still leave a lot of supported versions to upgrade to in case >> there are users to need this. > > Pushed. Looking at the original commit, I noticed one now-obsolete > comment that should also be removed, so I did that. Thanks, I was looking around but totally missed that comment. cheers ./daniel