Dear Amit,
I created updated patch. I did some self-reviewing and updated.
> One disadvantage of the approach you suggest is that the changes in
> the new queue won't be accounted for in logical_decoding_work_mem
> computation, which can be done in the proposed approach, although the
> patch hasn't implemented it as of now.
This part is not implemented yet, because it is under discussion. Roughly considered,
we must consider several points - it might be complex:
1. What is the ordering of serialization? Should we prioritize spilling
invalidation messages, or normal changes?
2. What is the filename? Can we use same one as changes?
3. IIUC we shouldn't stream inval messages even when stream=on/parallel case.
Isn't it hacky?
> A few comments on v4:
> ===================
> 1.
> +static void
> +ReorderBufferExecuteInvalidationsInQueue(ReorderBuffer *rb,
> ReorderBufferTXN *txn)
> +{
> ...
> ...
> + /* Skip other changes because the transaction was aborted */
> + case REORDER_BUFFER_CHANGE_INSERT:
> + case REORDER_BUFFER_CHANGE_UPDATE:
> + case REORDER_BUFFER_CHANGE_DELETE:
> + case REORDER_BUFFER_CHANGE_MESSAGE:
> + case REORDER_BUFFER_CHANGE_INTERNAL_SNAPSHOT:
> + case REORDER_BUFFER_CHANGE_INTERNAL_COMMAND_ID:
> + case REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID:
> + case REORDER_BUFFER_CHANGE_INTERNAL_SPEC_INSERT:
> + case REORDER_BUFFER_CHANGE_INTERNAL_SPEC_CONFIRM:
> + case REORDER_BUFFER_CHANGE_INTERNAL_SPEC_ABORT:
> + case REORDER_BUFFER_CHANGE_TRUNCATE:
> + break;
>
> This cases should never happen, so we should have either an Assert or
> an elog here.
Fixed. Switch was removed and Assert() was added.
> 2.
> +void
> +ReorderBufferAddInvalidationsExtended(ReorderBuffer *rb, TransactionId xid,
> + XLogRecPtr lsn, Size nmsgs,
> + SharedInvalidationMessage *msgs,
> + bool needs_distribute)
> {
> ...
> + * XXX: IIUC This must be done only to the toptxns, but is it right?
> + */
> + if (!needs_distribute && !TransactionIdIsValid(txn->toplevel_xid))
> + {
> + ReorderBufferChange *inval_change;
> +
> + /* Duplicate the inval change to queue it */
> + inval_change = ReorderBufferAllocChange(rb);
> + inval_change->action = REORDER_BUFFER_CHANGE_INVALIDATION;
>
> The name needs_distribute seems confusing because when this function
> is invoked from SnapBuildDistributeSnapshotAndInval(), the parameter
> is passed as false; it should be true in that case, and the meaning of
> this parameter in the function should be reversed.
Fixed.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
