On Sat, Dec 4, 2021 at 10:13 AM Euler Taveira <eu...@eulerto.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