Обсуждение: Remove redundant assignment of a variable in function AlterPublicationTables

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.

[1]: https://www.postgresql.org/message-id/CALdSSPgiDj8S8POf_6kiUkVrt9-EqYmn%2B2ahDqpdBpyteeAz-Q%40mail.gmail.com

Thanks,
Shlok Kyal

On Wed, Aug 20, 2025 at 05:18:53PM +0800, Chao Li wrote:
> If we look into the subsequent functions, yes, “isnull” will always
> be assigned. But how about if someone incidentally changed a
> subsequent function and moved an assignment?
> I think giving an initial value is a good habit without much
> performance burden.

It does not matter to leave the code as is.  We have a bunch of these
depending on how people feel on the day when they implement something.

Compilers are smart enough to optimize such things away when they don't
matter, usually.  Some compiler versions and flags can also be very
picky with initializations not done, missing that sometimes a variable
is actually set in all the relevant code paths.  Warnings can show
depending on the complexity of the branches used, as well.
--
Michael

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

On Thu, Aug 21, 2025 at 10:11 AM Michael Paquier <michael@paquier.xyz> wrote:
...
>
> It does not matter to leave the code as is.  We have a bunch of these
> depending on how people feel on the day when they implement something.
>

Hi Michael.

Some declaration assignments may be arbitrary, but I think 'isNull'
follows a well-established pattern in PG source.

FWIW, here are some search results:

Search for "&isNull[,)]" ==> ~427

Search for declarations assignments like
"bool\s*isNull\s*=\s*(true|false)" ==> ~12

Of those 12 declarations, I found only ~4 that go on to pass
unconditionally to functions by &isNull:
- AlterPublicationTables() in publicationcmds.c (this patch)
- percentile_disc_multi_final() in orderedsetaggs.c
- CatalogCacheComputeTupleHashValue() in catcache.c
- pltcl_returnnext() in pltcl.c

To summarise:
Only ~4 places are redundantly assigning isNull values before calling
functions that use &isNull.
But ~400 function calls that are passing &isNull do not pre-assignment
of that variable.

Choosing to keep this patch would be consistent with 99% of existing examples.

~~

Aside:

Of course, I recognise this is a small change that may not seem worth
the discussion overhead. However, I feel it is indicative of a problem
that's worth addressing.

During patch reviews, when I suggest fixing small issues alongside the
main changes (e.g. "let's clean up this redundant assignment while
we're here"), the typical response is that unrelated changes should be
submitted separately to keep patches focused.

But when these minor fixes are submitted as standalone patches,
they're often dismissed as too trivial to be worth the review effort
and commit overhead.

This creates a catch-22 where small but legitimate improvements never
get addressed. They fall into a gap where changes are either "too
small to include" or "too small to stand alone." This leaves valid
improvements in limbo indefinitely.

Do you have any advice for how to handle such changes?

======
Kind Regards,
Peter Smith.
Futjisu Australia

On Fri, Aug 22, 2025 at 09:55:23AM +1000, Peter Smith wrote:
> To summarise:
> Only ~4 places are redundantly assigning isNull values before calling
> functions that use &isNull.
> But ~400 function calls that are passing &isNull do not pre-assignment
> of that variable.
>
> Choosing to keep this patch would be consistent with 99% of existing examples.

Sure, but I don't quite see what really requires fixing here.  Even if
there's an "inconsistency" with variable manipulation pattern, it
does not present a risk of becoming broken if we begin to manipulate
or refactor this area of the code, nor do we need to force one rule
across the whole source code.  I'd suggest to keep this one for
patches that rework the whole area, if these churns prove to be
annoying.

If somebody else wants to come around and apply this patch, I'm OK
with that, of course, my opinion is just one voice among many others.
--
Michael