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.
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
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/
