From: | "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> |
---|---|
To: | 'Shubham Khanna' <khannashubham1197(at)gmail(dot)com> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | RE: Improve the error message for logical replication of regular column to generated column. |
Date: | 2024-11-15 02:49:16 |
Message-ID: | TYAPR01MB569230B7BF79C08284FEB706F5242@TYAPR01MB5692.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Dear Shubham,
Thanks for creating a patch! I checked yours and I have comments.
01.
```
+ StringInfoData gencolsattsbuf;
+ int generatedatts = 0;
+
+ initStringInfo(&gencolsattsbuf);
```
gencolsattsbuf is initialized at the beginning but won't be free'd.
But I prefer the Peter's suggestion - you can combine the reporting stuff to
logicalrep_report_missing_attrs and rename the function. This is clearer than
directly adding declarations and ereport() in logicalrep_rel_open().
02.
```
+ /*
+ * Check if the subscription table generated column has
+ * same name as a non-generated column in the
+ * corresponding publication table.
+ */
```
I don't think this comment is correct. The error can be reported even when
both publisher and subscriber has the generated column, right?
Also, I feel comments can be located atop "if".
03.
I feel if you combine the reporting stuff with logicalrep_report_missing_attrs, some
of changes are not needed anymore. You can just add comment in logicalrep_rel_open
and modify the message in logicalrep_report_missing_attrs.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
From | Date | Subject | |
---|---|---|---|
Next Message | Pavan Deolasee | 2024-11-15 03:05:48 | Re: Potential ABI breakage in upcoming minor releases |
Previous Message | torikoshia | 2024-11-15 01:51:58 | Re: Change COPY ... ON_ERROR ignore to ON_ERROR ignore_row |