Re: pg_upgrade and logical replication

On Mon, Dec 4, 2023 at 8:01 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Fri, Dec 1, 2023 at 11:24 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > The attached v22 version patch has the changes for the same.
> >
>
> I have made minor changes in the comments and code at various places.
> See and let me know if you are not happy with the changes. I think
> unless there are more suggestions or comments, we can proceed with
> committing it.
>

It seems the patch is already close to ready-to-commit state but I've
had a look at the v23 patch with fresh eyes. It looks mostly good to
me and there are some minor comments:

---
+ tup = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
+ if (!HeapTupleIsValid(tup))
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("relation %u does not exist", relid));
+ ReleaseSysCache(tup);

Given what we want to do here is just an existence check, isn't it
clearer if we use SearchSysCacheExists1() instead?

---
+ query = createPQExpBuffer();
+ appendPQExpBuffer(query, "SELECT srsubid, srrelid,
srsubstate, srsublsn"
+ " FROM
pg_catalog.pg_subscription_rel"
+ " ORDER BY srsubid");
+ res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
+

Probably we don't need to use PQExpBuffer here since the query to
execute is a static string.

---
+# The subscription's running status should be preserved. Old subscription
+# regress_sub1 should be enabled and old subscription regress_sub2 should be
+# disabled.
+$result =
+ $new_sub->safe_psql('postgres',
+ "SELECT subenabled FROM pg_subscription ORDER BY subname");
+is( $result, qq(t
+f),
+ "check that the subscription's running status are preserved");
+

How about showing the subname along with the subenabled so that we can
check if each subscription is in an expected state in case where
something error happens?

---
+# Subscription relations should be preserved
+$result =
+ $new_sub->safe_psql('postgres',
+ "SELECT count(*) FROM pg_subscription_rel WHERE srsubid = $sub_oid");
+is($result, qq(2),
+ "there should be 2 rows in pg_subscription_rel(representing
tab_upgraded1 and tab_upgraded2)"
+);

Is there any reason why we check only the number of rows in
pg_subscription_rel? I guess it might be a good idea to check if table
OIDs there are also preserved.

---
+# Enable the subscription
+$new_sub->safe_psql('postgres', "ALTER SUBSCRIPTION regress_sub2 ENABLE");
+$publisher->wait_for_catchup('regress_sub2');
+

IIUC after making the subscription regress_sub2 enabled, we will start
the initial table sync for the table tab_upgraded2. If so, shouldn't
we use wait_for_subscription_sync() instead?

---
+# Create another subscription and drop the subscription's replication origin
+$old_sub->safe_psql('postgres',
+ "CREATE SUBSCRIPTION regress_sub4 CONNECTION '$connstr'
PUBLICATION regress_pub3 WITH (enabled=false)"

It's better to put spaces before and after '='.

---
+my $subid = $old_sub->safe_psql('postgres',
+ "SELECT oid FROM pg_subscription WHERE subname = 'regress_sub4'");

I think we can reuse $sub_oid.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com