From: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
---|---|
To: | "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> |
Cc: | "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com>, Peter Smith <smithpb2250(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Dilip Kumar <dilipbalaut(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Bruce Momjian <bruce(at)momjian(dot)us>, Julien Rouhaud <rjuju123(at)gmail(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> |
Subject: | Re: [PoC] pg_upgrade: allow to upgrade publisher node |
Date: | 2023-10-23 08:30:00 |
Message-ID: | CALj2ACX+E0QCyM99tez0tyoVBpdmFz4qp3J7yiLnx4ffY9mq2w@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Mon, Oct 23, 2023 at 11:10 AM Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Hayato Kuroda (Fujitsu) | 2023-10-23 08:57:19 | RE: pg_ctl start may return 0 even if the postmaster has been already started on Windows |
Previous Message | Jeevan Chalke | 2023-10-23 07:20:55 | Re: More new SQL/JSON item methods |