On Tue, Feb 11, 2025 at 8:49 AM Peter Smith <smithpb2...@gmail.com> wrote: > > Hi Nisha. > > Some review comments about v74-0001 > > ====== > src/backend/replication/slot.c > > 1. > /* Maximum number of invalidation causes */ > -#define RS_INVAL_MAX_CAUSES RS_INVAL_WAL_LEVEL > - > -StaticAssertDecl(lengthof(SlotInvalidationCauses) == (RS_INVAL_MAX_CAUSES + > 1), > - "array length mismatch"); > +#define RS_INVAL_MAX_CAUSES (lengthof(InvalidationCauses)-1) > > The static assert was here to protect against dev mistakes in keeping > the lookup table up-to-date with the enum of slot.h. So it's not a > good idea to remove it... > > IMO the RS_INVAL_MAX_CAUSES should be relocated to slot.h where the > enum is defined and where the devs know exactly how many invalidation > types there are. Then this static assert can be put back in to do its > job of ensuring the integrity properly again for this lookup table. >
How about keeping RS_INVAL_MAX_CAUSES dynamic in slot.c (as it was) and updating the static assert to ensure the lookup table stays up-to-date with the enums? The change has been implemented in v75. > ~~~ > > ====== Here are v75 patches addressing comments in [1], [2] and [3]. - A new function, "EvaluateSlotInvalidationCause()", has been introduced to separate the invalidation_cause evaluation logic from InvalidatePossiblyObsoleteSlot(). - Also, another new inline function "CalculateTimeDuration()" added as suggested in [3]. [1] https://www.postgresql.org/message-id/CAHut%2BPvsvHWoiEkGTP4NfVNsADsy-Jan3Dvp%2B_GW3gmPDHf5Qw%40mail.gmail.com [2] https://www.postgresql.org/message-id/OS0PR01MB57163889BE5F9F30DD3318A394FD2%40OS0PR01MB5716.jpnprd01.prod.outlook.com [3] https://www.postgresql.org/message-id/CAA4eK1LuvXa6sVj3xuLoe2X%3D0xjbJXrnJePbpXQZaTMws8pZqg%40mail.gmail.com -- Thanks, Nisha
v75-0001-Introduce-inactive_timeout-based-replication-slo.patch
Description: Binary data
v75-0002-Add-TAP-test-for-slot-invalidation-based-on-inac.patch
Description: Binary data
