BUG #16939: Plural interval for negative singular
The following bug has been logged on the website:
Bug reference: 16939
Logged by: Max Neverov
Email address: neverov.max@gmail.com
PostgreSQL version: 13.2
Operating system: alpine
Description:
Execute the query:
postgres=# set intervalstyle='postgres';
SET
postgres=# select interval '-1 year -1 day';
interval
------------------
-1 years -1 days
(1 row)
Expected output:
-1 year -1 day
The code
(https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/datetime.c#L4193)
pluralizes a time unit if the value is not 1, should check both -1 and 1.
On Tue, Mar 23, 2021 at 05:21:30AM +0000, PG Bug reporting form wrote:
The following bug has been logged on the website:
Bug reference: 16939
Logged by: Max Neverov
Email address: neverov.max@gmail.com
PostgreSQL version: 13.2
Operating system: alpine
Description:Execute the query:
postgres=# set intervalstyle='postgres';
SET
postgres=# select interval '-1 year -1 day';
interval
------------------
-1 years -1 days
(1 row)Expected output:
-1 year -1 dayThe code
(https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/datetime.c#L4193)
pluralizes a time unit if the value is not 1, should check both -1 and 1.
Wow, interesting. Seems there were a few places in the code that
handled this properly, and several that didn't. The attached patch
fixes these and updates the regression tests. I will apply this for PG
14 in the next few days. This is too likely to break something to
backpatch. Thanks for the report.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
If only the physical world exists, free will is an illusion.
Attachments:
plural.difftext/x-diff; charset=us-asciiDownload+17-15
On Tue, Mar 23, 2021 at 12:03:36PM -0400, Bruce Momjian wrote:
On Tue, Mar 23, 2021 at 05:21:30AM +0000, PG Bug reporting form wrote:
The following bug has been logged on the website:
Bug reference: 16939
Logged by: Max Neverov
Email address: neverov.max@gmail.com
PostgreSQL version: 13.2
Operating system: alpine
Description:Execute the query:
postgres=# set intervalstyle='postgres';
SET
postgres=# select interval '-1 year -1 day';
interval
------------------
-1 years -1 days
(1 row)Expected output:
-1 year -1 dayThe code
(https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/datetime.c#L4193)
pluralizes a time unit if the value is not 1, should check both -1 and 1.Wow, interesting. Seems there were a few places in the code that
handled this properly, and several that didn't. The attached patch
fixes these and updates the regression tests. I will apply this for PG
14 in the next few days. This is too likely to break something to
backpatch. Thanks for the report.
Patch applied and will appear in PG 14. Thanks for the report.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
If only the physical world exists, free will is an illusion.
On Tue, Mar 23, 2021 at 12:03:36PM -0400, Bruce Momjian wrote:
On Tue, Mar 23, 2021 at 05:21:30AM +0000, PG Bug reporting form wrote:
postgres=# set intervalstyle='postgres';
SET
postgres=# select interval '-1 year -1 day';
interval
------------------
-1 years -1 days
(1 row)Expected output:
-1 year -1 dayThe code
(https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/datetime.c#L4193)
pluralizes a time unit if the value is not 1, should check both -1 and 1.
From the first several links at
https://www.google.com/search?q=negative+one+singular+plural the choice is a
matter of dispute among English grammar commentators. Singular "-1" is
popular, and plural "-1" is also popular.
Wow, interesting. Seems there were a few places in the code that
handled this properly, and several that didn't. The attached patch
fixes these and updates the regression tests. I will apply this for PG
14 in the next few days. This is too likely to break something to
backpatch. Thanks for the report.
Let's not change from one popular spelling to another when doing so creates a
compatibility hazard. That is to say, I think PostgreSQL would be better with
this patch reverted.
If we did want to standardize on singular for -1, EVALUATE_MESSAGE_PLURAL()
would be a key bit of code to change.
On Sun, Apr 25, 2021 at 04:57:26AM -0700, Noah Misch wrote:
On Tue, Mar 23, 2021 at 12:03:36PM -0400, Bruce Momjian wrote:
On Tue, Mar 23, 2021 at 05:21:30AM +0000, PG Bug reporting form wrote:
postgres=# set intervalstyle='postgres';
SET
postgres=# select interval '-1 year -1 day';
interval
------------------
-1 years -1 days
(1 row)Expected output:
-1 year -1 dayThe code
(https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/datetime.c#L4193)
pluralizes a time unit if the value is not 1, should check both -1 and 1.From the first several links at
https://www.google.com/search?q=negative+one+singular+plural the choice is a
matter of dispute among English grammar commentators. Singular "-1" is
popular, and plural "-1" is also popular.Wow, interesting. Seems there were a few places in the code that
handled this properly, and several that didn't. The attached patch
fixes these and updates the regression tests. I will apply this for PG
14 in the next few days. This is too likely to break something to
backpatch. Thanks for the report.Let's not change from one popular spelling to another when doing so creates a
compatibility hazard. That is to say, I think PostgreSQL would be better with
this patch reverted.If we did want to standardize on singular for -1, EVALUATE_MESSAGE_PLURAL()
would be a key bit of code to change.
Oh, good point. I think we should just pick one and be consistent --- I
don't care which we choose.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
If only the physical world exists, free will is an illusion.
Bruce Momjian <bruce@momjian.us> writes:
On Sun, Apr 25, 2021 at 04:57:26AM -0700, Noah Misch wrote:
Let's not change from one popular spelling to another when doing so creates a
compatibility hazard. That is to say, I think PostgreSQL would be better with
this patch reverted.If we did want to standardize on singular for -1, EVALUATE_MESSAGE_PLURAL()
would be a key bit of code to change.
Oh, good point. I think we should just pick one and be consistent --- I
don't care which we choose.
I agree with Noah's opinion that we should stick to the historical
behavior in the interval I/O functions. There is not enough solidity
in the "this is grammatically wrong" argument to justify taking any
risk of application breakage, and it seems like there is some risk of
that there.
For the sorts of human-readable messages that EVALUATE_MESSAGE_PLURAL
tends to be used for, I don't think there's a reason to worry that
we might break applications if we change it. So I don't have a
strong opinion about what to do there. Still, by the same token
that the grammatical argument is weak, I lean towards not spending
effort on changing it.
regards, tom lane
On Mon, Apr 26, 2021 at 12:45:34PM -0400, Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
On Sun, Apr 25, 2021 at 04:57:26AM -0700, Noah Misch wrote:
Let's not change from one popular spelling to another when doing so creates a
compatibility hazard. That is to say, I think PostgreSQL would be better with
this patch reverted.If we did want to standardize on singular for -1, EVALUATE_MESSAGE_PLURAL()
would be a key bit of code to change.Oh, good point. I think we should just pick one and be consistent --- I
don't care which we choose.I agree with Noah's opinion that we should stick to the historical
behavior in the interval I/O functions. There is not enough solidity
in the "this is grammatically wrong" argument to justify taking any
risk of application breakage, and it seems like there is some risk of
that there.For the sorts of human-readable messages that EVALUATE_MESSAGE_PLURAL
tends to be used for, I don't think there's a reason to worry that
we might break applications if we change it. So I don't have a
strong opinion about what to do there. Still, by the same token
that the grammatical argument is weak, I lean towards not spending
effort on changing it.
Are you saying we should revert the patch and leave the plurals
inconsistent in different places? We were already expressing -1 as
singular in most of the places I saw.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
If only the physical world exists, free will is an illusion.
Bruce Momjian <bruce@momjian.us> writes:
On Mon, Apr 26, 2021 at 12:45:34PM -0400, Tom Lane wrote:
I agree with Noah's opinion that we should stick to the historical
behavior in the interval I/O functions. There is not enough solidity
in the "this is grammatically wrong" argument to justify taking any
risk of application breakage, and it seems like there is some risk of
that there.
Are you saying we should revert the patch and leave the plurals
inconsistent in different places?
As far as the changes in datetime.c and interval.c are concerned,
yes. I don't care too much about what you did in fe-print.c,
although TBH that case should be unreachable shouldn't it?
When would PQntuples() return -1?
(I shy gently away from the fact that that fe-print.c code is
relentlessly untranslatable.)
regards, tom lane
On Mon, Apr 26, 2021 at 01:02:44PM -0400, Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
On Mon, Apr 26, 2021 at 12:45:34PM -0400, Tom Lane wrote:
I agree with Noah's opinion that we should stick to the historical
behavior in the interval I/O functions. There is not enough solidity
in the "this is grammatically wrong" argument to justify taking any
risk of application breakage, and it seems like there is some risk of
that there.Are you saying we should revert the patch and leave the plurals
inconsistent in different places?As far as the changes in datetime.c and interval.c are concerned,
yes. I don't care too much about what you did in fe-print.c,
Well, we should then add a comment that this is inconsistent but should
not be changed.
although TBH that case should be unreachable shouldn't it?
When would PQntuples() return -1?
I changed that just for consistency with other calls, in case it is ever
copied somewhere else.
(I shy gently away from the fact that that fe-print.c code is
relentlessly untranslatable.)
Again, if we revert, we should document is via a C comment.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
If only the physical world exists, free will is an illusion.
On Mon, Apr 26, 2021 at 01:06:16PM -0400, Bruce Momjian wrote:
On Mon, Apr 26, 2021 at 01:02:44PM -0400, Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
On Mon, Apr 26, 2021 at 12:45:34PM -0400, Tom Lane wrote:
I agree with Noah's opinion that we should stick to the historical
behavior in the interval I/O functions. There is not enough solidity
in the "this is grammatically wrong" argument to justify taking any
risk of application breakage, and it seems like there is some risk of
that there.Are you saying we should revert the patch and leave the plurals
inconsistent in different places? We were already expressing -1 as
singular in most of the places I saw.
Which places did you find? I used the following to check for code like you
were adding, but it probably missed things:
$ git grep '"s"' 5da9868^ | grep abs
5da9868^:src/backend/utils/adt/datetime.c: (abs(sec) != 1 || fsec != 0) ? "s" : "");
5da9868^:src/interfaces/ecpg/pgtypeslib/interval.c: (abs(sec) != 1 || fsec != 0) ? "s" : "");
As far as the changes in datetime.c and interval.c are concerned,
yes. I don't care too much about what you did in fe-print.c,Well, we should then add a comment that this is inconsistent but should
not be changed.
No objection to having a comment.
On Mon, Apr 26, 2021 at 11:13:29PM -0700, Noah Misch wrote:
On Mon, Apr 26, 2021 at 01:06:16PM -0400, Bruce Momjian wrote:
On Mon, Apr 26, 2021 at 01:02:44PM -0400, Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
On Mon, Apr 26, 2021 at 12:45:34PM -0400, Tom Lane wrote:
I agree with Noah's opinion that we should stick to the historical
behavior in the interval I/O functions. There is not enough solidity
in the "this is grammatically wrong" argument to justify taking any
risk of application breakage, and it seems like there is some risk of
that there.Are you saying we should revert the patch and leave the plurals
inconsistent in different places? We were already expressing -1 as
singular in most of the places I saw.Which places did you find? I used the following to check for code like you
were adding, but it probably missed things:$ git grep '"s"' 5da9868^ | grep abs
5da9868^:src/backend/utils/adt/datetime.c: (abs(sec) != 1 || fsec != 0) ? "s" : "");
5da9868^:src/interfaces/ecpg/pgtypeslib/interval.c: (abs(sec) != 1 || fsec != 0) ? "s" : "");
Yes, I found the same like you did. Are you saying we should remove the
abs() calls here so we are consistent?
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
If only the physical world exists, free will is an illusion.
On 2021-Apr-26, Tom Lane wrote:
(I shy gently away from the fact that that fe-print.c code is
relentlessly untranslatable.)
It's also unused. Maybe we should consider removing instead ...
--
�lvaro Herrera Valdivia, Chile
"I can see support will not be a problem. 10 out of 10." (Simon Wittber)
(http://archives.postgresql.org/pgsql-general/2004-12/msg00159.php)
On 28/04/21 10:50 am, Alvaro Herrera wrote:
On 2021-Apr-26, Tom Lane wrote:
(I shy gently away from the fact that that fe-print.c code is
relentlessly untranslatable.)It's also unused. Maybe we should consider removing instead ...
fe-print.c is in the source tree for LibreOffice 7.2, when I downloaded git master:
... git-libreoffice/workdir/UnpackedTarball/postgresql/src/interfaces/libpq/fe-print.c'
On 2021-Apr-28, Gavin Flower wrote:
On 28/04/21 10:50 am, Alvaro Herrera wrote:
On 2021-Apr-26, Tom Lane wrote:
(I shy gently away from the fact that that fe-print.c code is
relentlessly untranslatable.)It's also unused. Maybe we should consider removing instead ...
fe-print.c is in the source tree for LibreOffice 7.2, when I downloaded git master:
... git-libreoffice/workdir/UnpackedTarball/postgresql/src/interfaces/libpq/fe-print.c'
I bet that's because they import the whole src/interfaces/libpq, but
don't actually use the functions provided by that file.
... updates local clone ...
Nope -- grepping for PQprint or PQdisplayTup doesn't yield anything in
the libreoffice git repo.
--
�lvaro Herrera 39�49'30"S 73�17'W
"Once again, thank you and all of the developers for your hard work on
PostgreSQL. This is by far the most pleasant management experience of
any database I've worked on." (Dan Harris)
http://archives.postgresql.org/pgsql-performance/2006-04/msg00247.php
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
Nope -- grepping for PQprint or PQdisplayTup doesn't yield anything in
the libreoffice git repo.
Hm. We actually document PQprint. While the other two entry points
in that file aren't documented, they are listed in exports.txt,
which means somebody *could* be calling them. I think we'd at least
have to go through a long deprecation period before we could consider
removing them.
There are also questions of whether we can remove entry points
without drawing the wrath of ABI checkers. I suppose we could
dodge that by reducing them to code that returns an error.
regards, tom lane
I wrote:
Hm. We actually document PQprint. While the other two entry points
in that file aren't documented, they are listed in exports.txt,
which means somebody *could* be calling them. I think we'd at least
have to go through a long deprecation period before we could consider
removing them.
Meanwhile, returning to the point at hand in this thread: AFAICS
from a quick review of the git logs, the last intentional changes
of user-visible behavior in fe-print.c were more than 20 years ago.
Since then it's only seen in-passing bug fixes, such as libpq-wide
fixes of signal handling.
So, to the extent that this code has any use at all it's for
legacy applications. I don't think we should be making random
changes in its behavior for at-best-weak reasons.
In short, I'm now a vote to revert every single bit of 5da9868ed.
regards, tom lane
On Tue, Apr 27, 2021 at 04:10:19PM -0400, Bruce Momjian wrote:
On Mon, Apr 26, 2021 at 11:13:29PM -0700, Noah Misch wrote:
On Mon, Apr 26, 2021 at 01:06:16PM -0400, Bruce Momjian wrote:
On Mon, Apr 26, 2021 at 01:02:44PM -0400, Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
On Mon, Apr 26, 2021 at 12:45:34PM -0400, Tom Lane wrote:
I agree with Noah's opinion that we should stick to the historical
behavior in the interval I/O functions. There is not enough solidity
in the "this is grammatically wrong" argument to justify taking any
risk of application breakage, and it seems like there is some risk of
that there.Are you saying we should revert the patch and leave the plurals
inconsistent in different places? We were already expressing -1 as
singular in most of the places I saw.Which places did you find? I used the following to check for code like you
were adding, but it probably missed things:$ git grep '"s"' 5da9868^ | grep abs
5da9868^:src/backend/utils/adt/datetime.c: (abs(sec) != 1 || fsec != 0) ? "s" : "");
5da9868^:src/interfaces/ecpg/pgtypeslib/interval.c: (abs(sec) != 1 || fsec != 0) ? "s" : "");Yes, I found the same like you did. Are you saying we should remove the
abs() calls here so we are consistent?
No, I advise just reverting 5da9868. The output funcs should keep doing what
they did in v13, inconsistency and all.
I was asking the above question to see which of singular or plural was more
common before $SUBJECT's commit. I count two examples of singular and seven
examples of plural (one in EVALUATE_MESSAGE_PLURAL(), six modified in commit
5da9868). Given that, ...
On Mon, Apr 26, 2021 at 01:06:16PM -0400, Bruce Momjian wrote:
Well, we should then add a comment that this is inconsistent but should
not be changed.
... adding comments to the two places that use abs() to achieve singular "-1"
sounds fine.
On Tue, Apr 27, 2021 at 10:44:15PM -0700, Noah Misch wrote:
On Tue, Apr 27, 2021 at 04:10:19PM -0400, Bruce Momjian wrote:
Yes, I found the same like you did. Are you saying we should remove the
abs() calls here so we are consistent?No, I advise just reverting 5da9868. The output funcs should keep doing what
they did in v13, inconsistency and all.I was asking the above question to see which of singular or plural was more
common before $SUBJECT's commit. I count two examples of singular and seven
examples of plural (one in EVALUATE_MESSAGE_PLURAL(), six modified in commit
5da9868). Given that, ...On Mon, Apr 26, 2021 at 01:06:16PM -0400, Bruce Momjian wrote:
Well, we should then add a comment that this is inconsistent but should
not be changed.... adding comments to the two places that use abs() to achieve singular "-1"
sounds fine.
OK, first, I never realized that saying -1 plural was valid. Second,
the two places you showed are actually using abs() properly since
negative values are shown with an "ago" suffix, not a negative sign:
SET intervalstyle = postgres_verbose;
SELECT '2020-01-01 01:03:00'::timestamptz - '2020-01-01 02:02:01'::timestamptz;
?column?
---------------------
@ 59 mins 1 sec ago
(1 row)
Attached is a patch which reverts 9ee7d533da and 5da9868ed9, and adds a
comment about the "ago" usage. The good news is that we were already
consistent in using plural -1. :-)
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
If only the physical world exists, free will is an illusion.
Attachments:
negative.difftext/x-diff; charset=us-asciiDownload+22-16
On Thu, Apr 29, 2021 at 09:00:04PM -0400, Bruce Momjian wrote:
On Tue, Apr 27, 2021 at 10:44:15PM -0700, Noah Misch wrote:
On Tue, Apr 27, 2021 at 04:10:19PM -0400, Bruce Momjian wrote:
Yes, I found the same like you did. Are you saying we should remove the
abs() calls here so we are consistent?No, I advise just reverting 5da9868. The output funcs should keep doing what
they did in v13, inconsistency and all.I was asking the above question to see which of singular or plural was more
common before $SUBJECT's commit. I count two examples of singular and seven
examples of plural (one in EVALUATE_MESSAGE_PLURAL(), six modified in commit
5da9868). Given that, ...On Mon, Apr 26, 2021 at 01:06:16PM -0400, Bruce Momjian wrote:
Well, we should then add a comment that this is inconsistent but should
not be changed.... adding comments to the two places that use abs() to achieve singular "-1"
sounds fine.OK, first, I never realized that saying -1 plural was valid. Second,
the two places you showed are actually using abs() properly since
negative values are shown with an "ago" suffix, not a negative sign:SET intervalstyle = postgres_verbose;
SELECT '2020-01-01 01:03:00'::timestamptz - '2020-01-01 02:02:01'::timestamptz;
?column?
---------------------
@ 59 mins 1 sec ago
(1 row)
Nice. The legendary consistency of PostgreSQL remains intact.
Attached is a patch which reverts 9ee7d533da and 5da9868ed9, and adds a
comment about the "ago" usage.
Looks good.
On Thu, Apr 29, 2021 at 09:00:04PM -0400, Bruce Momjian wrote:
OK, first, I never realized that saying -1 plural was valid. Second,
the two places you showed are actually using abs() properly since
negative values are shown with an "ago" suffix, not a negative sign:SET intervalstyle = postgres_verbose;
SELECT '2020-01-01 01:03:00'::timestamptz - '2020-01-01 02:02:01'::timestamptz;
?column?
---------------------
@ 59 mins 1 sec ago
(1 row)Attached is a patch which reverts 9ee7d533da and 5da9868ed9, and adds a
comment about the "ago" usage. The good news is that we were already
consistent in using plural -1. :-)
Here is a clearer example:
SET intervalstyle = postgres_verbose;
SELECT '2020-01-01 00:00:01'::timestamptz - '2020-01-01 00:00:00'::timestamptz;
?column?
----------
@ 1 sec
SELECT '2020-01-01 00:00:00'::timestamptz - '2020-01-01 00:00:01'::timestamptz;
?column?
-------------
@ 1 sec ago
Patch applied. Thanks for the research.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
If only the physical world exists, free will is an illusion.