| From: | "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> |
|---|---|
| To: | 'Ajin Cherian' <itsajin(at)gmail(dot)com>, Rui Zhao <zhaorui126(at)gmail(dot)com> |
| Cc: | shveta malik <shveta(dot)malik(at)gmail(dot)com>, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | RE: [PATCH] Preserve replication origin OIDs in pg_upgrade |
| Date: | 2026-08-14 05:29:22 |
| Message-ID: | OS9PR01MB12149F1F09E710EF9E005A24DF5DA2@OS9PR01MB12149.jpnprd01.prod.outlook.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Dear Ajin,
Thanks for updating the patch. I think the patch series is mostly good.
Few cosmetic comments:
```
+/*
+ * This will be set by the pg_upgrade_support function --
+ * binary_upgrade_set_next_pg_subscription_oid().
+ */
+Oid binary_upgrade_next_pg_subscription_oid = InvalidOid;
```
Other binary_upgade_next variables have the code comment
"Potentially set by pg_upgrade_support functions", isn't it enough?
```
--- a/src/bin/pg_upgrade/pg_upgrade.c
+++ b/src/bin/pg_upgrade/pg_upgrade.c
@@ -35,6 +35,9 @@
*
* We control all assignments of pg_database.oid because we want the directory
* names to match between the old and new cluster.
+ *
+ * We control assignment of pg_subscription.oid because we want the oid to
+ * match between the old and new cluster.
```
Indent of added lines were not correct.
```
- * needing out-of-line storage. If you add a TOAST table to this catalog,
- * be sure to set up a snapshot everywhere it might be needed. For more
+ * needing out-of-line storage. If you add a TOAST table to this catalog,
+ * be sure to set up a snapshot everywhere it might be needed. For more
```
This change looks unrelated, can we retain?
```
- true /* indexOK */ ,
+ true /* indexOK */,
```
This change looks unrelated, can we retain?
```
+#include "utils/relcache.h"
```
This seems to be added only for referring the data structure Relation. If so,
can we avoid by adding like below?
```
-#include "utils/relcache.h"
+
+/* avoid including utils/relcache.h */
+typedef struct RelationData *Relation;
```
Best regards,
Hayato Kuroda
FUJITSU LIMITED
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Nikolay Samokhvalov | 2026-08-14 05:31:59 | Prevent premature startup of pg_upgrade targets |
| Previous Message | Denis Smirnov | 2026-08-14 05:19:58 | Re: Redundant qualifier elimination |