Some review comments for v78-0001.
======
src/backend/replication/slot.c
ReportSlotInvalidation:
1.
+ int minutes;
+ int secs;
The variables 'minutes' and 'seconds' are only used by case
RS_INVAL_IDLE_TIMEOUT, so I think it would be better to make a new
code block for that case where you can declare and initialise these in
one go at that scope.
~~~
DetermineSlotInvalidationCause:
2.
+ if (SlotIsLogical(s) &&
+ /* invalid DB oid signals a shared relation */
+ (dboid == InvalidOid || dboid == s->data.database))
+ {
The comment placement in the master code was ok because then there
were different statements, but now in this patch multiple conditions
are combined, and this comment seems strangely placed.
~~~
GetSlotInvalidationCause:
3.
I understand your argument "let's not change anything unless
absolutely necessary for our patch", but in this case since half the
function is changing anyway it seems a missed opportunity to not
simplify the rest of the code "in passing" to make it consistent with
the newly added partner function GetSlotInvalidationCauseName. My
question is "if not now, then when?", because I expect a future patch
to do this might be rejected as being too trivial, so by not changing
now probably these functions are doomed to be inconsistent forever.
Anyway it is just my opinion -- leave it as-is if you wish.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
