Re: [PATCH] /src/backend/access/transam/xlog.c, tiny improvements

Em dom., 26 de jan. de 2020 às 23:48, Mark Dilger <
mark(dot)dilger(at)enterprisedb(dot)com> escreveu:

> > On Jan 24, 2020, at 12:48 PM, Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:
> >
> > 3. At function KeepLogSeg (line 9357) the test if (slotSegNo <= 0), the
> var slotSegNo is uint64 and not can be < 0.
>
> There is something unusual about comparing XLogSegNo variable in this
> way, but it seems to go back to 2014 when the replication slots were
> introduced in commit 858ec11858a914d4c380971985709b6d6b7dd6fc, and
> XLogSegNo was unsigned then, too. Depending on how you look at it, this
> could be a thinko, or it could be defensive programming against future
> changes to the XLogSegNo typedef. I’m betting it was defensive
> programming, given the context. As such, I don’t think it would be
> appropriate to remove this defense in your patch.
>
while in general terms I am in favor of defensive programming, it is not
needed everywhere.
I am surprised that the final code contains a lot of code that is not
actually executed.
It should be an interesting exercise to debug postgres, which I haven't
done yet.
See variables being declared, functions called, memories being touched,
none of which occurs in production.

In the case of XLogSegNo, it is almost impossible to change it to signed,
as this would limit the number of segments that could be used.
Even so, the test could be removed and put into comment, the warning that
in the future, in case of change to signed, it should be tested less than
zero.

regards,
Ranier Vilela