On Mon, Dec 4, 2023 at 8:01 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> On Fri, Dec 1, 2023 at 11:24 PM vignesh C <vignes...@gmail.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
