Dear Shlok,
Thanks for updating patches! Below are my comments, maybe only for 0002.
01. General
IIUC, we are not discussed why ALTER SUBSCRIPTION ... SET include_generated_columns
is prohibit. Previously, it seems okay because there are exclusive options. But now,
such restrictions are gone. Do you have a reason in your mind? It is just not considered
yet?
02. General
According to the doc, we allow to alter a column to non-generated one, by ALTER
TABLE ... ALTER COLUMN ... DROP EXPRESSION command. Not sure, what should be
when the command is executed on the subscriber while copying the data? Should
we continue the copy or restart? How do you think?
03. Tes tcode
IIUC, REFRESH PUBLICATION can also lead the table synchronization. Can you add
a test for that?
04. Test code (maybe for 0001)
Please test the combination with TABLE ... ALTER COLUMN ... DROP EXPRESSION command.
05. logicalrep_rel_open
```
+ /*
+ * In case 'include_generated_columns' is 'false', we should skip the
+ * check of missing attrs for generated columns.
+ * In case 'include_generated_columns' is 'true', we should check if
+ * corresponding column for the generated column in publication column
+ * list is present in the subscription table.
+ */
+ if (!MySubscription->includegencols && attr->attgenerated)
+ {
+ entry->attrmap->attnums[i] = -1;
+ continue;
+ }
```
This comment is not very clear to me, because here we do not skip anything.
Can you clarify the reason why attnums[i] is set to -1 and how will it be used?
06. make_copy_attnamelist
```
+ gencollist = palloc0(MaxTupleAttributeNumber * sizeof(bool));
```
I think this array is too large. Can we reduce a size to (desc->natts * sizeof(bool))?
Also, the free'ing should be done.
07. make_copy_attnamelist
```
+ /* Loop to handle subscription table generated columns. */
+ for (int i = 0; i < desc->natts; i++)
```
IIUC, the loop is needed to find generated columns on the subscriber side, right?
Can you clarify as comment?
08. copy_table
```
+ /*
+ * Regular table with no row filter and 'include_generated_columns'
+ * specified as 'false' during creation of subscription.
+ */
```
I think this comment is not correct. After patching, all tablesync command becomes
like COPY (SELECT ...) if include_genereted_columns is set to true. Is it right?
Can we restrict only when the table has generated ones?
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/
