Обсуждение: [HACKERS] removing abstime, reltime, tinterval.c, spi/timetravel
In-Reply-To: <20180928223240.kgwc4czzzekrpsid@alap3.anarazel.de> Hi, As discussed below (at [1]), I think we should remove $subject. I plan to do so, unless somebody protests soon-ish. I thought it'd be better to call attention to this in a new thread, to make sure people had a chance to object. Regards, Andres In "Something for the TODO list: deprecating abstime and friends" On 2018-09-28 15:32:40 -0700, Andres Freund wrote: > On 2017-12-13 00:05:06 -0800, Andres Freund wrote: > > > * contrib/spi/timetravel depends on abstime columns to represent what > > > would nowadays be better done as a tstzrange. I'd have thought we > > > could maybe toss that example module overboard, but we just today got > > > a question about its usage, so I'm afraid it's not quite dead yet. > > > What shall we do with that? > > > > Looking at the code I'd be pretty strongly inclined to scrap it. > > > > > > Before I'd discovered this thread, I'd started to write up a > > patch. Attached. It's clearly not fully done. Questions I'd while > > hacking things up: > > - what to do with contrib/spi/timetravel - I'd just removed it from the > > relevant Makefile for now. > > - nabstime.c currently implements timeofday() which imo is a pretty > > weird function. I'd be quite inclined to remove it at the same time as > > this. > > Here's a refreshed version of this patch. First patch removes > contrib/spi/timetravel, second patch removes abstime, reltime, tinterval > together with timeofday(). > > I think we should just go ahead and commit something like this soon. [1] http://archives.postgresql.org/message-id/20180928223240.kgwc4czzzekrpsid%40alap3.anarazel.de
On Tue, Oct 09, 2018 at 12:22:37PM -0700, Andres Freund wrote: > In-Reply-To: <20180928223240.kgwc4czzzekrpsid@alap3.anarazel.de> > > Hi, > > As discussed below (at [1]), I think we should remove $subject. I plan > to do so, unless somebody protests soon-ish. I thought it'd be better > to call attention to this in a new thread, to make sure people had a > chance to object. How much time would someone have to convert the timetravel piece of contrib/spi to use non-deprecated time types in order to make this window? Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Hi, On 2018-10-09 21:26:31 +0200, David Fetter wrote: > On Tue, Oct 09, 2018 at 12:22:37PM -0700, Andres Freund wrote: > > In-Reply-To: <20180928223240.kgwc4czzzekrpsid@alap3.anarazel.de> > > As discussed below (at [1]), I think we should remove $subject. I plan > > to do so, unless somebody protests soon-ish. I thought it'd be better > > to call attention to this in a new thread, to make sure people had a > > chance to object. > > How much time would someone have to convert the timetravel piece of > contrib/spi to use non-deprecated time types in order to make this > window? "this window"? It's not entirely trivial, but also not that hard. It'd break existing users however, as obviously their tables wouldn't dump / load or pg_upgrade into a working state. But I think spi/timetravel is not something people can actually use / do use much, the functionality is way too limited in practice, the datatypes have been arcane for about as long as postgres existed, etc. And the code isn't fit to serve as an example. In my opinion it has negative value at this point. Greetings, Andres Freund
On Tue, Oct 09, 2018 at 12:31:19PM -0700, Andres Freund wrote: > Hi, > > On 2018-10-09 21:26:31 +0200, David Fetter wrote: > > On Tue, Oct 09, 2018 at 12:22:37PM -0700, Andres Freund wrote: > > > In-Reply-To: <20180928223240.kgwc4czzzekrpsid@alap3.anarazel.de> > > > As discussed below (at [1]), I think we should remove $subject. I plan > > > to do so, unless somebody protests soon-ish. I thought it'd be better > > > to call attention to this in a new thread, to make sure people had a > > > chance to object. > > > > How much time would someone have to convert the timetravel piece of > > contrib/spi to use non-deprecated time types in order to make this > > window? > > "this window"? > > It's not entirely trivial, but also not that hard. It'd break existing > users however, as obviously their tables wouldn't dump / load or > pg_upgrade into a working state. > > But I think spi/timetravel is not something people can actually use / do > use much, the functionality is way too limited in practice, the > datatypes have been arcane for about as long as postgres existed, > etc. And the code isn't fit to serve as an example. > > In my opinion it has negative value at this point. I suppose the proposals to add the standard-conformant temporal stuff would make this moot, but I don't recall a complete patch for that. Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
On October 9, 2018 1:40:34 PM PDT, David Fetter <david@fetter.org> wrote: >On Tue, Oct 09, 2018 at 12:31:19PM -0700, Andres Freund wrote: >> Hi, >> >> On 2018-10-09 21:26:31 +0200, David Fetter wrote: >> > On Tue, Oct 09, 2018 at 12:22:37PM -0700, Andres Freund wrote: >> > > In-Reply-To: <20180928223240.kgwc4czzzekrpsid@alap3.anarazel.de> >> > > As discussed below (at [1]), I think we should remove $subject. >I plan >> > > to do so, unless somebody protests soon-ish. I thought it'd be >better >> > > to call attention to this in a new thread, to make sure people >had a >> > > chance to object. >> > >> > How much time would someone have to convert the timetravel piece of >> > contrib/spi to use non-deprecated time types in order to make this >> > window? >> >> "this window"? >> >> It's not entirely trivial, but also not that hard. It'd break >existing >> users however, as obviously their tables wouldn't dump / load or >> pg_upgrade into a working state. >> >> But I think spi/timetravel is not something people can actually use / >do >> use much, the functionality is way too limited in practice, the >> datatypes have been arcane for about as long as postgres existed, >> etc. And the code isn't fit to serve as an example. >> >> In my opinion it has negative value at this point. > >I suppose the proposals to add the standard-conformant temporal stuff >would make this moot, but I don't recall a complete patch for that. spi/timetravel is just a trigger. Can be written in a few lines of plpgsql. What's functionality of your concern here? Comparing it to actual temporal functionality doesn't strike me as meaningful. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Tue, Oct 09, 2018 at 01:43:48PM -0700, Andres Freund wrote: > > > On October 9, 2018 1:40:34 PM PDT, David Fetter <david@fetter.org> > wrote: > >On Tue, Oct 09, 2018 at 12:31:19PM -0700, Andres Freund wrote: > >> Hi, > >> > >> On 2018-10-09 21:26:31 +0200, David Fetter wrote: > >> > On Tue, Oct 09, 2018 at 12:22:37PM -0700, Andres Freund wrote: > >> > > In-Reply-To: > >> > > <20180928223240.kgwc4czzzekrpsid@alap3.anarazel.de> As > >> > > discussed below (at [1]), I think we should remove $subject. > >I plan > >> > > to do so, unless somebody protests soon-ish. I thought it'd > >> > > be > >better > >> > > to call attention to this in a new thread, to make sure > >> > > people > >had a > >> > > chance to object. > >> > > >> > How much time would someone have to convert the timetravel > >> > piece of contrib/spi to use non-deprecated time types in order > >> > to make this window? > >> > >> "this window"? > >> > >> It's not entirely trivial, but also not that hard. It'd break > >existing > >> users however, as obviously their tables wouldn't dump / load or > >> pg_upgrade into a working state. > >> > >> But I think spi/timetravel is not something people can actually > >> use / > >do > >> use much, the functionality is way too limited in practice, the > >> datatypes have been arcane for about as long as postgres existed, > >> etc. And the code isn't fit to serve as an example. > >> > >> In my opinion it has negative value at this point. > > > >I suppose the proposals to add the standard-conformant temporal > >stuff would make this moot, but I don't recall a complete patch for > >that. > > spi/timetravel is just a trigger. Can be written in a few lines of > plpgsql. What's functionality of your concern here? Comparing it > to actual temporal functionality doesn't strike me as meaningful. Fair enough. Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Andres Freund <andres@anarazel.de> writes:
> As discussed below (at [1]), I think we should remove $subject. I plan
> to do so, unless somebody protests soon-ish. I thought it'd be better
> to call attention to this in a new thread, to make sure people had a
> chance to object.
I complained about this already on the other thread, I think, but:
I do not think we should remove timeofday(). It's unrelated to these
datatypes and it offers functionality that isn't quite duplicated
elsewhere.
regards, tom lane
Hi, On 2018-10-09 17:27:17 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > As discussed below (at [1]), I think we should remove $subject. I plan > > to do so, unless somebody protests soon-ish. I thought it'd be better > > to call attention to this in a new thread, to make sure people had a > > chance to object. > > I complained about this already on the other thread, I think, but: > I do not think we should remove timeofday(). It's unrelated to these > datatypes and it offers functionality that isn't quite duplicated > elsewhere. Ok. I find it a somewhat weird function, but I agree it's not strongly related. It only came up because it's implemented in nabstime.c. Moving it to utils/adt/timestamp.c seems to make the most sense? Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2018-10-09 17:27:17 -0400, Tom Lane wrote:
>> I complained about this already on the other thread, I think, but:
>> I do not think we should remove timeofday(). It's unrelated to these
>> datatypes and it offers functionality that isn't quite duplicated
>> elsewhere.
> Ok. I find it a somewhat weird function, but I agree it's not strongly
> related. It only came up because it's implemented in nabstime.c. Moving
> it to utils/adt/timestamp.c seems to make the most sense?
Sure, if you want to flush nabstime.c completely, just shove it over
there.
regards, tom lane
> On Oct 9, 2018, at 12:22 PM, Andres Freund <andres@anarazel.de> wrote: > > In-Reply-To: <20180928223240.kgwc4czzzekrpsid@alap3.anarazel.de> > > Hi, > > As discussed below (at [1]), I think we should remove $subject. I plan > to do so, unless somebody protests soon-ish. I thought it'd be better > to call attention to this in a new thread, to make sure people had a > chance to object. I have no objection, but I'm curious, when retiring a datatype and associated functions, do the Oids that were assigned to them become available for new uses, or do you have to expire them to avoid breaking pg_upgrade and such? Retiring built-in types and functions seems rare enough that I've not seen how this is handled before. mark
Hi, On 2018-10-09 16:17:44 -0700, Mark Dilger wrote: > > On Oct 9, 2018, at 12:22 PM, Andres Freund <andres@anarazel.de> wrote: > > As discussed below (at [1]), I think we should remove $subject. I plan > > to do so, unless somebody protests soon-ish. I thought it'd be better > > to call attention to this in a new thread, to make sure people had a > > chance to object. > > I have no objection, but I'm curious, when retiring a datatype and > associated functions, do the Oids that were assigned to them become > available for new uses, or do you have to expire them to avoid breaking > pg_upgrade and such? Retiring built-in types and functions seems > rare enough that I've not seen how this is handled before. I don't really see a need for preserving them. pg_upgrade should fail because the schema dump won't restore (as that has textual oids). You could argue that external drivers could have the oids builtin, but I don't find that convincing, because they'd be in trouble for new types etc anyway. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2018-10-09 16:17:44 -0700, Mark Dilger wrote:
>> I have no objection, but I'm curious, when retiring a datatype and
>> associated functions, do the Oids that were assigned to them become
>> available for new uses, or do you have to expire them to avoid breaking
>> pg_upgrade and such? Retiring built-in types and functions seems
>> rare enough that I've not seen how this is handled before.
> I don't really see a need for preserving them.
Yeah, should be fine to recycle them.
regards, tom lane
Hi, On 2018-10-09 17:39:09 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2018-10-09 17:27:17 -0400, Tom Lane wrote: > >> I complained about this already on the other thread, I think, but: > >> I do not think we should remove timeofday(). It's unrelated to these > >> datatypes and it offers functionality that isn't quite duplicated > >> elsewhere. > > > Ok. I find it a somewhat weird function, but I agree it's not strongly > > related. It only came up because it's implemented in nabstime.c. Moving > > it to utils/adt/timestamp.c seems to make the most sense? > > Sure, if you want to flush nabstime.c completely, just shove it over > there. I've done that now, together with two commits for removal of timetravel and abstime, reltime, tinterval. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> I've done that now, together with two commits for removal of timetravel
> and abstime, reltime, tinterval.
Unsurprisingly-in-retrospect, buildfarm member crake is now bitching
that cross-version pg_upgrade fails, since it's trying to test importing
back-branch regression DBs that contain tables with the desupported types.
Perhaps the best fix for this is to teach the cross-version-upgrade
buildfarm module to drop the affected tables from the old DB before
testing pg_upgrade. However, that does nothing to help manual testing
of similar scenarios.
Another idea would be to put table drops into the back branch regression
tests, so that their ending states don't include any such tables. That
would cripple pg_dump testing of these types in the back branches, but
I'm not sure if we really care much.
I don't especially like either of these choices --- anyone got another
idea?
regards, tom lane
Hi, On 2018-10-11 16:57:02 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > I've done that now, together with two commits for removal of timetravel > > and abstime, reltime, tinterval. > > Unsurprisingly-in-retrospect, buildfarm member crake is now bitching > that cross-version pg_upgrade fails, since it's trying to test importing > back-branch regression DBs that contain tables with the desupported types. > > Perhaps the best fix for this is to teach the cross-version-upgrade > buildfarm module to drop the affected tables from the old DB before > testing pg_upgrade. However, that does nothing to help manual testing > of similar scenarios. > > Another idea would be to put table drops into the back branch regression > tests, so that their ending states don't include any such tables. That > would cripple pg_dump testing of these types in the back branches, but > I'm not sure if we really care much. I think the latter is the better choice. Given the code for those types hasn't changed meaningfully in the last decade, I think not having pg_dump coverage would be ok. > I don't especially like either of these choices --- anyone got another > idea? Nope :( Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2018-10-11 16:57:02 -0400, Tom Lane wrote:
>> Another idea would be to put table drops into the back branch regression
>> tests, so that their ending states don't include any such tables. That
>> would cripple pg_dump testing of these types in the back branches, but
>> I'm not sure if we really care much.
> I think the latter is the better choice. Given the code for those types
> hasn't changed meaningfully in the last decade, I think not having
> pg_dump coverage would be ok.
>> I don't especially like either of these choices --- anyone got another
>> idea?
> Nope :(
A compromise that occurred to me after a bit of reflection is to place
the necessary table-drop commands in a new regression test script that's
meant to be executed last, but isn't actually run by default. Then
teach the cross-version-update test script to include that script via
EXTRA_TESTS. Manual testing could do likewise. Then we have a small
amount of pain for testing upgrades, but we lose no test coverage in
back branches.
regards, tom lane
Hi, On 2018-10-11 17:11:47 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2018-10-11 16:57:02 -0400, Tom Lane wrote: > >> Another idea would be to put table drops into the back branch regression > >> tests, so that their ending states don't include any such tables. That > >> would cripple pg_dump testing of these types in the back branches, but > >> I'm not sure if we really care much. > > > I think the latter is the better choice. Given the code for those types > > hasn't changed meaningfully in the last decade, I think not having > > pg_dump coverage would be ok. > > >> I don't especially like either of these choices --- anyone got another > >> idea? > > > Nope :( > > A compromise that occurred to me after a bit of reflection is to place > the necessary table-drop commands in a new regression test script that's > meant to be executed last, but isn't actually run by default. Then > teach the cross-version-update test script to include that script via > EXTRA_TESTS. Manual testing could do likewise. Then we have a small > amount of pain for testing upgrades, but we lose no test coverage in > back branches. To me that seems to be more work / infrastructure than warranted. abstime/reltime/tinterval don't present pg_dump with any special challenges compared to a lot of other types we do test, no? Greetings, Andres Freund
On 10/11/2018 05:11 PM, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: >> On 2018-10-11 16:57:02 -0400, Tom Lane wrote: >>> Another idea would be to put table drops into the back branch regression >>> tests, so that their ending states don't include any such tables. That >>> would cripple pg_dump testing of these types in the back branches, but >>> I'm not sure if we really care much. >> I think the latter is the better choice. Given the code for those types >> hasn't changed meaningfully in the last decade, I think not having >> pg_dump coverage would be ok. >>> I don't especially like either of these choices --- anyone got another >>> idea? >> Nope :( > A compromise that occurred to me after a bit of reflection is to place > the necessary table-drop commands in a new regression test script that's > meant to be executed last, but isn't actually run by default. Then > teach the cross-version-update test script to include that script via > EXTRA_TESTS. Manual testing could do likewise. Then we have a small > amount of pain for testing upgrades, but we lose no test coverage in > back branches. That's not how it works. The module doesn't itself run the regression tests. It uses saved data from a normal buildfarm run against the version being upgraded from. It does, however, do some fixups along the way. See <https://github.com/PGBuildFarm/client-code/blob/master/PGBuild/Modules/TestUpgradeXversion.pm> around line 315. We could have it do something there, if the target branch was > 11. That way these things would still be tested for in an upgrade to any version that supports them. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Andres Freund <andres@anarazel.de> writes:
> On 2018-10-11 17:11:47 -0400, Tom Lane wrote:
>> A compromise that occurred to me after a bit of reflection is to place
>> the necessary table-drop commands in a new regression test script that's
>> meant to be executed last, but isn't actually run by default. Then
>> teach the cross-version-update test script to include that script via
>> EXTRA_TESTS. Manual testing could do likewise. Then we have a small
>> amount of pain for testing upgrades, but we lose no test coverage in
>> back branches.
> To me that seems to be more work / infrastructure than
> warranted. abstime/reltime/tinterval don't present pg_dump with any
> special challenges compared to a lot of other types we do test, no?
Well, in any case I'd say we should put the dropping commands into
a separate late-stage test script. Whether that's run by default is a
secondary issue: if it is, somebody who wanted to test this stuff could
remove the script from their test schedule file.
regards, tom lane
On 10/12/2018 10:03 AM, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: >> On 2018-10-11 17:11:47 -0400, Tom Lane wrote: >>> A compromise that occurred to me after a bit of reflection is to place >>> the necessary table-drop commands in a new regression test script that's >>> meant to be executed last, but isn't actually run by default. Then >>> teach the cross-version-update test script to include that script via >>> EXTRA_TESTS. Manual testing could do likewise. Then we have a small >>> amount of pain for testing upgrades, but we lose no test coverage in >>> back branches. >> To me that seems to be more work / infrastructure than >> warranted. abstime/reltime/tinterval don't present pg_dump with any >> special challenges compared to a lot of other types we do test, no? > Well, in any case I'd say we should put the dropping commands into > a separate late-stage test script. Whether that's run by default is a > secondary issue: if it is, somebody who wanted to test this stuff could > remove the script from their test schedule file. > > Anything that runs at the time we do the regression tests has problems, from my POV. If we run the drop commands then upgrading these types to a target <= 11 isn't tested. If we don't run them then upgrade to a version > 11 will fail. This is why I suggested doing it later in the buildfarm module that actaally does cross version upgrade testing. It can drop or not drop prior to running the upgrade test, depending on the target version. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> On 10/12/2018 10:03 AM, Tom Lane wrote:
>> Well, in any case I'd say we should put the dropping commands into
>> a separate late-stage test script. Whether that's run by default is a
>> secondary issue: if it is, somebody who wanted to test this stuff could
>> remove the script from their test schedule file.
> Anything that runs at the time we do the regression tests has problems,
> from my POV. If we run the drop commands then upgrading these types to a
> target <= 11 isn't tested. If we don't run them then upgrade to a
> version > 11 will fail. This is why I suggested doing it later in the
> buildfarm module that actaally does cross version upgrade testing. It
> can drop or not drop prior to running the upgrade test, depending on the
> target version.
I'd like a solution that isn't impossibly difficult for manual testing
of cross-version cases, too. That's why I'd like there to be a regression
test script that does the necessary drops. Exactly when and how that
gets invoked is certainly open for discussion. In the manual case
I'd imagine calling it with EXTRA_TESTS while running the setup of
the source database, if it's not run by default. Maybe the buildfarm
module could just invoke the same script directly at a suitable point?
regards, tom lane
Hi, On 2018-10-12 11:23:30 -0400, Andrew Dunstan wrote: > Anything that runs at the time we do the regression tests has problems, from > my POV. If we run the drop commands then upgrading these types to a target > <= 11 isn't tested. I'm asking again, what exactly do we test by having these types in the dump? They're bog standard types, there's nothing new for pg_dump to test with them. No weird typmod rules, no weird parse-time type mapping, nothing? Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> I'm asking again, what exactly do we test by having these types in the
> dump? They're bog standard types, there's nothing new for pg_dump to
> test with them. No weird typmod rules, no weird parse-time type mapping,
> nothing?
That's a pretty fair point. The types' I/O functions will be exercised
well enough by the regression tests themselves, and it's hard to see what
more test coverage is gained by including them in pg_dump/pg_upgrade
testing. Maybe we should just drop those tables and be done with it.
regards, tom lane
On 10/12/2018 12:20 PM, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: >> I'm asking again, what exactly do we test by having these types in the >> dump? They're bog standard types, there's nothing new for pg_dump to >> test with them. No weird typmod rules, no weird parse-time type mapping, >> nothing? > That's a pretty fair point. The types' I/O functions will be exercised > well enough by the regression tests themselves, and it's hard to see what > more test coverage is gained by including them in pg_dump/pg_upgrade > testing. Maybe we should just drop those tables and be done with it. > If you're happy with that then go for it. It will be less work for me ;-) cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> On 10/12/2018 12:20 PM, Tom Lane wrote:
>> That's a pretty fair point. The types' I/O functions will be exercised
>> well enough by the regression tests themselves, and it's hard to see what
>> more test coverage is gained by including them in pg_dump/pg_upgrade
>> testing. Maybe we should just drop those tables and be done with it.
> If you're happy with that then go for it. It will be less work for me ;-)
Done.
regards, tom lane
BTW, I was annoyed while looking things over that this patch had broken
a couple of comments in opr_sanity.sql:
@@ -823,7 +823,6 @@ WHERE a.aggfnoid = p.oid AND
-- Cross-check transfn against its entry in pg_proc.
-- NOTE: use physically_coercible here, not binary_coercible, because
--- max and min on abstime are implemented using int4larger/int4smaller.
SELECT a.aggfnoid::oid, p.proname, ptr.oid, ptr.proname
FROM pg_aggregate AS a, pg_proc AS p, pg_proc AS ptr
WHERE a.aggfnoid = p.oid AND
@@ -978,7 +977,6 @@ WHERE a.aggfnoid = p.oid AND
-- Check that all combine functions have signature
-- combine(transtype, transtype) returns transtype
-- NOTE: use physically_coercible here, not binary_coercible, because
--- max and min on abstime are implemented using int4larger/int4smaller.
SELECT a.aggfnoid, p.proname
FROM pg_aggregate as a, pg_proc as p
Just removing a fraction of the sentence is not good.
So I went looking for a different example to plug in there, and soon
found that there weren't any. If you change all the physically_coercible
calls in that script to binary_coercible, its output doesn't change.
I'm thinking that we ought to do that, and just get rid of
physically_coercible(), so that we have a tighter, more semantically
meaningful set of checks here. We can always undo that if we ever
have occasion to type-cheat like that again, but offhand I'm not sure
why we would do so.
regards, tom lane
Hi, On 2018-10-12 19:47:40 -0400, Tom Lane wrote: > BTW, I was annoyed while looking things over that this patch had broken > a couple of comments in opr_sanity.sql: > > @@ -823,7 +823,6 @@ WHERE a.aggfnoid = p.oid AND > > -- Cross-check transfn against its entry in pg_proc. > -- NOTE: use physically_coercible here, not binary_coercible, because > --- max and min on abstime are implemented using int4larger/int4smaller. > SELECT a.aggfnoid::oid, p.proname, ptr.oid, ptr.proname > FROM pg_aggregate AS a, pg_proc AS p, pg_proc AS ptr > WHERE a.aggfnoid = p.oid AND > @@ -978,7 +977,6 @@ WHERE a.aggfnoid = p.oid AND > -- Check that all combine functions have signature > -- combine(transtype, transtype) returns transtype > -- NOTE: use physically_coercible here, not binary_coercible, because > --- max and min on abstime are implemented using int4larger/int4smaller. > > SELECT a.aggfnoid, p.proname > FROM pg_aggregate as a, pg_proc as p > > Just removing a fraction of the sentence is not good. Fair. > So I went looking for a different example to plug in there, and soon > found that there weren't any. If you change all the physically_coercible > calls in that script to binary_coercible, its output doesn't change. > I'm thinking that we ought to do that, and just get rid of > physically_coercible(), so that we have a tighter, more semantically > meaningful set of checks here. We can always undo that if we ever > have occasion to type-cheat like that again, but offhand I'm not sure > why we would do so. Hm, I wonder if it's not a good idea to leave the test there, or rewrite it slightly, so we have a a more precise warning about cheats like that? I also don't really see why we'd introduce new hacks like reusing functions like that, but somebody might do it while hacking something together and forget about it. Probably still not worth it? Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2018-10-12 19:47:40 -0400, Tom Lane wrote:
>> So I went looking for a different example to plug in there, and soon
>> found that there weren't any. If you change all the physically_coercible
>> calls in that script to binary_coercible, its output doesn't change.
>> I'm thinking that we ought to do that, and just get rid of
>> physically_coercible(), so that we have a tighter, more semantically
>> meaningful set of checks here. We can always undo that if we ever
>> have occasion to type-cheat like that again, but offhand I'm not sure
>> why we would do so.
> Hm, I wonder if it's not a good idea to leave the test there, or rewrite
> it slightly, so we have a a more precise warning about cheats like that?
After thinking about this for awhile, I decided that
physically_coercible() is poorly named, because it suggests that it
might for instance allow any 4-byte type to be cast to any other one.
Actually the additional cases it allows are just ones where an explicit
binary-coercion cast would be needed. So I still think we should
tighten up the tests while we can, but I left that function in place
with a different name and a better comment.
regards, tom lane