Retiring support for pre-7.3 FK constraint triggers

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 from committers 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 from committers 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

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