Re: BUG #18230: Redundant comparison of a local variable 'tzp' address with a NULL value at dt_common.c

The following bug has been logged on the website:

Bug reference: 18230
Logged by: RekGRpth
Email address: rekgrpth(at)gmail(dot)com
PostgreSQL version: 16.1
Operating system: all
Description:

All versions of PostgreSQL has redundant comparison of a local variable
'tzp' address with a NULL value at dt_common.c in DecodeDateTime function.

```c
...
int t = 0;
int *tzp = &t;
...
if (tzp != NULL)
...

if (tzp == NULL)
...
```

On Wed, 2023-12-06 at 04:49 +0000, PG Bug reporting form wrote:
> The following bug has been logged on the website:
>
> Bug reference:      18230
> Logged by:          RekGRpth
> Email address:      rekgrpth(at)gmail(dot)com
> PostgreSQL version: 16.1
> Operating system:   all
> Description:       
>
> All versions of PostgreSQL has redundant comparison of a local variable
> 'tzp' address with a NULL value at dt_common.c in DecodeDateTime function.
>
> ```c
> ...
> int t = 0;
> int    *tzp = &t;
> ...
> if (tzp != NULL)
> ...
>
> if (tzp == NULL)

That's not really a bug, but should certainly be improved.

At fault is commit 635a0b9a864, which removed "tzp" as a parameter from
"DecodeDateTime" and replaced it with a constant pointer to 0, when it
should have done more, like remove the variable and its uses.

Do you want to write a patch?

Yours,
Laurenz Albe

something like?

ср, 6 дек. 2023 г. в 18:55, Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at>:
>
> On Wed, 2023-12-06 at 04:49 +0000, PG Bug reporting form wrote:
> > The following bug has been logged on the website:
> >
> > Bug reference: 18230
> > Logged by: RekGRpth
> > Email address: rekgrpth(at)gmail(dot)com
> > PostgreSQL version: 16.1
> > Operating system: all
> > Description:
> >
> > All versions of PostgreSQL has redundant comparison of a local variable
> > 'tzp' address with a NULL value at dt_common.c in DecodeDateTime function.
> >
> > ```c
> > ...
> > int t = 0;
> > int *tzp = &t;
> > ...
> > if (tzp != NULL)
> > ...
> >
> > if (tzp == NULL)
>
> That's not really a bug, but should certainly be improved.
>
> At fault is commit 635a0b9a864, which removed "tzp" as a parameter from
> "DecodeDateTime" and replaced it with a constant pointer to 0, when it
> should have done more, like remove the variable and its uses.
>
> Do you want to write a patch?
>
> Yours,
> Laurenz Albe

On Wed, 2023-12-06 at 19:44 +0500, RekGRpth wrote:
> something like?

Yes, perhaps - although the variable name "tzp" = "time zone pointer"
is not appropriate any more.

Perhaps the "tzp" variable could be left as it is, and only the
comparisons "if (tzp == NULL)" be removed.

Yours,
Laurenz Albe

Fixed!

ср, 6 дек. 2023 г. в 21:38, Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at>:
>
> On Wed, 2023-12-06 at 19:44 +0500, RekGRpth wrote:
> > something like?
>
> Yes, perhaps - although the variable name "tzp" = "time zone pointer"
> is not appropriate any more.
>
> Perhaps the "tzp" variable could be left as it is, and only the
> comparisons "if (tzp == NULL)" be removed.
>
> Yours,
> Laurenz Albe
>

On Wed, 2023-12-06 at 21:41 +0500, RekGRpth wrote:
> Fixed!

That patch looks good to me.

Yours,
Laurenz Albe

On Thu, Dec 7, 2023 at 12:41 AM RekGRpth <rekgrpth(at)gmail(dot)com> wrote:

> Fixed!

Nice catch.

There are several places in DecodeDateTime() where the value of '*tzp'
is changed. It seems to me that these assignments are unnecessary since
the value of '*tzp' is not utilized anywhere in the code. Can we also
remove these assignments?

Thanks
Richard

Fixed!

чт, 7 дек. 2023 г. в 06:52, Richard Guo <guofenglinux(at)gmail(dot)com>:
>
>
> On Thu, Dec 7, 2023 at 12:41 AM RekGRpth <rekgrpth(at)gmail(dot)com> wrote:
>>
>> Fixed!
>
>
> Nice catch.
>
> There are several places in DecodeDateTime() where the value of '*tzp'
> is changed. It seems to me that these assignments are unnecessary since
> the value of '*tzp' is not utilized anywhere in the code. Can we also
> remove these assignments?
>
> Thanks
> Richard

On Thu, Dec 07, 2023 at 09:51:57AM +0800, Richard Guo wrote:
> There are several places in DecodeDateTime() where the value of '*tzp'
> is changed. It seems to me that these assignments are unnecessary since
> the value of '*tzp' is not utilized anywhere in the code. Can we also
> remove these assignments?

*tzp could be given to DecodeTimezone() and manipulated inside it, but
you are right that we don't use it at all in the DecodeDateTime()
path. Other callers of DecodeTimezone() may depend on the changes
done inside it, though. Perhaps DecodeTimezone() should be extended
so as it can accept a NULL value and use that in DecodeDateTime(),
eliminating the need for *tzp entirely?
--
Michael

Fixed!

чт, 7 дек. 2023 г. в 07:32, Michael Paquier <michael(at)paquier(dot)xyz>:
>
> On Thu, Dec 07, 2023 at 09:51:57AM +0800, Richard Guo wrote:
> > There are several places in DecodeDateTime() where the value of '*tzp'
> > is changed. It seems to me that these assignments are unnecessary since
> > the value of '*tzp' is not utilized anywhere in the code. Can we also
> > remove these assignments?
>
> *tzp could be given to DecodeTimezone() and manipulated inside it, but
> you are right that we don't use it at all in the DecodeDateTime()
> path. Other callers of DecodeTimezone() may depend on the changes
> done inside it, though. Perhaps DecodeTimezone() should be extended
> so as it can accept a NULL value and use that in DecodeDateTime(),
> eliminating the need for *tzp entirely?
> --
> Michael

Michael Paquier <michael(at)paquier(dot)xyz> writes:
> *tzp could be given to DecodeTimezone() and manipulated inside it, but
> you are right that we don't use it at all in the DecodeDateTime()
> path. Other callers of DecodeTimezone() may depend on the changes
> done inside it, though. Perhaps DecodeTimezone() should be extended
> so as it can accept a NULL value and use that in DecodeDateTime(),
> eliminating the need for *tzp entirely?

TBH, I think our best path here is to leave well enough alone.
The main impact of the proposed changes is to make ecpg's copy
of the datetime parsing code diverge even further from the
backend's copy. I don't think that these changes buy anything
meaningful, and what they will do is make it harder to reunify
that logic, which somebody will probably try to do someday.

(The recent changes to support non-error-throwing returns from
input functions probably made that task notably easier, BTW.)

regards, tom lane