[HACKERS] removing abstime, reltime, tinterval.c, spi/timetravel

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&gt;
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