Hi Shubham, As you can see, most of my recent review comments for patch 0001 are only cosmetic nitpicks. But, there is still one long-unanswered design question from a month ago [1, #G.2]
A lot of the patch code of pgoutput.c and proto.c and logicalproto.h is related to the introduction and passing everywhere of new 'include_generated_columns' function parameters. These same functions are also always passing "BitMapSet *columns" representing the publication column list. My question was about whether we can't make use of the existing BMS parameter instead of introducing all the new API parameters. The idea might go something like this: * If 'include_generated_columns' option is specified true and if no column list was already specified then perhaps the relentry->columns can be used for a "dummy" column list that has everything including all the generated columns. * By doing this: -- you may be able to avoid passing the extra 'include_gernated_columns' everywhere -- you may be able to avoid checking for generated columns deeper in the code (since it is already checked up-front when building the column list BMS) ~~ I'm not saying this design idea is guaranteed to work, but it might be worth considering, because if it does work then there is potential to make the current 0001 patch significantly shorter. ====== [1] https://www.postgresql.org/message-id/CAHut%2BPsuJfcaeg6zst%3D6PE5uyJv_UxVRHU3ck7W2aHb1uQYKng%40mail.gmail.com Kind Regards, Peter Smith. Fujitsu Australia
