Re: Signed vs Unsigned (take 2) (src/backend/storage/ipc/procarray.c)

On Thu, 15 Jul 2021 at 23:38, Aleksander Alekseev
<aleksander(at)timescale(dot)com> wrote:
> I'm updating the status to "Ready for Committer".

I think that might be a bit premature. I can't quite see how changing
the pids List to a const List makes any sense, especially when the
code goes and calls lappend_int() on it to assign it some different
value.

There are also problems in BackendPidGetProcWithLock around consts.

Much of this patch kinda feels like another one of those "I've got a
fancy new static analyzer" patches. Unfortunately, it just introduces
a bunch of compiler warnings as a result of the changes it makes.

I'd suggest splitting each portion of the patch out into parts related
to what it aims to achieve. For example, it looks like there's some
renaming going on to remove a local variable from shadowing a function
parameter. Yet the patch is claiming performance improvements. I
don't see how that part relates to performance. The changes to
ProcArrayClearTransaction() seem also unrelated to performance.

I'm not sure what the point of changing things like for (int i =0...
to move the variable declaration somewhere else is about. That just
seems like needless stylistic changes that achieve nothing but more
headaches for committers doing backpatching work.

I'd say if this patch wants to be taken seriously it better decide
what it's purpose is, because to me it looks just like a jumble of
random changes that have no clear purpose.

I'm going to set this back to waiting on author.

David