On Thu, Nov 27, 2025 at 12:33 AM Peter Smith <[email protected]> wrote:
>
> Hi Sawada-San.
>
> Some review comments for v30-0001.
Thank you for the comments!
> ======
> src/backend/replication/logical/logicalctl.c
>
> 5.
> + * In the future, we could extend support to include automatic transitions
>
> Sawada-San reply to previous "XXX" comment:
> Hmm, I've seen we use an "XXX" marker like "TODO" or "FIXME", but what
> the comment explains is not actually a todo but possible future
> enhancements.
>
> ~
>
> You can search the source code for regex “XXX.*future” to find
> multiple examples just like this.
Yes, but I can also see some examples where we don't have "XXX"
marker. So I don't see why we should add it in this patch.
> ~
>
> 6b.
> Would it be better to have a self-documenting enum {UNKNOWN, ENABLED,
> DISABLED} instead of some magic int values that need lots of comments
> explaining how to use them?
No, we should not cast enum values directly to a boolean. We have a
precedent of such usage like LocalXLogInsertAllowed, and I don't think
we can reduce the comments even if we use a self-documenting enum
since the usage is complicated. We could consider #define values 0, 1,
and -1 but it seems to be overkill to this usage.
>
> ~~~
>
> LogicalDecodingCtlShmemInit:
>
> 7.
> +void
> +LogicalDecodingCtlShmemInit(void)
> +{
> + bool found;
> +
> + LogicalDecodingCtl = ShmemInitStruct("Logical decoding control",
> + LogicalDecodingCtlShmemSize(),
> + &found);
> +
> + if (!found)
> + MemSet(LogicalDecodingCtl, 0, LogicalDecodingCtlShmemSize());
> +}
>
> Maybe assign LogicalDecodingCtlShmemSize() to some variable; no need
> to invoke it 2x.
It's actually inlined so we don't need to have an additional variable here .
> InvalidatePossiblyObsoleteSlot:
>
> 11.
> /* Let caller know */
> - *invalidated = true;
> + invalidated = true;
>
> This was a review v29 (#12) review comment that seems to have been
> missed. That comment is stale (should be removed) because
> 'invalidated' is no longer returned by reference like before.
Yes, but it doesn't change the fact that the function lets the caller
know using this variable if the slot has been invalidated, no?
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
