Thread: 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 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. > 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.
Attachment
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 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. > > > > 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 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. 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 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. > > >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
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 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'
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