From: | "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> |
---|---|
To: | "Wei Wang (Fujitsu)" <wangw(dot)fnst(at)fujitsu(dot)com> |
Cc: | Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>, Julien Rouhaud <rjuju123(at)gmail(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, 'Peter Smith' <smithpb2250(at)gmail(dot)com> |
Subject: | RE: [PoC] pg_upgrade: allow to upgrade publisher node |
Date: | 2023-05-22 10:20:31 |
Message-ID: | TYAPR01MB5866657547769CABAC6FAE2CF5439@TYAPR01MB5866.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Dear Wang,
Thank you for reviewing! PSA new version.
> For patches 0001
>
> 1. The latest patch set fails to apply because the new commit (0245f8d) in HEAD.
I didn't notice that. Thanks, fixed.
> 2. In file pg_dump.h.
> ```
> +/*
> + * The LogicalReplicationSlotInfo struct is used to represent replication
> + * slots.
> + *
> + * XXX: add more attributes if needed
> + */
> +typedef struct _LogicalReplicationSlotInfo
> +{
> + DumpableObject dobj;
> + char *plugin;
> + char *slottype;
> + bool twophase;
> +} LogicalReplicationSlotInfo;
> ```
>
> Do we need the structure member "slottype"? It seems we do not use "slottype"
> because we only dump logical replication slot.
As you said, this attribute is not needed. This is a garbage of previous efforts.
Removed.
> For patch 0002
>
> 3. In the function SaveSlotToPath
> ```
> - /* and don't do anything if there's nothing to write */
> - if (!was_dirty)
> + /*
> + * and don't do anything if there's nothing to write, unless it's this is
> + * called for a logical slot during a shutdown checkpoint, as we want to
> + * persist the confirmed_flush_lsn in that case, even if that's the only
> + * modification.
> + */
> + if (!was_dirty && !is_shutdown && !SlotIsLogical(slot))
> ```
> It seems that the code isn't consistent with our expectation.
> If this is called for a physical slot during a shutdown checkpoint and there's
> nothing to write, I think it will also persist physical slots to disk.
You meant to say that we should not change handlings for physical case, right?
> For patch 0003
>
> 4. In the function check_for_parameter_settings
> ```
> + /* --include-logical-replication-slots can be used since PG 16. */
> + if (GET_MAJOR_VERSION(new_cluster->major_version < 1600))
> + return;
> ```
> It seems that there is a slight mistake (the input of GET_MAJOR_VERSION) in the
> if-condition:
> GET_MAJOR_VERSION(new_cluster->major_version < 1600)
> ->
> GET_MAJOR_VERSION(new_cluster->major_version) <= 1500
>
> Please also check the similar if-conditions in the below two functions
> check_for_confirmed_flush_lsn (in 0003 patch)
> check_are_logical_slots_active (in 0004 patch)
Done. I grepped with GET_MAJOR_VERSION, and confirmed they were fixed.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
Attachment | Content-Type | Size |
---|---|---|
v15-0001-pg_upgrade-Add-include-logical-replication-slots.patch | application/octet-stream | 32.7 KB |
v15-0002-Always-persist-to-disk-logical-slots-during-a-sh.patch | application/octet-stream | 4.8 KB |
v15-0003-pg_upgrade-Add-check-function-for-include-logica.patch | application/octet-stream | 5.7 KB |
v15-0004-Change-the-method-used-to-check-logical-replicat.patch | application/octet-stream | 7.0 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Hayato Kuroda (Fujitsu) | 2023-05-22 11:49:12 | RE: Time delayed LR (WAS Re: logical replication restrictions) |
Previous Message | torikoshia | 2023-05-22 09:48:37 | unnecessary #include "pg_getopt.h"? |