change in behaviour for format_type() call

Started by Rushabh Lathiaalmost 8 years ago6 messages
#1Rushabh Lathia
rushabh.lathia@gmail.com
1 attachment(s)

Commit a26116c6cbf4117e8efaa7cfc5bacc887f01517f changed the behaviour
for format_type.

Prior to commit, format_type() used to set typemod_given to false for
typemode = NULL.

format_type()
..
if (PG_ARGISNULL(1))
result = format_type_internal(type_oid, -1, false, true, false);
else
{
typemod = PG_GETARG_INT32(1);
result = format_type_internal(type_oid, typemod, true, true, false);
}
..

With the commit format_type() always set the FORMAT_TYPE_TYPEMOD_GIVEN
flag even when typemode is NULL (-1).

Below are the difference it's making to the query output:

Before commit:

postgres@95320=#select format_type('bpchar'::regtype, null);
format_type
-------------
character
(1 row)

postgres@95320=#select format_type('bit'::regtype, null);
format_type
-------------
bit
(1 row)

After commit:

postgres@90169=#select format_type('bpchar'::regtype, null);
format_type
-------------
bpchar
(1 row)

postgres@90169=#select format_type('bit'::regtype, null);
format_type
-------------
"bit"
(1 row)

Is this expected behaviour? attaching patch to get back the older
behaviour.

Thanks,

Regards,
Rushabh Lathia
www.EnterpriseDB.com

Attachments:

format_type_fix.patchtext/x-patch; charset=US-ASCII; name=format_type_fix.patchDownload
diff --git a/src/backend/utils/adt/format_type.c b/src/backend/utils/adt/format_type.c
index 872574f..86ef127 100644
--- a/src/backend/utils/adt/format_type.c
+++ b/src/backend/utils/adt/format_type.c
@@ -63,6 +63,7 @@ format_type(PG_FUNCTION_ARGS)
 	Oid			type_oid;
 	int32		typemod;
 	char	   *result;
+	bits16		flags = FORMAT_TYPE_ALLOW_INVALID;
 
 	/* Since this function is not strict, we must test for null args */
 	if (PG_ARGISNULL(0))
@@ -71,9 +72,10 @@ format_type(PG_FUNCTION_ARGS)
 	type_oid = PG_GETARG_OID(0);
 	typemod = PG_ARGISNULL(1) ? -1 : PG_GETARG_INT32(1);
 
-	result = format_type_extended(type_oid, typemod,
-								  FORMAT_TYPE_TYPEMOD_GIVEN |
-								  FORMAT_TYPE_ALLOW_INVALID);
+	if (typemod != -1)
+		flags |= FORMAT_TYPE_TYPEMOD_GIVEN;
+
+	result = format_type_extended(type_oid, typemod, flags);
 
 	PG_RETURN_TEXT_P(cstring_to_text(result));
 }
#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Rushabh Lathia (#1)
Re: change in behaviour for format_type() call

Rushabh Lathia <rushabh.lathia@gmail.com> writes:

Commit a26116c6cbf4117e8efaa7cfc5bacc887f01517f changed the behaviour
for format_type.
...
Is this expected behaviour? attaching patch to get back the older
behaviour.

I don't see anything in the commit message or linked discussion to
indicate that any visible behavior change was intended, so I think
you're right, this is a bug. Will check and push your patch.

regards, tom lane

#3Tom Lane
tgl@sss.pgh.pa.us
In reply to: Tom Lane (#2)
Re: change in behaviour for format_type() call

I wrote:

I don't see anything in the commit message or linked discussion to
indicate that any visible behavior change was intended, so I think
you're right, this is a bug. Will check and push your patch.

Actually, this patch still wasn't quite right: although it fixed
one aspect of the behavior, it still produced identical results
for typemod NULL and typemod -1, which as the function's comment
explains is not what should happen. I tweaked the logic to look
as much as possible like before, and added a regression test.

regards, tom lane

#4Alvaro Herrera
alvherre@alvh.no-ip.org
In reply to: Tom Lane (#3)
Re: change in behaviour for format_type() call

Tom Lane wrote:

I wrote:

I don't see anything in the commit message or linked discussion to
indicate that any visible behavior change was intended, so I think
you're right, this is a bug. Will check and push your patch.

Actually, this patch still wasn't quite right: although it fixed
one aspect of the behavior, it still produced identical results
for typemod NULL and typemod -1, which as the function's comment
explains is not what should happen. I tweaked the logic to look
as much as possible like before, and added a regression test.

Thanks!

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#5Michael Paquier
michael@paquier.xyz
In reply to: Alvaro Herrera (#4)
Re: change in behaviour for format_type() call

On Thu, Mar 01, 2018 at 02:54:22PM -0300, Alvaro Herrera wrote:

Tom Lane wrote:

Actually, this patch still wasn't quite right: although it fixed
one aspect of the behavior, it still produced identical results
for typemod NULL and typemod -1, which as the function's comment
explains is not what should happen. I tweaked the logic to look
as much as possible like before, and added a regression test.

Thanks!

Thanks Tom. Yes, we focused on not changing any existing behavior with
this patch. So this report was a bug.
--
Michael

#6Rushabh Lathia
rushabh.lathia@gmail.com
In reply to: Tom Lane (#3)
Re: change in behaviour for format_type() call

On Thu, Mar 1, 2018 at 10:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wrote:

I don't see anything in the commit message or linked discussion to
indicate that any visible behavior change was intended, so I think
you're right, this is a bug. Will check and push your patch.

Actually, this patch still wasn't quite right: although it fixed
one aspect of the behavior, it still produced identical results
for typemod NULL and typemod -1, which as the function's comment
explains is not what should happen. I tweaked the logic to look
as much as possible like before, and added a regression test.

Thanks Tom.

Regards,
Rushabh Lathia
www.EnterpriseDB.com