On Wed, Aug 20, 2025 at 6:41 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> Hi,
>
> While working on the thread [1]. We encountered that in the
> 'AlterPublicationTables' function, the assignment 'isnull = true' is
> redundant. This assignment is not required, and the variable will be
> reassigned before use.
> I have attached a patch to address this.
>
Since I gave this review comment in the first place, I feel obliged to say +1
I feel that having redundant assignments can be misleading because
someone reading the code might think the initial value matters or has
some significance, when it does not.
~~~
Here's another example, in the same function:
----------
PublicationRelInfo *newpubrel;
Oid newrelid;
Bitmapset *newcolumns = NULL;
----------
None of those initial values above is needed because these variables
are all immediately/unconditionally reassigned. So, why is 'newpubrel'
not assigned up-front, but 'newcolumns' is assigned? IMO this implies
that the 'newcolumns' initial value has some meaning (which it
doesn't), so it just introduces unnecessary doubts for the reader...
YMMV.
======
Kind Regards,
Peter Smith
Futjisu Australia