On Sun, Jun 23, 2024 at 10:28 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Hi Shubham,
>
> Thanks for sharing new patch! You shared as v9, but it should be v10, right?
> Also, since there are no commitfest entry, I registered [1]. You can rename the
> title based on the needs. Currently CFbot said OK.
>
> Anyway, below are my comments.
>
> 01. General
> Your patch contains unnecessary changes. Please remove all of them. E.g.,
>
> ```
> " s.subpublications,\n");
> -
> ```
> And
> ```
> appendPQExpBufferStr(query, " o.remote_lsn AS suboriginremotelsn,\n"
> - " s.subenabled,\n");
> + " s.subenabled,\n");
> ```
>
> 02. General
> Again, please run the pgindent/pgperltidy.
>
> 03. test_decoding
> Previously I suggested to the default value of to be include_generated_columns
> should be true, so you modified like that. However, Peter suggested opposite
> opinion [3] and you just revised accordingly. I think either way might be okay, but
> at least you must clarify the reason why you preferred to set default to false and
> changed accordingly.
I have set the default value as true in case of test_decoding. The
reason for this is even before the new feature implementation,
generated columns were getting selected.
> 04. decoding_into_rel.sql
> According to the comment atop this file, this test should insert result to a table.
> But added case does not - we should put them at another place. I.e., create another
> file.
>
> 05. decoding_into_rel.sql
> ```
> +-- when 'include-generated-columns' is not set
> ```
> Can you clarify the expected behavior as a comment?
>
> 06. getSubscriptions
> ```
> + else
> + appendPQExpBufferStr(query,
> + " false AS subincludegencols,\n");
> ```
> I think the comma is not needed.
> Also, this error meant that you did not test to use pg_dump for instances prior PG16.
> Please verify whether we can dump subscriptions and restore them accordingly.
>
> [1]: https://commitfest.postgresql.org/48/5068/
> [2]:
https://www.postgresql.org/message-id/OSBPR01MB25529997E012DEABA8E15A02F5E52%40OSBPR01MB2552.jpnprd01.prod.outlook.com
> [3]: https://www.postgresql.org/message-id/CAHut%2BPujrRQ63ju8P41tBkdjkQb4X9uEdLK_Wkauxum1MVUdfA%40mail.gmail.com
All the comments are handled.
The attached Patches contains all the suggested changes.
Thanks and Regards,
Shubham Khanna.
