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
