Dear Hou,
Thanks for updating the patch. Here are my comments.
01.
```
<para>
A <firstterm>sequence synchronization worker</firstterm> will be started
- after executing any of the above subscriber commands, and will exit once the
- sequences are synchronized.
+ after executing any of the above subscriber commands. The worker will
+ remain running for the life of the subscription, periodically
+ synchronizing all published sequences.
</para>
```
I think it's not accurate, because REFRESH SEQUENCE command does not need the
sequencesync worker anymore.
02.
```
void
GetSequence(Relation seqrel, int64 *last_value, bool *is_called)
```
I think GetSequence() itself should conitan the permission check like
SetSequence().
My idea is to set NULL for last_value and is_called in this case.
03.
```
/*
* Verify that the current user has SELECT privilege on the
sequence.
* This is required to read the sequence state below.
*/
aclresult = pg_class_aclcheck(seqoid, GetUserId(), ACL_SELECT);
if (aclresult != ACLCHECK_OK)
return COPYSEQ_INSUFFICIENT_PERM;
/* Get current local sequence state */
GetSequence(sequence_rel, &local_last_value, &local_is_called);
```
If you accept above comment, this part can be simplified.
04.
```
/*
* get_and_validate_seq_info
*
* Extracts remote sequence information from the tuple slot received from the
* publisher, and validates it against the corresponding local sequence
* definition.
*/
static CopySeqResult
get_and_validate_seq_info(TupleTableSlot *slot, Relation *sequence_rel,
LogicalRepSequenceInfo
**seqinfo, int *seqidx,
List *seqinfos)
```
It can return COPYSEQ_SUCCESS, but it might be misleading; copying is not
happened yet here. How about returning boolean and add another argument to
indicate the reason if the validation is failed?
05.
LogicalRepSyncSequences() starts the transaction and read sequences every time.
Can we cache the seqinfos to reuse in the next iteration? My idea is to
introduce
a syscache callback for the pg_subscription_relto invalidate the cached list.
How about measuring performance once and considering it's a good improvement?
Best regards,
Hayato Kuroda
FUJITSU LIMITED
