Re: row filtering for logical replication

On Sat, Dec 4, 2021 at 10:13 AM Euler Taveira <euler(at)eulerto(dot)com> wrote:
>
> On Thu, Dec 2, 2021, at 4:18 AM, Peter Smith wrote:
>
> PSA a new v44* patch set.
>
...

> I used the last patch series (v44) posted by Peter Smith [1]. I did a lot of
> improvements in this new version (v45). I merged 0001 (it is basically the main
> patch I wrote) and 0004 (autocomplete). As I explained in [2], I implemented a
> patch (that is incorporated in the v45-0001) to fix this issue. I saw that
> Peter already proposed a slightly different patch (0006). I read this patch and
> concludes that it would be better to keep the version I have. It fixes a few
> things and also includes more comments.
> [1] https://postgr.es/m/CAHut%2BPtJnnM8MYQDf7xCyFAp13U_0Ya2dv-UQeFD%3DghixFLZiw%40mail.gmail.com
> [2] https://postgr.es/m/ca8d270d-f930-4d15-9f24-60f95b364173%40www.fastmail.com

>> As I explained in [2], I implemented a
patch (that is incorporated in the v45-0001) to fix this issue. I saw that
Peter already proposed a slightly different patch (0006). I read this patch and
concludes that it would be better to keep the version I have. It fixes a few
things and also includes more comments.

Your ExprState exprstate array code is essentially exactly the same
logic that was int patch v44-0006 isn't it?

The main difference I saw was
1. I pass the cache index (e.g. IDX_PUBACTION_DELETE etc) to the
pgoutput_filter, but
2. You are passing in the ReorderBufferChangeType value.

IMO the ability to directly access the cache array is more efficient.

The function is called for every row operation (e.g. consider x 1
million rows) so I felt the overhead to have unnecessary if/else
should be avoided.
e.g.
------
if (action == REORDER_BUFFER_CHANGE_INSERT)
result = pgoutput_row_filter_exec_expr(entry->exprstate[0], ecxt);
else if (action == REORDER_BUFFER_CHANGE_UPDATE)
result = pgoutput_row_filter_exec_expr(entry->exprstate[1], ecxt);
else if (action == REORDER_BUFFER_CHANGE_DELETE)
result = pgoutput_row_filter_exec_expr(entry->exprstate[2], ecxt);
else
Assert(false);
------

Why not just use a direct index like was in patch v44-0006 in the first place?
e.g.
------
result = pgoutput_row_filter_exec_expr(entry->exprstate[idx_pubaction], ecxt);
------

Conveniently, those ReorderBufferChangeType first 3 enums are the ones
you want so you can still pass them if you want.
REORDER_BUFFER_CHANGE_INSERT,
REORDER_BUFFER_CHANGE_UPDATE,
REORDER_BUFFER_CHANGE_DELETE,

Just use them to directly index into entry->exprstate[action] and so
remove the excessive if/else.

What do you think?

------
Kind Regards,
Peter Smith.
Fujitsu Australia