RE: row filtering for logical replication

On Thursday, January 13, 2022 2:49 PM Peter Smith <smithpb2250@gmail.com>
> Thanks for posting the merged v63.
> 
> Here are my review comments for the v63-0001 changes.
> 
> ~~~

Thanks for the comments!

> 1. src/backend/replication/logical/proto.c - logicalrep_write_tuple
> 
>   TupleDesc desc;
> - Datum values[MaxTupleAttributeNumber];
> - bool isnull[MaxTupleAttributeNumber];
> + Datum    *values;
> + bool    *isnull;
>   int i;
>   uint16 nliveatts = 0;
> 
> Those separate declarations for values / isnull are not strictly
> needed anymore, so those vars could be deleted. IIRC those were only
> added before when there were both slots and tuples. OTOH, maybe you
> prefer to keep it this way just for code readability?

Yes, I prefer the current style for code readability.

> 
> 2. src/backend/replication/pgoutput/pgoutput.c - typedef
> 
> +typedef enum RowFilterPubAction
> +{
> + PUBACTION_INSERT,
> + PUBACTION_UPDATE,
> + PUBACTION_DELETE,
> + NUM_ROWFILTER_PUBACTIONS  /* must be last */
> +} RowFilterPubAction;
> 
> This typedef is not currently used by any of the code.
> 
> So I think choices are:
> 
> - Option 1: remove the typedef, because nobody is using it.
> 
> - Option 2: keep the typedef, but use it! e.g. everywhere there is an
> exprstate array index variable probably it should be declared as a
> 'RowFilterPubAction idx' instead of just 'int idx'.

Thanks, I used the option 1.

> 
> 3. src/backend/replication/pgoutput/pgoutput.c - map_changetype_pubaction
> 
> After this recent v63 refactoring and merging of some APIs it seems
> that the map_changetype_pubaction is now ONLY used by
> pgoutput_row_filter function. So this can now be a static member of
> pgoutput_row_filter function instead of being declared at file scope.
> 

Changed.

> 
> 4. src/backend/replication/pgoutput/pgoutput.c - pgoutput_row_filter
> comments
> The function header comment says the same thing 2x about the return values.
> 

Changed.

> 
> ~~~
> 
> 5. src/backend/replication/pgoutput/pgoutput.c - pgoutput_row_filter
> 
> + ExprContext    *ecxt;
> + int filter_index = map_changetype_pubaction[*action];
> +
> + /* *action is already assigned default by caller */
> + Assert(*action == REORDER_BUFFER_CHANGE_INSERT ||
> +    *action == REORDER_BUFFER_CHANGE_UPDATE ||
> +    *action == REORDER_BUFFER_CHANGE_DELETE);
> +
> 
> Accessing the map_changetype_pubaction array should be done *after* the
> Assert.
> 
> ~~~

Changed.

> 
> 6. src/backend/replication/pgoutput/pgoutput.c - pgoutput_row_filter
> 
> ExprState *filter_exprstate;
> ...
> filter_exprstate = entry->exprstate[map_changetype_pubaction[*action]];
> 
> Please consider what way you think is best.

Changed as suggested.

> 
> 7. src/backend/replication/pgoutput/pgoutput.c - pgoutput_row_filter
> There are several things not quite right with that comment:
> a. I thought now it should refer to "slots" instead of "tuples"
> 
> Suggested replacement comment:

Changed but I prefer "tuple" which is easy to understand.

> ~~~
> 
> 8. src/backend/replication/pgoutput/pgoutput.c - pgoutput_row_filter
> 
> + if (!new_slot || !old_slot)
> + {
> + ecxt->ecxt_scantuple = new_slot ? new_slot : old_slot;
> + result = pgoutput_row_filter_exec_expr(entry->exprstate[filter_index],
> +    ecxt);
> +
> + FreeExecutorState(estate);
> + PopActiveSnapshot();
> +
> + return result;
> + }
> +
> + tmp_new_slot = new_slot;
> + slot_getallattrs(new_slot);
> + slot_getallattrs(old_slot);
> 
> I think after this "if" condition then the INSERT, DELETE and simple
> UPDATE are already handled. So, the remainder of the code is for
> deciding what update transformation is needed etc.
> 
> I think there should be some block comment somewhere here to make that
> more obvious.

Changed.
> ~~
> 
> 9. src/backend/replication/pgoutput/pgoutput.c - pgoutput_row_filter
> 
> Many of the comments in this function are still referring to old/new
> "tuple". Now that all the params are slots instead of tuples maybe now
> all the comments should also refer to "slots" instead of "tuples".
> Please search all the comments - e.g. including all the "Case 1:" ...
> "Case 4:" comments.

I also think tuple still makes sense, so I didn’t change this.

> 
> 10. src/backend/replication/pgoutput/pgoutput.c - pgoutput_row_filter_init
> 
> + int idx_ins = PUBACTION_INSERT;
> + int idx_upd = PUBACTION_UPDATE;
> + int idx_del = PUBACTION_DELETE;
> 
> These variables are unnecessary now... They previously were added only
> as short synonyms because the other enum names were too verbose (e.g.
> REORDER_BUFFER_CHANGE_INSERT) but now that we have shorter enum
> names
> like PUBACTION_INSERT we can just use those names directly
> 
Changed.

> 
> 11. src/backend/replication/pgoutput/pgoutput.c - pgoutput_row_filter_init
> 
> I felt that the code would seem more natural if the
> pgoutput_row_filter_init function came *before* the
> pgoutput_row_filter function in the source code.
> 
Changed.

> 
> 12. src/backend/replication/pgoutput/pgoutput.c - pgoutput_change
> 
> @@ -634,6 +1176,9 @@ pgoutput_change(LogicalDecodingContext *ctx,
> ReorderBufferTXN *txn,
>   RelationSyncEntry *relentry;
>   TransactionId xid = InvalidTransactionId;
>   Relation ancestor = NULL;
> + ReorderBufferChangeType modified_action = change->action;
> + TupleTableSlot *old_slot = NULL;
> + TupleTableSlot *new_slot = NULL;
> 
> It seemed a bit misleading to me to call this variable
> 'modified_action' since mostly it is not modified at all.
> 
> IMO it is better just to call this as 'action' but then add a comment
> (above the "switch (modified_action)") to say the previous call to
> pgoutput_row_filter may have transformed it to a different action.
> 
Changed.

I have included these changes in v64 patch set.

Best regards,
Hou zj