RE: Pgoutput not capturing the generated columns

From: "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com>
To: 'Shlok Kyal' <shlok(dot)kyal(dot)oss(at)gmail(dot)com>
Cc: Shubham Khanna <khannashubham1197(at)gmail(dot)com>, Peter Smith <smithpb2250(at)gmail(dot)com>, Rajendra Kumar Dangwal <dangwalrajendra888(at)gmail(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, "euler(at)eulerto(dot)com" <euler(at)eulerto(dot)com>
Subject: RE: Pgoutput not capturing the generated columns
Date: 2024-06-25 13:19:21
Message-ID: OSBPR01MB25526C6ADFEA2448FAE07AADF5D52@OSBPR01MB2552.jpnprd01.prod.outlook.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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/

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Eisentraut 2024-06-25 13:21:37 Re: pg_combinebackup --clone doesn't work
Previous Message Andres Freund 2024-06-25 13:06:59 Re: Vacuum ERRORs out considering freezing dead tuples from before OldestXmin