Some review comments for patch v2-0001.
======
Commit message
1.
Currently we can copy an invalidated logical and physical replication slot
using functions 'pg_copy_logical_replication_slot' and
'pg_copy_physical_replication_slot' respectively.
With this patch we will throw an error in such cases.
/we can copy an invalidated logical and physical replication slot/we
can copy invalidated logical and physical replication slots/
======
doc/src/sgml/func.sgml
pg_copy_physical_replication_slot:
2.
- is omitted, the same value as the
source slot is used.
+ is omitted, the same value as the source slot is used. Copy of an
+ invalidated physical replication slot in not allowed.
Typo /in/is/
Also, IMO you don't need to say "physical replication slot" because it
is clear from the function's name.
SUGGESTION
Copy of an invalidated slot is not allowed.
~~~
pg_copy_logical_replication_slot:
3.
+ Copy of an invalidated logical replication slot in not allowed.
Typo /in/is/
Also, IMO you don't need to say "logical replication slot" because it
is clear from the function's name.
SUGGESTION
Copy of an invalidated slot is not allowed.
======
src/backend/replication/slotfuncs.c
copy_replication_slot:
4.
+ /* Check if source slot was invalidated while copying of slot */
+ SpinLockAcquire(&src->mutex);
+ first_slot_contents = *src;
+ SpinLockRelease(&src->mutex);
+
+ src_isinvalidated = (first_slot_contents.data.invalidated != RS_INVAL_NONE);
+
+ if (src_isinvalidated)
+ ereport(ERROR,
+ (errmsg("could not copy replication slot \"%s\"",
+ NameStr(*src_name)),
+ errdetail("The source replication slot was invalidated during the
copy operation.")));
4a.
We already know that it was not invalid the FIRST time we looked at
it, so IMO we only need to confirm that the SECOND look gives the same
answer. IOW, I thought the code should be like below. (AFAICT
Sawada-san's review says the same as this).
Also, I think it is better to say "became invalidated" instead of "was
invalidated", to imply the state was known to be ok before the copy.
SUGGESTION
+ /* Check if source slot became invalidated during the copy operation */
+ if (second_slot_contents.data.invalidated != RS_INVAL_NONE)
+ ereport(ERROR,
~
4b.
Unnecessary parentheses in the ereport.
~
4c.
There seems some weird mix of tense "cannot copy" versus "could not
copy" already in this file. But, maybe at least you can be consistent
within the patch and always say "cannot".
======
Kind Regards,
Peter Smith.
Fujitsu Australia
