Few comments for 0002:
1)
+ /*
+ * Allow synchronization if the remote sequence data has a more recent
+ * LSN than the local state, or if no local LSN exists yet.
+ */
+ if (!XLogRecPtrIsValid(local_page_lsn) ||
+ local_page_lsn < seqinfo->page_lsn)
+ return COPYSEQ_ALLOWED;
/*
* Skip synchronization if the current user does not have sufficient
* privileges to read the sequence data.
@@ -384,6 +418,40 @@ validate_seqsync_state(LogicalRepSequenceInfo
*seqinfo, Relation sequence_rel)
if (!GetSequence(sequence_rel, &local_last_value, &local_is_called))
return COPYSEQ_INSUFFICIENT_PERM;
Shouldn't the 'COPYSEQ_INSUFFICIENT_PERM' check be the first one, else
we may end up allowing the copy due to the first check even if the
user has no privileges? Or let me know if I am missing something here.
2)
+ ereport(WARNING,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("skipped synchronizing the sequence \"%s.%s\"",
+ seqinfo->nspname, seqinfo->seqname),
+ seqinfo->increment
+ ? errdetail("The local last_value %lld is ahead of the one on publisher",
+ (long long int) local_last_value)
+ : errdetail("The local last_value %lld is behind of the one on publisher",
+ (long long int) local_last_value));
This error message and DETAIL might not be very clear to the user.
IMO, it may not be easy for the user to deduce that a sequence is
descending and thus it being "behind" is the reason for skipping
synchronization.
Shall we update ERROR msg to say ascending/descending
ERROR: skipped synchronizing ascending sequence <name>
ERROR: skipped synchronizing descending sequence <name>
Since we use descending/ascending in sequence doc many times, I feel
it is okay to use it in ERROR messages too to give more clarity.
Thoughts?
3)
A <firstterm>sequence synchronization worker</firstterm> will be started
- after executing any of the above subscriber commands. The worker will
- remain running for the life of the subscription, periodically
- synchronizing all published sequences.
A <firstterm>sequence synchronization worker</firstterm> will be started
+ after executing <command>ALTER SUBSCRIPTION ... REFRESH
PUBLICATION</command>
+ command. The worker will remain running for the life of the subscription,
+ periodically synchronizing all published sequences.
Why have we changed this to mention REFRESH PUB alone responsible for
starting worker? From user's point of view, even CREATE SUB (which has
sequences) will also start the worker no?
thanks
Shveta
