On Mon, Oct 23, 2023 at 11:10 AM Hayato Kuroda (Fujitsu)
<kuroda.hay...@fujitsu.com> wrote:
>
> Thank you for reviewing! PSA new version.
> > 6. A nit: how about is_decodable_txn or is_decodable_change or some
> > other instead of just a plain name processing_required?
> > + /* Do we need to process any change in 'fast_forward' mode? */
> > + bool processing_required;
>
> I preferred current one. Because not only decodable txn, non-txn change and
> empty transactions also be processed.
Right. It's not the txn, but the change. processing_required seems too
generic IMV. A nit: is_change_decodable or something?
Thanks for the patch. Here are few comments on v56 patch:
1.
+ *
+ * Although this function is currently used only during pg_upgrade, there are
+ * no reasons to restrict it, so IsBinaryUpgrade is not checked here.
This comment isn't required IMV, because anyone looking at the code
and callsites can understand it.
2. A nit: IMV "This is a special purpose ..." statement seems redundant.
+ *
+ * This is a special purpose function to ensure that the given slot can be
+ * upgraded without data loss.
How about
Verify that the given replication slot has consumed all the WAL changes.
If there's any decodable WAL record after the slot's
confirmed_flush_lsn, the slot's consumer will lose that data after the
slot is upgraded.
Returns true if there are no decodable WAL records after the
confirmed_flush_lsn. Otherwise false.
3.
+ if (PG_ARGISNULL(0))
+ elog(ERROR, "null argument to
binary_upgrade_validate_wal_records is not allowed");
I can see the above style is referenced from
binary_upgrade_create_empty_extension, but IMV the following looks
better and latest (ereport is new style than elog)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("replication slot name cannot be null")));
4. The following comment seems frivolous, the code tells it all.
Please remove the comment.
+
+ /* No need to check this slot, seek to new one */
+ continue;
5. A typo - s/gets/Gets
+ * gets the LogicalSlotInfos for all the logical replication slots of the
6. An optimization in count_old_cluster_logical_slots(void): Turn
slot_count to a function static variable so that the for loop isn't
required every time because the slot count is prepared in
get_old_cluster_logical_slot_infos only once and won't change later
on. Do you see any problem with the following? This saves a few CPU
cycles when there are large number of replication slots.
{
static int slot_count = 0;
static bool first_time = true;
if (first_time)
{
for (int dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
slot_count += old_cluster.dbarr.dbs[dbnum].slot_arr.nslots;
first_time = false;
}
return slot_count;
}
7. A typo: s/slotname/slot name. "slot name" looks better in user
visible messages.
+ pg_log(PG_VERBOSE, "slotname: \"%s\", plugin: \"%s\", two_phase: %s",
8.
+else
+{
+ test_upgrade_from_pre_PG17($old_publisher, $new_publisher,
+ @pg_upgrade_cmd);
+}
Will this ever be tested in current TAP test framework? I mean, will
the TAP test framework allow testing upgrades from one PG version to
another PG version?
9. A nit: Can single quotes around variable names in the comments be
removed just to be consistent?
+ * We also skip decoding in 'fast_forward' mode. This check must be last
+ /* Do we need to process any change in 'fast_forward' mode? */
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
